https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=2981
--- Comment #21 from Peter Harris <peter.harris@xxxxxxxxxxxxxxx> 2009-09-10 10:09:17 PDT ---
(In reply to comment #20)
> 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.
> 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.
> 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.
> You can remove all the calls to check_col()--it's no longer needed.
Okay.
> 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 }},
Hmm? Oh, I see. process-x11-fields.pl has changed since then. I'll fix that for
v6.
> 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?
> 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?
> Looks like you've done good work here.
Thanks.
--
Configure bugmail: https://bugs.wireshark.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
