| Message ID | 20210812074346.1048470-1-damien.lemoal@wdc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | scsi: cleanup scsi_get_vpd_page() | expand |
On 8/12/21 12:43 AM, Damien Le Moal wrote: > Get rid of all the gotos in scsi_get_vpd_page() to improve the code > readability and make it more compact. Also update the outdated kernel > doc comment for that function. > > Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> > --- > drivers/scsi/scsi.c | 37 ++++++++++++++++++------------------- > 1 file changed, 18 insertions(+), 19 deletions(-) > > diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c > index b241f9e3885c..14f7bb5e16cc 100644 > --- a/drivers/scsi/scsi.c > +++ b/drivers/scsi/scsi.c > @@ -339,47 +339,46 @@ 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) > { > + bool found = false; > int i, result; > > 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; The above conversions look fine to me. > /* 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; > + for (i = 4; i < min(result, buf_len); i++) { > + if (buf[i] == page) { > + found = true; > + break; > + } > + } The above change goes against the kernel coding style and in my opinion makes this function harder to read. Please keep the goto. Additionally, it is not clear to me why memchr() has been open-coded in this function? Thanks, Bart.
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c index b241f9e3885c..14f7bb5e16cc 100644 --- a/drivers/scsi/scsi.c +++ b/drivers/scsi/scsi.c @@ -339,47 +339,46 @@ 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) { + bool found = false; int i, result; 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; + for (i = 4; i < min(result, buf_len); i++) { + if (buf[i] == page) { + found = true; + break; + } + } - if (i < result && i >= buf_len) - /* ran off the end of the buffer, give us benefit of doubt */ - goto found; - /* The device claims it doesn't support the requested page */ - goto fail; + /* If we ran off the end of the buffer, give us benefit of doubt */ + found = found || (i < result && i >= buf_len); + if (!found) + /* The device claims it doesn't support the requested page */ + 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);
Get rid of all the gotos in scsi_get_vpd_page() to improve the code readability and make it more compact. Also update the outdated kernel doc comment for that function. Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> --- drivers/scsi/scsi.c | 37 ++++++++++++++++++------------------- 1 file changed, 18 insertions(+), 19 deletions(-)