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

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

On Tue, Nov 15, 2016 at 5:05 PM, Dmitry Lazurkin <dilaz03@xxxxxxxxx> 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@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
>> ___________________________________________________________________________
>> 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