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@xxxxxxx>
---
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.
