On 09/29/2015 03:47 AM, Hannes Reinecke wrote:
+static const struct {
+ enum scsi_access_state value;
+ char *name;
Had you considered to use "const char *" here instead of "char *" ?
+const char *scsi_access_state_name(enum scsi_access_state state)
+{
+ int i;
+ char *name = NULL;
+
+ for (i = 0; i < ARRAY_SIZE(sdev_access_states); i++) {
+ if (sdev_access_states[i].value == state) {
+ name = sdev_access_states[i].name;
+ break;
+ }
+ }
+ return name;
+}
The return value of this function is passed without further processing
to the format specifier "%s". How about initializing "name" to "(?)" to
avoid that "(null)" is printed if the access state is not recognized ?
+static ssize_t
+sdev_show_access_state(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct scsi_device *sdev = to_scsi_device(dev);
+ enum scsi_access_state access_state;
+ bool pref = false;
+
+ if (sdev->access_state & SCSI_ACCESS_STATE_PREFERRED)
+ pref = true;
+
+ access_state = (sdev->access_state & SCSI_ACCESS_STATE_MASK);
+
+ return snprintf(buf, 32, "%s%s\n",
+ scsi_access_state_name(access_state),
+ pref ? " preferred" :"");
+}
Shouldn't this function check whether a device handler has been
associated with sdev and also whether the active device handler is the
ALUA device handler ? Otherwise incorrect data will be reported before
the ALUA device handler has been loaded, after it has been unloaded or
if another device handler is active.
Additionally, please insert a separator between the preferred state and
the access state name. Had you considered to use PAGE_SIZE instead of
"32" as output buffer length ? Magic constants in code always make the
reader wonder where these come from ...
How about reporting the target port group ID next to the ALUA state ? I
think that data would be very useful because it allows users to verify
which LUNs have been associated with which port group by this patch series.
Thanks,
Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
