Wireshark · Wireshark-bugs: [Wireshark-bugs] [Bug 2981] Patch to add extension support to the X11 dissector

[bugzilla-daemon@xxxxxxxxxxxxx]

https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=2981





--- Comment #20 from Jeff Morriss <jeff.morriss.ws@xxxxxxxxx>  2009-09-09 19:03:45 PDT ---
(In reply to comment #18)
>  - indents generated code for 'struct' functions properly.

Cool; I hope that wasn't too much of a pain, it was just a minor complaint.

>  - implements xcb <union> support, prompted by:
>  - omits unused structure dissectors via the brute-force method of blacklisting
> the ones that are never used.

Hmmm, I wonder if I misunderstood something.  I saw some functions marked as
"static _U_ void" so I thought the script knew what wasn't being used.  But,
now that the brute-force code is there...


Reading more code, I have concerns about:

+static void struct_TRAP(tvbuff_t *tvb, int *offsetp, proto_tree *root, int
little_endian, int count)
+{
+    int i;
+    for (i = 0; i < count; i++) {
+          proto_item *item;
+       proto_tree *t;   
+
+          item = proto_tree_add_item(root, hf_x11_struct_TRAP, tvb, *offsetp,
24, little_endian);
+          t = proto_item_add_subtree(item, ett_x11_rectangle);
+          struct_SPANFIX(tvb, offsetp, t, little_endian, 1);
+          struct_SPANFIX(tvb, offsetp, t, little_endian, 1);
+    }
+}

Note how there's a 'count' but the offset never increases.  The loop will end
eventually but it could take a long time (since no exception will be thrown
when the offset gets too big for the TVB).  Should there not be a count or
should there be an offset increment (like there is elsewhere)?  struct_Event()
has the same problem.


In dispatch_composite() (and other dispatch functions) there's no default case
in the switch statement. Should we tell the user "unrecognized minor number" or
something?  Actually, most of the switch statements don't have default cases. 
Is that intended?

What's the point of code like:

+    f_grab_window = VALUE32(tvb, *offsetp);
+    proto_tree_add_item(t, hf_x11_xinput_GrabDeviceKey_grab_window, tvb,
*offsetp, 4, little_endian);

Why pull the value from the tvb if it's not going to be used?  In this function
one of the values _is_ used, but only one.  I guess it just doesn't know which
value will be used until it gets to the end of the function?  Ah, maybe that's
this comment?

+#TODO
+# - look ahead to see if values are ever used again before creating an "int"
in the output


You can remove all the calls to check_col()--it's no longer needed.

The hf_ entries need to have the empty strings ("") replaced with NULL:

+{ &hf_x11_above_sibling, { "above-sibling", "x11.above-sibling", FT_UINT32,
BASE_HEX, NULL, 0, "", HFILL }},



When I look in the sample captures provided, I see that the 'opcode' doesn't
get decoded right in the tree:

: X11, Request, opcode: 140 (XKEYBOARD)
:     opcode: Unknown (140)  <<<< here
:         undecoded

The top line decodes the opcode correctly because it's using
'state->opcode_vals', but the hf_ entry is still using 'opcode_vals'.  Can
anything be done about that?


Looking at some QueryExtension packets, I noticed that major-opcode doesn't
have VALS entry (meaning that you only see the value, not the name of the
opcode)--could/should it?

Anyway, I think that's about it.  Sorry for all the delay(s).  Looks like
you've done good work here.


-- 
Configure bugmail: https://bugs.wireshark.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.