| Message ID | 20241002135043.942327-1-colin.i.king@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [next] scsi: scsi_debug: remove a redundant assignment to variable ret | expand |
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(-)