Re: Dissector code feedback request (Cassandra CQL)

["Aaron Ten Clay" <wireshark-dev () aarontc com>]

On 08 Dec 2015, you wrote:
> 2015-12-08 22:46 GMT+01:00 Aaron Ten Clay <wireshark-dev () aarontc com>:
>
> > On 03 Dec 2015, you wrote:
> >
> > > On Thu, Dec 3, 2015 at 9:27 AM, <wireshark-dev () aarontc com> wrote:
> > >
> > > > Hello everyone,
> > > >
> > > > I've started cobbling together a dissector plugin for the CQL binary
> > > > protocol used by Apache Cassandra. I'm brand new to Wireshark
> > development,
> > > > so I'm sure some patterns could be improved. I'm hoping to get some
> > > > feedback on what I have so far:
> > https://gist.githubusercontent.com/aarontc/d285047c78b4b2a3c1d3/raw/4eff8ed1c6ba342434eae770a3921ae656a3d3d7/packet-cql.c
> > > > Any suggestions/criticism would be appreciated. If this dissector
> > would be
> > > > useful to anyone else, I'd like to see about getting it included with
> > core
> > > > Wireshark once any issues are resolved and the dissector is more
> > feature
> > > > complete.
> > > >
> > > > Thanks,
> > > > -Aaron
> > > Hi Aaron,
> > >
> > > for feedback/review, the better is push your patch on Gerrit (
> > > https://code.wireshark.org/review ), and core can be directly review you
> > > code (if you want to don't merge soon you can WIP flag...)
> > >
> > > Cheers
> >
> > Hi Alexis,
> >
> > Thanks for the feedback. I've uploaded the current state of the code to
> > Gerrit:
> > https://code.wireshark.org/review/#/c/12479/1/plugins/cql/packet-cql.c
> >
> > It still needs work but I'm definitely interested in early feedback if
> > there are
> > better patterns I could follow, etc.
> >
> > Thanks!
> > -Aaron
>
> Hi Aaron,
>
> at first look the dissector seems ok (will need a better review, for now I
> just had a basic overview). Some comments though:
> - please convert it to a builtin dissector: we are not integrating new
> plugins
> - given that you use a port number not assigned by IANA, you should add a
> preference so as to change the value. Is the port being hardcodded a well
> known port for this protocol? If not, we might default it to 0 instead.
>
> Regards,
> Pascal.
Pascal,

Thanks for the feedback. I've converted to a builtin dissector.

I got stuck on adding the preference to handle the port number, I'll have to
take another stab at it tomorrow. The #defined port is the default for
Apache's Cassandra. I don't know what else in the world might use that port
as well. If defaulting to 0 is a better route I'm happy to accommodate.

-Aaron

___________________________________________________________________________
Sent via:    Wireshark-dev mailing list <wireshark-dev () wireshark org>
Archives:    https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
             mailto:wireshark-dev-request () wireshark org?subject=unsubscribe