Re: preliminary code submission

["Maynard, Chris" <Christopher.Maynard () GTECH COM>]

I am not a core developer, just a contributor like you, but since I'm on this thread, I'll add my 2 cents ...

 

I don't think there really are any hard requirements per se.  But, once a dissector is accepted, the core development 
team is now somewhat responsible for maintaining it (patches can always come from anyone but the core developers are 
the ones who ultimately have to analyze the patches and check them in, etc.).

 

So, I think this is a judgment call that each core developer makes.  As such, the "requirements" are not well-defined 
and never will be whenever there's subjectivity involved.  If I were to decide upon accepting it or not, I would ask 
myself if the proposed dissector is in such a state that if someone had to go in there and modify it, etc., would they 
be able to reasonably understand it and easily maintain it later on down the road?  Once I felt it was in good enough 
shape that I could answer "yes" to that question, I would probably accept it.  I don't know about everyone else, but my 
time is extremely limited and I wouldn't want to spend an inordinate amount of it trying to modify some obscure 
protocol that I never heard of before and will never use in my lifetime, so the dissector had better be in darn good 
shape and very easy to understand before I would accept it.  As I said, I'm not a core developer, so I play no role in 
making any decisions on whether a dissector is accepted or not.

 

OK, maybe that was more like a nickel.

- Chris

 

 

From: wireshark-dev-bounces () wireshark org [mailto:wireshark-dev-bounces () wireshark org] On Behalf Of philippe 
alarcon
Sent: Thursday, February 04, 2010 11:02 AM
To: wireshark-dev
Subject: Re: [Wireshark-dev] preliminary code submission

 

Hello,

Related to the question Chris has raised here below
regarding the new protocol, I have a more general
question :

- what are the criteria (or the requirements) to add
a new dissector in main Wireshark distribution ?
(independantly of the compliance of the code to the
development rules)

Regards
Philippe


> Date: Thu, 4 Feb 2010 10:37:14 -0500
> From: Christopher.Maynard () GTECH COM
> To: wireshark-dev () wireshark org
> Subject: Re: [Wireshark-dev] preliminary code submission
>
> Some other observations/feedback:
>
> You should look again at the doc/README.developer skeleton code and follow the common structure there. For example, 
> you need to add a GPL statement, you should move the handoff and register routines to the bottom, etc.
>
> Run tools/check*.pl on your dissector.
> Fuzztest your dissector if you haven't already done so.
>
> I would recommend breaking up your for() loop into smaller functions and add some handling for an unknown code. For 
> example:
> for (;;) {
> ...
> if (code == 0)
> break;
> else if (code == 1)
> offset += dissect_gps(...);
> else if (code == 2)
> offset += dissect_flow(...);
> else if (code == 3)
> offset += dissect_host(...);
> else
> offset += dissect_unknown(...);
> ...
> }
>
> Or replace the if()/else if()/else structure with a switch() and use an appropriate condition to exit your for() 
> loop. Or you could use a vector table, i.e., 
>
> #define MAX_CODES 4
> static guint32 (*dissect_code[MAX_CODES])(...) = {
> dissect_null, dissect_gps, dissect_flow, dissect_host
> };
> if (code < MAX_CODES)
> offset += dissect_code[code](...);
> else
> offset += dissect_unknown(...);
>
> Port 7636 is not registered with IANA. http://www.iana.org/assignments/port-numbers 
> Is that a port you picked? You should either register it or add a preference to make the port configurable or choose 
> a port past the registered port range. 
>
> Is this protocol documented anywhere? You should reference the RFC(s) or whatever it is that describes it.
>
> - Chris
>
> -----Original Message-----
> From: wireshark-dev-bounces () wireshark org [mailto:wireshark-dev-bounces () wireshark org] On Behalf Of Gerasimos 
> Dimitriadis
> Sent: Thursday, February 04, 2010 4:40 AM
> To: Developer support list for Wireshark
> Subject: Re: [Wireshark-dev] preliminary code submission
>
> Hi Brian,
>
> there is no need to add a text string as blurb, if it is the same as
> the title of the field, so for example the ipv4 one would become:
> { &hf_helen_ipv4,
> { "IPv4", "helen.ipv4address", FT_IPv4, BASE_NONE, NULL, 0x0,
> NULL, HFILL}},
>
> Also, continuing from Michael's comments there is no need to read the
> value of status and use proto_tree_add_uint, since you are not using
> status later on. I think that it would be best to change it to:
>
> void dissect_helen(tvbuff_t *tvb, packet_info *pinfo, proto_tree *tree)
> {
> [...]
> /* Status: */
> if ((fieldsAvail & 1) != 0) {
> proto_tree_add_item(helen_sub_tree, hf_helen_gpsstatus,
> tvb, offset, 1, FALSE);
> offset += 1;
> }
> [...]
> }
>
> Also at dissect_helen, I propose to deal with the packet code in the
> same way as the gpsstatus. Add an entry for it in the hf[] array,
> define an array of value_string with the code names (i.e. End of
> Packet, GPS Extension...) and use proto_tree_add_item to add it to the
> tree. Apart from all other advantages, you don't need to explicitly
> deal with the unknown code strings.
>
> Best regards,
>
> Gerasimos
>
> 2010/2/4 Speck Michael EHWG AVL/GAE <Michael.Speck () avl com>:
> > Hi Brian,
> >
> > just had a look on your latest preview.
> > There is one more thing you can do to reduce the code size even further:
> > as Jakub has already suggested earlier, it is a good idea to use
> > value_string struct to map values with strings. In your case it is
> > especially true for the GPS status.
> >
> > There are only a few changes necessary to your code. First define a
> > value_string typed constant, then slightly change the status field
> > definition in function proto_register_helen() and finally shorten the
> > status field dissection in function dissect_helen(). Below is shown how
> > these code sections could look like afterwards.
> >
> >
> > static const value_string helen_gps_status[] = {
> >        { 0, "Good" },
> >        { 1, "No Fix" },
> >        { 2, "Bad GPS Read" },
> >        { 0, NULL }
> > };
> >
> >
> > void proto_register_helen(void) {
> > [...]
> >        { &hf_helen_gpsstatus,
> >                { "GPS Status", "helen.gpsStatus", FT_UINT8, BASE_DEC,
> > VALS(helen_gps_status), 0x00,
> >                "GPS Status", HFILL}},
> > [...]
> > }
> >
> >
> > void dissect_helen(tvbuff_t *tvb, packet_info *pinfo, proto_tree *tree)
> > {
> >        [...]
> >        /* Status: */
> >        if ((fieldsAvail & 1) != 0) {
> >                guint8 status;
> >                status = tvb_get_guint8(tvb,offset);
> >                proto_tree_add_uint(helen_sub_tree, hf_helen_gpsstatus,
> > tvb, offset, 1, status);
> >                offset += 1;
> >        }
> >        [...]
> > }
> >
> >
> >
> > Cheers
> > Mike
> >
> >
> >
> >
> > -----Original Message-----
> > From: wireshark-dev-bounces () wireshark org
> > [mailto:wireshark-dev-bounces () wireshark org] On Behalf Of Brian Oleksa
> > Sent: Mittwoch, 3. Februar 2010 21:21
> > To: Developer support list for Wireshark
> > Subject: Re: [Wireshark-dev] preliminary code submission
> >
> > Jakub
> >
> > Yes...you are right. I am not using alot of those variables. I was
> > mislead by what I was doing before I started to use the built in
> > routines. :-)
> >
> > My code base keeps getting smaller and smaller. I do understand that
> > code quality / readability is a key factor in this industry. I still
> > need to fix my IDE to get the correct formatting.
> >
> > Perhaps once last quick look..?? Attached is the updated file.
> >
> > Thanks again for the great feedback..!!
> >
> > Brian
> >
> >
> >
> >
> >
> > Jakub Zawadzki wrote:
> > > Hi,
> > >
> > > On Wed, Feb 03, 2010 at 01:05:32PM -0500, Brian Oleksa wrote:
> > >
> > > > Jakub
> > > >
> > > > Thanks for this feedback. It is always good to have an extra set of
> > > > eyes :-)
> > >
> > > You missunderstood my comment about check_col()
> > >
> > > instead of:
> > >     if (check_col(pinfo->cinfo, COL_PROTOCOL))
> > >         col_set_str(pinfo->cinfo, COL_PROTOCOL, PROTO_TAG_HELEN);
> >
> > >     if (check_col(pinfo->cinfo, COL_INFO))
> > >         col_clear(pinfo->cinfo, COL_INFO);
> > >
> > > Just write:
> > >       col_set_str(pinfo->cinfo, COL_PROTOCOL, PROTO_TAG_HELEN);
> >
> > >         col_clear(pinfo->cinfo, COL_INFO);
> > >
> > >
> > > > gfloat latitude;
> > > > latitude = tvb_get_ntohieee_float(tvb,offset);
> > > >
> > > > Why do you think this variable is not being used..??
> > >
> > > Well because it's not used :) What is used for?
> > >
> > > And one more thing...
> > >
> > > You declare: ett_helen_ipv6, ett_helen_nos, ...
> > > I believe you need only ett_helen from ett_*
> > >
> > > Cheers.
> > ________________________________________________________________________
> > ___
> > > Sent via:    Wireshark-dev mailing list <wireshark-dev () wireshark org>
> > > Archives:    http://www.wireshark.org/lists/wireshark-dev
> > > Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
> > >
> > > mailto:wireshark-dev-request () wireshark org?subject=unsubscribe
> > ___________________________________________________________________________
> > Sent via:    Wireshark-dev mailing list <wireshark-dev () wireshark org>
> > Archives:    http://www.wireshark.org/lists/wireshark-dev
> > Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
> >             mailto:wireshark-dev-request () wireshark org?subject=unsubscribe
> ___________________________________________________________________________
> Sent via: Wireshark-dev mailing list <wireshark-dev () wireshark org>
> Archives: http://www.wireshark.org/lists/wireshark-dev
> Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
> mailto:wireshark-dev-request () wireshark org?subject=unsubscribe
> CONFIDENTIALITY NOTICE: The contents of this email are confidential
> and for the exclusive use of the intended recipient. If you receive this
> email in error, please delete it from your system immediately and 
> notify us either by email, telephone or fax. You should not copy,
> forward, or otherwise disclose the content of the email.
>
> ___________________________________________________________________________
> Sent via: Wireshark-dev mailing list <wireshark-dev () wireshark org>
> Archives: http://www.wireshark.org/lists/wireshark-dev
> Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
> mailto:wireshark-dev-request () wireshark org?subject=unsubscribe

________________________________

Discute avec tes amis partout, grâce à Messenger sur ton mobile. Cliquez ici ! 
<http://www.messengersurvotremobile.com/> 

CONFIDENTIALITY NOTICE: The contents of this email are confidential
and for the exclusive use of the intended recipient. If you receive this
email in error, please delete it from your system immediately and 
notify us either by email, telephone or fax. You should not copy,
forward, or otherwise disclose the content of the email.

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