Wireshark mailing list archives
bug 5175 PPI-GEOLOCATION patch, input pcap
From: johnny cache <johnycsh () gmail com>
Date: Mon, 8 Nov 2010 15:15:42 -0600
Hi Everyone, Just thought I would point out that I uploaded a patch with 3 new dissectors. Steven had reviewed my first pass at the GPS dissector previously. I have addressed his comments inline below. I hope everything is in order. Please let me know if I need to make any changes for it to be merged. Thanks, -Jon E The following line from packet-ppi-gps.c doesn't need the (guint32) cast since
tvb_get_letohl() already returns a guint32 (despite its historical use
of the
"long"): fixtype_flags = (guint32) tvb_get_letohl(tvb, offset);
Fixed.
Is there a reason g_appstr_num is fetched using tvb_get_letohl() and
then added
to the tree with proto_tree_add_uint() instead of adding it directly with proto_tree_add_item()? That would make for cleaner code as it appears that g_appstr_num isn't used after the proto_tree_add_uint() call. using _add_item() makes the code easier to read if the variable isn't needed
after
that. There may be other cases of this; this is just an example I noticed.
\Point made, however I would like to keep a copy of the values locally for future improvements (maybe making a cleaner text representation with the appropriate degrees of precision displayed). Also, if I were to skip the tvb_get_letohl() call, how do I make sure values are byte-swapped on big-endian systems?
Don't use g_ as a prefix on things as that is used by GLib and could cause confusion.
Fixed.
Wireshark code typically formats the braces a little differently: if(condition) { code; }
I ran the code through a prettifier before generating the patch. It has these style brackets now. If there are any particular indent/astyle/whatever formatter rules you'd like me to run it through that is fine with me. I just ran astyle --brackets=attach. I would still like to pursue breaking PPI stuff out with a dissector table. However I wasn't able to get to it yet, and I'd like to minimize the amount of changes to existing code I make while committing my own dissectors. I may get some more PPI tags shortly, that would be a good opportunity for me to make the change. Also, I re-ran the fuzzer (with all dissectors loaded and with a input that triggered every field to begin with) No events after 7k passes. ---- Starting pass 7176: /home/johnycsh/ppi_sdk/out.pcap: OK Starting pass 7177: /home/johnycsh/ppi_sdk/out.pcap: ^C ---- ___________________________________________________________________________ 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:
- bug 5175 PPI-GEOLOCATION patch, input pcap johnny cache (Nov 08)
- <Possible follow-ups>
- bug 5175 PPI-GEOLOCATION patch, input pcap Jon Ellch (Nov 11)
- Re: bug 5175 PPI-GEOLOCATION patch, input pcap Guy Harris (Nov 11)