On Wed, 2018-11-28 at 17:28 +0100, David Disseldorp wrote:
> On Wed, 28 Nov 2018 08:08:30 -0800, Bart Van Assche wrote:
>
> > > Just a follow up here. I think it's safer to retain strncpy() in this
> > > function for the purpose of zero filling. scsi_dump_inquiry() and
> > > target_stat_lu_vend_show() walk the entire length of the t10_wwn.vendor
> > > buffer.
> >
> > How about adding a comment about above struct t10_wwn that vendor[], model[]
> > and revision[] in that structure may but do not have to be terminated with a
> > trailing '\0' and also that unit_serial[] is '\0'-terminated?
> >
> > It would make me happy if the size of the character arrays in that structure
> > would be increased by one and if the target code would be modified such that
> > these strings are always '\0'-terminated.
>
> I'm working on the +1 length increase for those t10_wwn members ATM, but
> just thought I'd mention that the zeroing of unused bytes is still
> required due to the scsi_dump_inquiry() + target_stat_lu_vend_show()
> behaviour.
Hi David,
Maybe I'm missing something, but why is zeroing of unused bytes in these
functions
necessary? Would the following be correct if all strings in struct t10_wwn
would be
'\0'-terminated?
static ssize_t target_stat_lu_vend_show(struct config_item *item, char *page)
{
struct se_device *dev = to_stat_lu_dev(item);
- int i;
- char str[sizeof(dev->t10_wwn.vendor)+1];
- /* scsiLuVendorId */
- for (i = 0; i < sizeof(dev->t10_wwn.vendor); i++)
- str[i] = ISPRINT(dev->t10_wwn.vendor[i]) ?
- dev->t10_wwn.vendor[i] : ' ';
- str[i] = '\0';
- return snprintf(page, PAGE_SIZE, "%s\n", str);
+ return snprintf(page, PAGE_SIZE, "%-16s\n", dev->t10_wwn.vendor);
}
[ ... ]
static void scsi_dump_inquiry(struct se_device *dev)
{
struct t10_wwn *wwn = &dev->t10_wwn;
- char buf[17];
- int i, device_type;
+ int device_type = dev->transport->get_device_type(dev);
+
/*
* Print Linux/SCSI style INQUIRY formatting to the kernel ring buffer
*/
- for (i = 0; i < 8; i++)
- if (wwn->vendor[i] >= 0x20)
- buf[i] = wwn->vendor[i];
- else
- buf[i] = ' ';
- buf[i] = '\0';
- pr_debug(" Vendor: %s\n", buf);
-
- for (i = 0; i < 16; i++)
- if (wwn->model[i] >= 0x20)
- buf[i] = wwn->model[i];
- else
- buf[i] = ' ';
- buf[i] = '\0';
- pr_debug(" Model: %s\n", buf);
-
- for (i = 0; i < 4; i++)
- if (wwn->revision[i] >= 0x20)
- buf[i] = wwn->revision[i];
- else
- buf[i] = ' ';
- buf[i] = '\0';
- pr_debug(" Revision: %s\n", buf);
-
- device_type = dev->transport->get_device_type(dev);
+ pr_debug(" Vendor: %-8s\n", wwn->vendor);
+ pr_debug(" Model: %-16s\n", wwn->model);
+ pr_debug(" Revision: %-4s\n", wwn->revision);
pr_debug(" Type: %s ", scsi_device_type(device_type));
}
Thanks,
Bart.
