Huge thanks to our Platinum Members Endace and LiveAction,
and our Silver Member Veeam, for supporting the Wireshark Foundation and project.

Wireshark-bugs: [Wireshark-bugs] [Bug 2935] New simulcrypt protocol dissector

Date: Tue, 7 Oct 2008 06:59:40 -0700 (PDT)
https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=2935





--- Comment #6 from David Castleford <david.castleford@xxxxxxxxxxxxxxxxxx>  2008-10-07 06:59:38 PDT ---
(In reply to comment #4)
> A few remarks after a quick review:
> 
> Replace // comment strings with /*  */. The double slash is C++ only.
OK

> Clean up development left overs as well please.
OK

> value_string arrays have to end in {0, NULL} tuple to cover non listed values.
OK, added for messagetypenames, for parametertypenames value 0 is covered by
SIMULCRYPT_DVB_RESERVED = 0x0000

> {name, abbrev, type, display, strings, bitmask, blurb, HFILL}
> if abbrev eq blurb then blurb can be NULL.
OK, used NULL
> 
> { &hf_simulcrypt_version,
> { "Version", "simulcrypt.version", FT_UINT8, BASE_HEX, NULL, 0x0,
> "Version Length", HFILL }},
> Is it version or version length ?
good catch, it's version, set to NULL now
> 
> FT_BOOLEAN fields have no BASE_, they have a fieldwidth.
OK, 1 byte so set to 8
> 
> +       tempType = tvb_get_guint8(tvb,1); // Get the type 2 bytes - MSB byte
> +       type+=tempType;
> +       type=type<<8; // shift 1 byte as MSB
> +       tempType = tvb_get_guint8(tvb,2); // Get the type 2 bytes - LSB byte
> +       type+=tempType; // add LSB to 2 byte type
> why not use
> +       type = tvb_get_ntohs(tvb, 1);
> This comes up numerous times.
thanks, have done so, much nicer code. 
> 
will reply to other comments, recompile and check things still work, create new
diff file and upload patch


-- 
Configure bugmail: https://bugs.wireshark.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.