Re: Actualize kafka dissector

[Martin Mathieson <martin.r.mathieson () googlemail com>]

On Tue, Nov 15, 2016 at 5:05 PM, Dmitry Lazurkin <dilaz03 () gmail com> wrote:
> Hello.
>
> > - attaching per-packet kafka info and looking it up all over the place?
> How to working with per-packet data? Which functions i should use for it?

I don't think we should use per-packet info to remember what the
api_version is, but as below just pass it down into called functions.

> > 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?
> I think this is two independent tasks.
>
> Ok. I understood following:
> - Add array of structs about supported min_version and max_version for
> each request/response. Parse only header for packates with unsupported
> version. Add expert info to unsupported packets.

v2 brings very few changes to fields, I'd rather see it all implemented than
track which methods are not supported or not.

> - Support latest kafka protocol. Better pass api_version to each function.

Yes, I think so.  Once its been passed down into all of the functions
it'll be there
to be used if needed.

> How to test kafka dissector? Catch real kafka session? Randpkt? Unit-tests?

Real captures we care about supporting are the most important.
You could also run through a tutorial such as this one
(https://www.tutorialspoint.com/apache_kafka/apache_kafka_basic_operations.htm)
and capture the traffic.

> Thanks to all.
>
> On 11/15/2016 12:53 PM, Martin Mathieson wrote:
> > 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 () netscape net> 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 () gmail com>
> > > To: Developer support list for Wireshark <wireshark-dev () wireshark org>
> > > 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 () wireshark org>
> > > Archives: https://www.wireshark.org/lists/wireshark-dev
> > > Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
> > > mailto:wireshark-dev-request () wireshark org?subject=unsubscribe
> > >
> > > ___________________________________________________________________________
> > > Sent via:    Wireshark-dev mailing list <wireshark-dev () wireshark org>
> > > Archives:    https://www.wireshark.org/lists/wireshark-dev
> > > Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
> > >              mailto:wireshark-dev-request () wireshark org?subject=unsubscribe
> > ___________________________________________________________________________
> > Sent via:    Wireshark-dev mailing list <wireshark-dev () wireshark org>
> > Archives:    https://www.wireshark.org/lists/wireshark-dev
> > Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
> >              mailto:wireshark-dev-request () wireshark org?subject=unsubscribe
>
>
>
> ___________________________________________________________________________
> Sent via:    Wireshark-dev mailing list <wireshark-dev () wireshark org>
> Archives:    https://www.wireshark.org/lists/wireshark-dev
> Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
>              mailto:wireshark-dev-request () wireshark org?subject=unsubscribe
___________________________________________________________________________
Sent via:    Wireshark-dev mailing list <wireshark-dev () wireshark org>
Archives:    https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
             mailto:wireshark-dev-request () wireshark org?subject=unsubscribe