https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=5466
--- Comment #18 from Chris Maynard <christopher.maynard@xxxxxxxxx> 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.