Re: preliminary code submission

[Gerasimos Dimitriadis <gedimitr () gmail com>]

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