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

[David Ameiss <netshark@xxxxxxxxxxxxx>]

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 :-)

> > 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