Wireshark-bugs: [Wireshark-bugs] [Bug 6428] BRP Dissector

Date: Mon, 3 Oct 2011 22:19:40 -0700 (PDT)
https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=6428

--- Comment #4 from Anders Broman <anders.broman@xxxxxxxxxxxx> 2011-10-03 22:19:38 PDT ---
Hi,
A quick review.
- Please make it a built in dissector.
- No C++ style comments(//)(see README.developer on compabillity)
- Follow the style of other dissectors by placing proto_reg_handoff() an
  proto_register() at the end of the file.
- +static const value_string packettypenames[] = {
  pregix with brp
  +static const value_string brp_packettype_names[] = {
- +static const value_string hf_brp_stat_vals[] = {
  do not prefix with hf
  +static const value_string brp_stat_vals[] = {
- #include <stdio.h> not needed.
- Declaring hf fields do not duplicate the field description
+        { &hf_brp_type,
+        { "Type", "brp.type", FT_UINT8, BASE_DEC, VALS(packettypenames), 0x0,
+        "Type", HFILL }},
Should be
+        { &hf_brp_type,
+        { "Type", "brp.type", FT_UINT8, BASE_DEC, VALS(packettypenames), 0x0,
+        NULL, HFILL }},
Run tools/checkhf.pl


- Do not use tvb_memcpy() use tvb_get_guint8()
- +            proto_tree_add_item( brp_tree, hf_brp_type, tvb, offset, 1,
FALSE ); offset += 1;
 I'd prefere that as two lines, FALSE should be ENC_BIG_ENDIAN as we are
chenging that.
- A reference to a protocol spec would be nice and an entry to the wiki
protocol pages.
- Using the standard boiler plate would be nice too
 * Wireshark - Network traffic analyzer
 * By Gerald Combs <gerald@xxxxxxxxxxxxx>
 * Copyright 1998 Gerald Combs
 *
 * This program is free software; you can redistribute it and/or
 * modify it under the terms of the GNU General Public License
 * as published by the Free Software Foundation; either version 2
 * of the License, or (at your option) any later version.
 *
 * This program is distributed in the hope that it will be useful,
 * but WITHOUT ANY WARRANTY; without even the implied warranty of
 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 * GNU General Public License for more details.
 *
 * You should have received a copy of the GNU General Public License
 * along with this program; if not, write to the Free Software
 * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA  02111-1307, USA.

Best regards
Anders

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