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.
