Wireshark · Wireshark-dev: Re: [Wireshark-dev] preliminary code submission

[philippe alarcon <philippe.alarcon@xxxxxxx>]

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@xxxxxxxxx
> To: wireshark-dev@xxxxxxxxxxxxx
> 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@xxxxxxxxxxxxx [mailto:wireshark-dev-bounces@xxxxxxxxxxxxx] 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@xxxxxxx>:
> > 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@xxxxxxxxxxxxx
> > [mailto:wireshark-dev-bounces@xxxxxxxxxxxxx] 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@xxxxxxxxxxxxx>
> >> Archives:    http://www.wireshark.org/lists/wireshark-dev
> >> Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
> >>
> >> mailto:wireshark-dev-request@xxxxxxxxxxxxx?subject=unsubscribe
> >>
> > ___________________________________________________________________________
> > 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
> >
> ___________________________________________________________________________
> 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
> 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@xxxxxxxxxxxxx>
> Archives: http://www.wireshark.org/lists/wireshark-dev
> Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
> mailto:wireshark-dev-request@xxxxxxxxxxxxx?subject=unsubscribe

---

Discute avec tes amis partout, grâce à Messenger sur ton mobile. Cliquez ici !