Wireshark-bugs: [Wireshark-bugs] [Bug 5466] Improve dissection of bit-oriented fields, text fiel
Date: Thu, 9 Dec 2010 11:37:41 -0800 (PST)
https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=5466

--- Comment #18 from Chris Maynard <[email protected]> 2010-12-09 11:37:34 PST ---
(In reply to comment #16)
> This patch dissects the parameters associated with a PERSISTENT RESERVE OUT
> commands.
Some feedback:
1) For the true_false_string's, rather than naming the field with an obscure
name and then providing more informative text within the true_false_string[],
might it be better/simpler to just replace the obscure name with the more
meaningful text right in the hf_* description and then either use the default
true_false string or one of the built-in ones from epan/tfs.h for that field?

For example, you could change this:

static const true_false_string scsi_spec_i_pt_tfs = {
    "Specify Initiator Ports is set",
    "Specify Initiator Ports is not set"
};
    { &hf_scsi_persresv_control_spec_i_pt,
      {"SPEC_I_PT", "scsi.persresv.control.spec_i_pt", FT_BOOLEAN, 8,
      TFS(&scsi_spec_i_pt_tfs), 0x08, NULL, HFILL}},

to this:
    { &hf_scsi_persresv_control_spec_i_pt,
      {"Specify Initiator Ports", "scsi.persresv.control.spec_i_pt",
FT_BOOLEAN, 8,
      TFS(&tfs_set_notset), 0x08, NULL, HFILL}},

2) Shouldn't hf_scsi_persresv_control_rsvd2 be an "FT_BOOLEAN, 8" rather than
an "FT_UINT8, BASE_HEX", since the field is a single bit according to the 0x02
mask?  If so, then this probably applies to the last patch as well with respect
to hf_scsi_control_obs1, hf_scsi_control_obs2, hf_scsi_inq_control_obs1, and
hf_scsi_inq_control_obs2.

3) Whitespace: From section 1.1.5 of README.developer:  When editing an
existing file, try following the existing indentation logic ...

Most of packet-scsi.c is indented using spaces, but your edits use tabs, which
will make it harder for code to lineup and look right with all editors.  I
generally look at the current indentation (and bracketing) scheme used
surrounding the code that I'm adding editing/adding and simply mimic what's
already there.  Unfortunately, many files do have a mix of tabs/spaces, but
it's better not to introduce more inconsistencies if we can avoid it.

4) For a future patch perhaps, not sure if you've seen the TODO's at the top of
the file?

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