* [PATCH][next] scsi: scsi_debug: remove a redundant assignment to variable ret @ 2024-10-02 13:50 Colin Ian King 2024-10-02 15:10 ` Dan Carpenter 0 siblings, 1 reply; 6+ messages in thread From: Colin Ian King @ 2024-10-02 13:50 UTC (permalink / raw) To: James E . J . Bottomley, Martin K . Petersen, linux-scsi Cc: kernel-janitors, linux-kernel 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, 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; -- 2.39.5 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH][next] scsi: scsi_debug: remove a redundant assignment to variable ret 2024-10-02 13:50 [PATCH][next] scsi: scsi_debug: remove a redundant assignment to variable ret Colin Ian King @ 2024-10-02 15:10 ` Dan Carpenter 2024-10-02 15:19 ` Dan Carpenter 2024-10-16 7:16 ` John Garry 0 siblings, 2 replies; 6+ messages in thread From: Dan Carpenter @ 2024-10-02 15:10 UTC (permalink / raw) To: Colin Ian King Cc: James E . J . Bottomley, Martin K . Petersen, linux-scsi, kernel-janitors, linux-kernel 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][next] scsi: scsi_debug: remove a redundant assignment to variable ret 2024-10-02 15:10 ` Dan Carpenter @ 2024-10-02 15:19 ` Dan Carpenter 2024-10-16 7:16 ` John Garry 1 sibling, 0 replies; 6+ messages in thread From: Dan Carpenter @ 2024-10-02 15:19 UTC (permalink / raw) To: Colin Ian King Cc: James E . J . Bottomley, Martin K . Petersen, linux-scsi, kernel-janitors, linux-kernel 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][next] scsi: scsi_debug: remove a redundant assignment to variable ret 2024-10-02 15:10 ` Dan Carpenter 2024-10-02 15:19 ` Dan Carpenter @ 2024-10-16 7:16 ` John Garry 2024-10-17 7:59 ` John Garry 2024-10-17 8:57 ` Dan Carpenter 1 sibling, 2 replies; 6+ messages in thread From: John Garry @ 2024-10-16 7:16 UTC (permalink / raw) To: Dan Carpenter, Colin Ian King Cc: James E . J . Bottomley, Martin K . Petersen, linux-scsi, kernel-janitors, linux-kernel 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. ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH][next] scsi: scsi_debug: remove a redundant assignment to variable ret 2024-10-16 7:16 ` John Garry @ 2024-10-17 7:59 ` John Garry 2024-10-17 8:57 ` Dan Carpenter 1 sibling, 0 replies; 6+ messages in thread From: John Garry @ 2024-10-17 7:59 UTC (permalink / raw) To: Dan Carpenter, Colin Ian King Cc: James E . J . Bottomley, Martin K . Petersen, linux-scsi, kernel-janitors, linux-kernel 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! ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][next] scsi: scsi_debug: remove a redundant assignment to variable ret 2024-10-16 7:16 ` John Garry 2024-10-17 7:59 ` John Garry @ 2024-10-17 8:57 ` Dan Carpenter 1 sibling, 0 replies; 6+ messages in thread From: Dan Carpenter @ 2024-10-17 8:57 UTC (permalink / raw) To: John Garry Cc: Colin Ian King, James E . J . Bottomley, Martin K . Petersen, linux-scsi, kernel-janitors, linux-kernel 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-10-17 8:57 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-10-02 13:50 [PATCH][next] scsi: scsi_debug: remove a redundant assignment to variable ret Colin Ian King 2024-10-02 15:10 ` Dan Carpenter 2024-10-02 15:19 ` Dan Carpenter 2024-10-16 7:16 ` John Garry 2024-10-17 7:59 ` John Garry 2024-10-17 8:57 ` Dan Carpenter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox