Wireshark · Wireshark-dev: Re: [Wireshark-dev] Actualize kafka dissector

[Martin Mathieson <martin.r.mathieson@xxxxxxxxxxxxxx>]

Hi, coming to this discussion late..

I added support for api_version 1 a few months ago when I needed to
see the decode of those messages.  I haven't yet needed to work with
api_version 2, but as I remember it also changes several of the
formats, including the 'kafka message' headers themselves - so this
would involve propagating api_version into many more functions (I did
the bare minimum for api_version 1, and have only done the messages I
was interested in at the time...).

The alternative to passing this api_version would maybe be:
- a global variable...?
- attaching per-packet kafka info and looking it up all over the place?

I agree that its not pretty, but I think propagating api_version down
into all of the functions that need it is probably the least bad way
to go.  I don't remember how big the protocol changes are, but I
wonder how much longer it would take to support api_version 2 than to
warn that we don't support it?  Having said that, it would be good to
detect and not try to dissect messages belonging to future protocol
versions we also don't support yet.

Regards,
Martin

On Tue, Nov 15, 2016 at 2:32 AM, Michael Mann <mmann78@xxxxxxxxxxxx> wrote:
> The answer is typically "follow the coding convention of the dissector if
> modifying an existing dissector".  And when most dissectors are created,
> they start by using the example packet-PROTOABBREV.c (which explicitly lists
> out the parameters).  I don't think the performance is an issue either way.
>
> The Wireshark dissector architecture is very conducive to copy/paste, which
> also perpetuates the convention of explicitly listing out the parameters (so
> proto_tree_add_xxx functions only need very minimal changes if copying from
> another dissector).  This is also why I don't think it's "extra typing" to
> list the parameters out because you're probably just doing from copy/paste
> shortcut keys.
>
> Wireshark does have a ptvcursor API that can save you some of the "standard"
> function parameters, but based on your example, I don't think it'll help
> that much.
>
>
>
> -----Original Message-----
> From: Dmitry Lazurkin <dilaz03@xxxxxxxxx>
> To: Developer support list for Wireshark <wireshark-dev@xxxxxxxxxxxxx>
> Sent: Mon, Nov 14, 2016 6:04 pm
> Subject: Re: [Wireshark-dev] Actualize kafka dissector
>
> Second question about using "API version" value in dissect functions.
> This value may change parsing of packet. For now it passed as argument
> to some dissect functions. But for now it used only in root dissect
> function of request/response type (not passed to nested functions). I
> have next solutions for this:
>
> ---------------------------------------
> Use current solution: always pass api_version variable to all functions
> and subfunctions.
>
> Too many arguments for all functions. May be this is normal (:
>
> ---------------------------------------
> Create context structure and pass it to functions/subfunctions:
>
> typedef struct _kafka_context_t {
> tvbuff_t *tvb;
> packet_info *pinfo;
> guint16 api_version;
> int offset;
> } kafka_context_t;
>
> offset field is not necessary. Create instance of this struct in root
> dissect function as local variable and pass it to dissect functions by
> pointer. Use context like this:
>
> proto_tree_add_item(tree, hf_kafka_string_len, ctx->tvb, ctx->offset, 2,
> ENC_BIG_ENDIAN);
>
> Here i have question. Are many pointer dereferences perfomance issue?
>
> ---------------------------------------
>
> Create separate function for parsing each version of packet with code
> duplication. Too much code but may be better for supporting.
>
>
> I like context but passing version as separate argument is good too.
> Which solution is better in wireshark community?
>
>
> ___________________________________________________________________________
> Sent via: Wireshark-dev mailing list <wireshark-dev@xxxxxxxxxxxxx>
> Archives: https://www.wireshark.org/lists/wireshark-dev
> Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
> mailto:wireshark-dev-request@xxxxxxxxxxxxx?subject=unsubscribe
>
> ___________________________________________________________________________
> Sent via:    Wireshark-dev mailing list <wireshark-dev@xxxxxxxxxxxxx>
> Archives:    https://www.wireshark.org/lists/wireshark-dev
> Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
>              mailto:wireshark-dev-request@xxxxxxxxxxxxx?subject=unsubscribe