Wireshark mailing list archives
Re: preliminary code submission
From: Gerasimos Dimitriadis <gedimitr () gmail com>
Date: Thu, 4 Feb 2010 11:39:56 +0200
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
Current thread:
- preliminary code submission Brian Oleksa (Feb 03)
- Re: preliminary code submission Jakub Zawadzki (Feb 03)
- Re: preliminary code submission Brian Oleksa (Feb 03)
- Re: preliminary code submission Jakub Zawadzki (Feb 03)
- Re: preliminary code submission Brian Oleksa (Feb 03)
- Re: preliminary code submission Speck Michael EHWG AVL/GAE (Feb 03)
- Re: preliminary code submission Gerasimos Dimitriadis (Feb 04)
- Re: preliminary code submission Maynard, Chris (Feb 04)
- Re: preliminary code submission philippe alarcon (Feb 04)
- Re: preliminary code submission Maynard, Chris (Feb 04)
- Re: preliminary code submission Jaap Keuter (Feb 04)
- Re: preliminary code submission Brian Oleksa (Feb 03)
- Re: preliminary code submission Maynard, Chris (Feb 05)
- Re: preliminary code submission Brian Oleksa (Feb 10)
- Re: preliminary code submission ronnie sahlberg (Feb 10)
- Re: preliminary code submission Maynard, Chris (Feb 12)
- Re: preliminary code submission Jakub Zawadzki (Feb 03)