Wireshark · Wireshark-dev: Re: [Wireshark-dev] New dissector to submit - how best to do it?

[Evan Huus <eapache@xxxxxxxxx>]

On Fri, Jan 31, 2014 at 3:29 PM, David Ameiss <netshark@xxxxxxxxxxxxx> wrote:
> On 01/31/2014 02:03 PM, Evan Huus wrote:
>>
>> On Fri, Jan 31, 2014 at 2:33 PM, David Ameiss <netshark@xxxxxxxxxxxxx>
>> wrote:
>>>
>>> On 01/31/2014 01:03 PM, Evan Huus wrote:
>>>>
>>>>
>>>> Wow, that sounds awesome.
>>>>
>>>> Question: What is the current layout/structure of the new code? I
>>>> imagine one c file per dissector in epan/dissectors/, but where are
>>>> all the other c files? Are the capabilities they add shared only
>>>> between your protocols, or might they be useful to other protocols?
>>>> How big/complex are they? More details on this kind of thing will help
>>>> us figure out how best to integrate.
>>>
>>>
>>>
>>> Right now all (non-GUI) files are in epan/dissectors. Most of the
>>> "support"
>>> files are small - the largest is 991 lines, the smallest is 37.
>>>
>>> Some background:
>>> - There is a single "resolver" or "advertising" protocol, which is used
>>> to
>>> resolve topic names to actual sources publishing on that topic. This
>>> protocol runs on UDP, either multicast or unicast.
>>> - There are 7 different data protocols, 2 of which are IPC, and one of
>>> which
>>> is RDMA. So 4 different actual wire protocols. But the resolver protocol
>>> (above) advertises even the "uncaptureable" protocols. These are
>>> essentially
>>> "transport" protocols.
>>> - Under each transport protocol is a common "channel" protocol - every
>>> transport protocol message reduces to this protocol.
>>>
>>> The support files are used to:
>>> - Track transport sessions - instances of the various transport protocols
>>> -
>>> to allow correlation between a transport and a topic name
>>> - Analyze the UDP-based transport sessions - sequence number analysis,
>>> rate
>>> calculation, and so forth.
>>> - Follow "conversations" for the TCP-based transport protocols
>>>
>>> I spent a considerable amount of time looking through the Wireshark
>>> source
>>> to find existing code to do these things, before inventing my own. It's
>>> still possible I simply missed existing code.
>>
>>
>> Wireshark does already have substantial code for conversation
>> tracking, see section 2.2 of README.dissector.
>>
>> We also have code for name resolution (DNS et al), which may be
>> adaptable for your resolver protocol.
>>
>> What kind of operations are you trying to do that you couldn't find
>> functions for?
>>
>
> Our conversations are determined by values within the message, and can be
> multi-hop (through a software gateway). Looking at the WS conversation code,
> either it didn't appear to do what we needed, or I just couldn't grok what
> it was doing. :-)
>
> For the resolver stuff - we get the topic names as part of the
> advertisements. That part is fairly simple: store the name and transport
> information. When a transport packet is seen, lookup the name based on the
> transport info.
>
> As for the rest... maybe the best approach is to submit the dissector code,
> then wait for the slew of questions about "why did you do it this way". I
> would welcome such questions and observations/suggestions. By no means do I
> assume I did things the best possible way :-)

I think this whole process is what Gerrit is supposed to make easier,
so we might as well use it :)

>>> One thought I had (and I've seen mentioned occasionally on the mailing
>>> list)
>>> is creating a separate subdirectory under epan/dissectors for dissectors
>>> with a large number of support files. It would certainly reduce the
>>> "clutter".
>>
>>
>> I wouldn't mind this, but there was fairly strong push-back last time
>> it was brought up on the list. YMMV.
>>
>>>>
>>>> Suggestion: Please please please read through the latest
>>>> README.developer and README.dissector and make sure you follow all the
>>>> things therein. 90% of the review comments I make are things that are
>>>> already mentioned in those documents, so making sure you follow them
>>>> makes things go much smoother. Also make sure your code passes the
>>>> various scripts (tools/check*) and fuzz-testing as well
>>>> (http://wiki.wireshark.org/FuzzTesting). If you have any questions
>>>> about style etc please ask in advance rather than wait for somebody to
>>>> catch it on review.
>>>
>>>
>>>
>>> Been doing that for quite a while. Unless those have significantly
>>> changed
>>> in the last 6 months or so, the code should be up to standards.
>>
>>
>> I don't think there's been a lot of churn, it's just surprising how
>> many people miss them completely. Glad to hear you're on top of it :)
>>
>>>>
>>>> Hope this helps,
>>>> Evan
>>>>
>>>> On Fri, Jan 31, 2014 at 1:47 PM, David Ameiss <netshark@xxxxxxxxxxxxx>
>>>> wrote:
>>>>>
>>>>>
>>>>> We've developed a set of dissectors for my company's protocols - all
>>>>> based
>>>>> on UDP and TCP. I've gotten the OK to submit them to Wireshark, and
>>>>> have
>>>>> spent the last 2 months tracking the development changes, keeping
>>>>> things
>>>>> current, and just finished moving over to the new git/gerrit approach.
>>>>>
>>>>> The issue is that these new dissectors are quite substantial - 8
>>>>> separate
>>>>> dissectors, 40 files (22 .c, 18 .h), containing nearly 20,000 lines of
>>>>> code.
>>>>> I also have some GUI additions (originally done for GTK, now in Qt) to
>>>>> provide analysis and stats capabilities for these dissectors. That adds
>>>>> another 6 files and 7,000+ lines of code (plus the 3 .ui files).
>>>>>
>>>>> As there is a large amount of common functionality that has been
>>>>> factored
>>>>> out into separate modules (hence the large number of files), adding in
>>>>> small
>>>>> pieces is not practical. The GUI component is obviously independent,
>>>>> and
>>>>> can
>>>>> be submitted separately once the dissector component is integrated.
>>>>>
>>>>> Or, I can submit the whole thing at once.
>>>>>
>>>>> What's the best approach to ensure the code gets reviewed, rather than
>>>>> completely overwhelming the reviewers? :-)
>>>>>
>>>>> --
>>>>> David Ameiss
>>>>> netshark@xxxxxxxxxxxxx
>>>>>
>>>>>
>>>>> ___________________________________________________________________________
>>>>> Sent via:    Wireshark-dev mailing list <wireshark-dev@xxxxxxxxxxxxx>
>>>>> Archives:    http://www.wireshark.org/lists/wireshark-dev
>>>>> Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
>>>>>
>>>>> mailto:wireshark-dev-request@xxxxxxxxxxxxx?subject=unsubscribe
>>>
>>>
>>>
>>>
>>> --
>>> David Ameiss
>>> netshark@xxxxxxxxxxxxx
>
>
>
> --
> David Ameiss
> netshark@xxxxxxxxxxxxx