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 #22 from Jeff Morriss <jeff.morriss.ws@xxxxxxxxx>  2009-09-10 10:37:53 PDT ---
(In reply to comment #21)
> > 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 offset increases inside struct_SPANFIX; offsetp is a pointer to the offset.
> 
> > struct_Event() has the same problem.
> 
> The UNUSED() macro (already defined in packet-x11.c, not in this patch)
> increments the offset.

Ah, good point. OK.

> > 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?
> 
> The user will be told "Unknown opcode %d" already by col_append_fstr before the
> switch. It was a while ago, but I think I intentionally omitted the default
> case because telling the user the same thing again would be redundant. The only
> thing left to do is mark the bits UNDECODED(), but dissect_x11_request already
> does that for me.

OK.  Maybe a comment would be in order in case someone in the future raises the
same question?

> > 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
> 
> Yes, that's it exactly.

OK

> > 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?
> 
> The hf_ entry is statically declared, so overwriting it is a no-go. I suppose
> we could move to a dynamically allocated list of hf_entries, or just output
> that one line by hand (not using an hf_ entry).
> 
> I was trying to touch the existing dissector as little as possible. Would you
> like me to look into this for v6 of the patch?

No, I guess there's nothing clean that can be done about that.  I just figured
I'd mention it.

> > 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?
> 
> Maybe it could/should. Again, I was trying to touch the existing dissector as
> little as possible. Would you like me to look into that for v6 of the patch?

Not a big deal, I just noticed it.  You're the one using the dissector, so
whatever is useful for you.


Looking over the changes for V6, it looks like trivial stuff (check_col, the
NULL strings in the hf_ entries, and maybe the default-case comment).  I could
fix that stuff tonight if you don't beat me to it.


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