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

[Dmitry Lazurkin <dilaz03@xxxxxxxxx>]

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 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.
- Support latest kafka protocol. Better pass api_version to each function.


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

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