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.mayn...@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-boun...@wireshark.org
> [mailto:wireshark-dev-boun...@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.sp...@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-boun...@wireshark.org
> > [mailto:wireshark-dev-boun...@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-requ...@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-requ...@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-requ...@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-requ...@wireshark.org?subject=unsubscribe
_________________________________________________________________
Discutez en direct avec vos amis sur Messenger !
http://www.windowslive.fr/messenger___________________________________________________________________________
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-requ...@wireshark.org?subject=unsubscribe
