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

--- Comment #20 from Richard Sharpe <[email protected]> 2010-12-09 16:08:12 PST ---
(In reply to comment #18)
> (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}},

I did it the way I currently have the code because the specs show SPEC_I_PT for
the field in the CDB, but the meaning of that field is Specify Initiator Ports.
So, in order not to confuse people who are looking at traces I opted to show
the name of the field and the meaning of the field. It's not clear from
SPEC_I_PT what it actually means.

> 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.

Yeah, these are just errors. I will fix them.

> 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.

I unfortunately work both on Linux kernel code, where tabs are to be used and,
for the moment, Wireshark (after not having worked on Wireshark for years). I
will try to get into the habit of setting my .vimrc to convert tabs to 8
spaces.

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

Thanks :-)

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