| Message ID | 20210820032154.574953-1-damien.lemoal@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] scsi: simplify scsi_get_vpd_page() | expand |
On Fri, Aug 20, 2021 at 12:21:54PM +0900, Damien Le Moal wrote: > Remove unnecessary gotos in scsi_get_vpd_page() to improve the code > readability and use memchr() instead of an open coded search loop. > Also update the outdated kernel doc comment for this function. > > Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> > --- > Changes from v1: > * Fix memchr() use to avoid accesses beyond the buffer and result size. > > drivers/scsi/scsi.c | 31 ++++++++++++++----------------- > 1 file changed, 14 insertions(+), 17 deletions(-) > > diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c > index b241f9e3885c..6be68b3427a0 100644 > --- a/drivers/scsi/scsi.c > +++ b/drivers/scsi/scsi.c > @@ -339,47 +339,44 @@ static int scsi_vpd_inquiry(struct scsi_device *sdev, unsigned char *buffer, > * > * SCSI devices may optionally supply Vital Product Data. Each 'page' > * of VPD is defined in the appropriate SCSI document (eg SPC, SBC). > - * If the device supports this VPD page, this routine returns a pointer > - * to a buffer containing the data from that page. The caller is > - * responsible for calling kfree() on this pointer when it is no longer > - * needed. If we cannot retrieve the VPD page this routine returns %NULL. > + * If the device supports this VPD page, this routine fills @buf > + * with the data from that page and return 0. If the VPD page is not > + * supported or its content cannot be retrieved, -EINVAL is returned. > */ > int scsi_get_vpd_page(struct scsi_device *sdev, u8 page, unsigned char *buf, > int buf_len) > { > - int i, result; > + int result, len; > > if (sdev->skip_vpd_pages) > - goto fail; > + return -EINVAL; > > /* Ask for all the pages supported by this device */ > result = scsi_vpd_inquiry(sdev, buf, 0, buf_len); > if (result < 4) > - goto fail; > + return -EINVAL; > > /* If the user actually wanted this page, we can skip the rest */ > if (page == 0) > return 0; > > - for (i = 4; i < min(result, buf_len); i++) > - if (buf[i] == page) > - goto found; > + len = min(result, buf_len); > + if (len > 4 && memchr(&buf[4], page, len - 4)) > + goto found; > > - if (i < result && i >= buf_len) > - /* ran off the end of the buffer, give us benefit of doubt */ > + /* If we ran off the end of the buffer, give us benefit of doubt */ > + if (result > buf_len) > goto found; > + > /* The device claims it doesn't support the requested page */ > - goto fail; > + return -EINVAL; > > found: > result = scsi_vpd_inquiry(sdev, buf, page, buf_len); > if (result < 0) > - goto fail; > + return -EINVAL; > > return 0; > - > - fail: > - return -EINVAL; > } > EXPORT_SYMBOL_GPL(scsi_get_vpd_page); > (Nit: I think you forgot to bump the version. Subject for this is v2, which is the same as the previous "scsi: simplify scsi_get_vpd_page()" patch.) Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c index b241f9e3885c..6be68b3427a0 100644 --- a/drivers/scsi/scsi.c +++ b/drivers/scsi/scsi.c @@ -339,47 +339,44 @@ static int scsi_vpd_inquiry(struct scsi_device *sdev, unsigned char *buffer, * * SCSI devices may optionally supply Vital Product Data. Each 'page' * of VPD is defined in the appropriate SCSI document (eg SPC, SBC). - * If the device supports this VPD page, this routine returns a pointer - * to a buffer containing the data from that page. The caller is - * responsible for calling kfree() on this pointer when it is no longer - * needed. If we cannot retrieve the VPD page this routine returns %NULL. + * If the device supports this VPD page, this routine fills @buf + * with the data from that page and return 0. If the VPD page is not + * supported or its content cannot be retrieved, -EINVAL is returned. */ int scsi_get_vpd_page(struct scsi_device *sdev, u8 page, unsigned char *buf, int buf_len) { - int i, result; + int result, len; if (sdev->skip_vpd_pages) - goto fail; + return -EINVAL; /* Ask for all the pages supported by this device */ result = scsi_vpd_inquiry(sdev, buf, 0, buf_len); if (result < 4) - goto fail; + return -EINVAL; /* If the user actually wanted this page, we can skip the rest */ if (page == 0) return 0; - for (i = 4; i < min(result, buf_len); i++) - if (buf[i] == page) - goto found; + len = min(result, buf_len); + if (len > 4 && memchr(&buf[4], page, len - 4)) + goto found; - if (i < result && i >= buf_len) - /* ran off the end of the buffer, give us benefit of doubt */ + /* If we ran off the end of the buffer, give us benefit of doubt */ + if (result > buf_len) goto found; + /* The device claims it doesn't support the requested page */ - goto fail; + return -EINVAL; found: result = scsi_vpd_inquiry(sdev, buf, page, buf_len); if (result < 0) - goto fail; + return -EINVAL; return 0; - - fail: - return -EINVAL; } EXPORT_SYMBOL_GPL(scsi_get_vpd_page);
Remove unnecessary gotos in scsi_get_vpd_page() to improve the code readability and use memchr() instead of an open coded search loop. Also update the outdated kernel doc comment for this function. Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> --- Changes from v1: * Fix memchr() use to avoid accesses beyond the buffer and result size. drivers/scsi/scsi.c | 31 ++++++++++++++----------------- 1 file changed, 14 insertions(+), 17 deletions(-)