https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=5466
--- Comment #20 from Richard Sharpe <realrichardsharpe@xxxxxxxxx> 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.
