| Message ID | 20241002135043.942327-1-colin.i.king@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [next] scsi: scsi_debug: remove a redundant assignment to variable ret | expand |
On Wed, Oct 02, 2024 at 02:50:43PM +0100, Colin Ian King wrote: > The variable ret is being assigned a value that is never read, the > following break statement exits the loop where ret is being re-assigned > a new value. Remove the redundant assignment. > > Signed-off-by: Colin Ian King <colin.i.king@gmail.com> > --- > drivers/scsi/scsi_debug.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c > index d95f417e24c0..7c60f5acc4a3 100644 > --- a/drivers/scsi/scsi_debug.c > +++ b/drivers/scsi/scsi_debug.c > @@ -3686,14 +3686,12 @@ static int do_device_access(struct sdeb_store_info *sip, struct scsi_cmnd *scp, > sdeb_data_sector_lock(sip, do_write); > ret = sg_copy_buffer(sdb->table.sgl, sdb->table.nents, You would think there would be a: total += ret; here. > fsp + (block * sdebug_sector_size), > sdebug_sector_size, sg_skip, do_write); > sdeb_data_sector_unlock(sip, do_write); > - if (ret != sdebug_sector_size) { > - ret += (i * sdebug_sector_size); > + if (ret != sdebug_sector_size) > break; > - } > sg_skip += sdebug_sector_size; > if (++block >= sdebug_store_sectors) > block = 0; > } > ret = num * sdebug_sector_size; ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ And that this would be a "return total;" The comment at the start of the function says that it's supposed to return the actual number of bytes that were copied. And you can see how that was the intention. But what it actually does is it always reports that it copied the complete number of bytes. #Success #Woohoo I wouldn't feel comfortable changing it to report partial copies without testing it. Someone needs to look at it more carefully to figure out what the correct fix is. regards, dan carpenter
Generally, functions which report the number of bytes copied are an anti-pattern. bytes = copied(); if (bytes < 0) return bytes; <-- forgot to handle partial copies bytes = copied(); if (bytes < 0 || bytes != total) return bytes; <-- forgot the error code for partial copies bytes = copied(); if (bytes < sizeof()) <-- negative error codes type promoted to positive return -EIO; We've seen subsystems move away from this. Other subsystems are like "Ugh. Updating all the callers is a headache. Let's either report that everything copied or a negative error code, but not the partial bytes." Now the first two examples above magically work. regards, dan carpenter
On 02/10/2024 16:10, Dan Carpenter wrote: > On Wed, Oct 02, 2024 at 02:50:43PM +0100, Colin Ian King wrote: >> The variable ret is being assigned a value that is never read, the >> following break statement exits the loop where ret is being re-assigned >> a new value. Remove the redundant assignment. >> >> Signed-off-by: Colin Ian King<colin.i.king@gmail.com> >> --- >> drivers/scsi/scsi_debug.c | 4 +--- >> 1 file changed, 1 insertion(+), 3 deletions(-) >> >> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c >> index d95f417e24c0..7c60f5acc4a3 100644 >> --- a/drivers/scsi/scsi_debug.c >> +++ b/drivers/scsi/scsi_debug.c >> @@ -3686,14 +3686,12 @@ static int do_device_access(struct sdeb_store_info *sip, struct scsi_cmnd *scp, >> sdeb_data_sector_lock(sip, do_write); >> ret = sg_copy_buffer(sdb->table.sgl, sdb->table.nents, > You would think there would be a: > > total += ret; > > here. > >> fsp + (block * sdebug_sector_size), >> sdebug_sector_size, sg_skip, do_write);T >> sdeb_data_sector_unlock(sip, do_write); >> - if (ret != sdebug_sector_size) { >> - ret += (i * sdebug_sector_size); >> + if (ret != sdebug_sector_size) >> break; >> - } >> sg_skip += sdebug_sector_size; >> if (++block >= sdebug_store_sectors) >> block = 0; >> } >> ret = num * sdebug_sector_size; > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > And that this would be a "return total;" Right, the function is currently a little messy as there is no variable for "total", and we re-assign ret per loop. So I think that we can either: a. introduce a variable to hold "total" b. this change: diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c index af5e3a7f47a9..39218ffc6a31 100644 --- a/drivers/scsi/scsi_debug.c +++ b/drivers/scsi/scsi_debug.c @@ -3690,13 +3690,14 @@ static int do_device_access(struct sdeb_store_info *sip, struct scsi_cmnd *scp, sdeb_data_sector_unlock(sip, do_write); if (ret != sdebug_sector_size) { ret += (i * sdebug_sector_size); - break; + goto out_unlock; } sg_skip += sdebug_sector_size; if (++block >= sdebug_store_sectors) block = 0; } ret = num * sdebug_sector_size; +out_unlock: sdeb_data_unlock(sip, atomic); Maybe a. is better, as b. is maintaining some messiness. Thanks, John > > The comment at the start of the function says that it's supposed to return the > actual number of bytes that were copied. And you can see how that was the > intention. > > But what it actually does is it always reports that it copied the complete > number of bytes. #Success #Woohoo > > I wouldn't feel comfortable changing it to report partial copies without testing > it. Someone needs to look at it more carefully to figure out what the correct > fix is.
On 16/10/2024 08:16, John Garry wrote: >>> scsi_debug.c >>> @@ -3686,14 +3686,12 @@ static int do_device_access(struct >>> sdeb_store_info *sip, struct scsi_cmnd *scp, >>> sdeb_data_sector_lock(sip, do_write); >>> ret = sg_copy_buffer(sdb->table.sgl, sdb->table.nents, >> You would think there would be a: >> >> total += ret; >> >> here. >> >>> fsp + (block * sdebug_sector_size), >>> sdebug_sector_size, sg_skip, do_write);T >>> sdeb_data_sector_unlock(sip, do_write); >>> - if (ret != sdebug_sector_size) { >>> - ret += (i * sdebug_sector_size); >>> + if (ret != sdebug_sector_size) >>> break; >>> - } >>> sg_skip += sdebug_sector_size; >>> if (++block >= sdebug_store_sectors) >>> block = 0; >>> } >>> ret = num * sdebug_sector_size; >> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ >> And that this would be a "return total;" > > Right, the function is currently a little messy as there is no variable > for "total", and we re-assign ret per loop. > > So I think that we can either: > a. introduce a variable to hold "total" > b. this change: > > diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c > index af5e3a7f47a9..39218ffc6a31 100644 > --- a/drivers/scsi/scsi_debug.c > +++ b/drivers/scsi/scsi_debug.c > @@ -3690,13 +3690,14 @@ static int do_device_access(struct > sdeb_store_info *sip, struct scsi_cmnd *scp, > sdeb_data_sector_unlock(sip, do_write); > if (ret != sdebug_sector_size) { > ret += (i * sdebug_sector_size); > - break; > + goto out_unlock; > } > sg_skip += sdebug_sector_size; > if (++block >= sdebug_store_sectors) > block = 0; > } > ret = num * sdebug_sector_size; > +out_unlock: > sdeb_data_unlock(sip, atomic); > > > Maybe a. is better, as b. is maintaining some messiness. BTW, let me know if you are happy for me to send a patch to fix this. Thanks!
On Wed, Oct 16, 2024 at 08:16:16AM +0100, John Garry wrote: > On 02/10/2024 16:10, Dan Carpenter wrote: > > On Wed, Oct 02, 2024 at 02:50:43PM +0100, Colin Ian King wrote: > > > The variable ret is being assigned a value that is never read, the > > > following break statement exits the loop where ret is being re-assigned > > > a new value. Remove the redundant assignment. > > > > > > Signed-off-by: Colin Ian King<colin.i.king@gmail.com> > > > --- > > > drivers/scsi/scsi_debug.c | 4 +--- > > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > > > diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c > > > index d95f417e24c0..7c60f5acc4a3 100644 > > > --- a/drivers/scsi/scsi_debug.c > > > +++ b/drivers/scsi/scsi_debug.c > > > @@ -3686,14 +3686,12 @@ static int do_device_access(struct sdeb_store_info *sip, struct scsi_cmnd *scp, > > > sdeb_data_sector_lock(sip, do_write); > > > ret = sg_copy_buffer(sdb->table.sgl, sdb->table.nents, > > You would think there would be a: > > > > total += ret; > > > > here. > > > > > fsp + (block * sdebug_sector_size), > > > sdebug_sector_size, sg_skip, do_write);T > > > sdeb_data_sector_unlock(sip, do_write); > > > - if (ret != sdebug_sector_size) { > > > - ret += (i * sdebug_sector_size); > > > + if (ret != sdebug_sector_size) > > > break; > > > - } > > > sg_skip += sdebug_sector_size; > > > if (++block >= sdebug_store_sectors) > > > block = 0; > > > } > > > ret = num * sdebug_sector_size; > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > And that this would be a "return total;" > > Right, the function is currently a little messy as there is no variable for > "total", and we re-assign ret per loop. > > So I think that we can either: > a. introduce a variable to hold "total" > b. this change: > > diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c > index af5e3a7f47a9..39218ffc6a31 100644 > --- a/drivers/scsi/scsi_debug.c > +++ b/drivers/scsi/scsi_debug.c > @@ -3690,13 +3690,14 @@ static int do_device_access(struct > sdeb_store_info *sip, struct scsi_cmnd *scp, > sdeb_data_sector_unlock(sip, do_write); > if (ret != sdebug_sector_size) { > ret += (i * sdebug_sector_size); > - break; > + goto out_unlock; > } > sg_skip += sdebug_sector_size; > if (++block >= sdebug_store_sectors) > block = 0; > } > ret = num * sdebug_sector_size; > +out_unlock: > sdeb_data_unlock(sip, atomic); > > > Maybe a. is better, as b. is maintaining some messiness. > I'm happy with option a. regards, dan carpenter
diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c index d95f417e24c0..7c60f5acc4a3 100644 --- a/drivers/scsi/scsi_debug.c +++ b/drivers/scsi/scsi_debug.c @@ -3686,14 +3686,12 @@ static int do_device_access(struct sdeb_store_info *sip, struct scsi_cmnd *scp, sdeb_data_sector_lock(sip, do_write); ret = sg_copy_buffer(sdb->table.sgl, sdb->table.nents, fsp + (block * sdebug_sector_size), sdebug_sector_size, sg_skip, do_write); sdeb_data_sector_unlock(sip, do_write); - if (ret != sdebug_sector_size) { - ret += (i * sdebug_sector_size); + if (ret != sdebug_sector_size) break; - } sg_skip += sdebug_sector_size; if (++block >= sdebug_store_sectors) block = 0; } ret = num * sdebug_sector_size;
The variable ret is being assigned a value that is never read, the following break statement exits the loop where ret is being re-assigned a new value. Remove the redundant assignment. Signed-off-by: Colin Ian King <colin.i.king@gmail.com> --- drivers/scsi/scsi_debug.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)