| Message ID | 20210421174749.11221-3-hare@suse.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | SCSI result cleanup, part 2 | expand |
On 4/21/21 10:47 AM, Hannes Reinecke wrote: > The callers of sg_scsi_ioctl() already check for negative > return values, so we can drop the usage of DRIVER_ERROR > and return the error from blk_rq_map_kern() instead. > > Signed-off-by: Hannes Reinecke <hare@suse.de> > --- > block/scsi_ioctl.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c > index 6599bac0a78c..99d58786e0d5 100644 > --- a/block/scsi_ioctl.c > +++ b/block/scsi_ioctl.c > @@ -488,9 +488,10 @@ int sg_scsi_ioctl(struct request_queue *q, struct gendisk *disk, fmode_t mode, > break; > } > > - if (bytes && blk_rq_map_kern(q, rq, buffer, bytes, GFP_NOIO)) { > - err = DRIVER_ERROR << 24; > - goto error; > + if (bytes) { > + err = blk_rq_map_kern(q, rq, buffer, bytes, GFP_NOIO); > + if (err) > + goto error; > } > > blk_execute_rq(disk, rq, 0); Is this perhaps a backwards-incompatible user space ABI change since the sg_scsi_ioctl() return value is reported to user space? Thanks, Bart.
On 4/21/21 11:01 PM, Bart Van Assche wrote: > On 4/21/21 10:47 AM, Hannes Reinecke wrote: >> The callers of sg_scsi_ioctl() already check for negative >> return values, so we can drop the usage of DRIVER_ERROR >> and return the error from blk_rq_map_kern() instead. >> >> Signed-off-by: Hannes Reinecke <hare@suse.de> >> --- >> block/scsi_ioctl.c | 7 ++++--- >> 1 file changed, 4 insertions(+), 3 deletions(-) >> >> diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c >> index 6599bac0a78c..99d58786e0d5 100644 >> --- a/block/scsi_ioctl.c >> +++ b/block/scsi_ioctl.c >> @@ -488,9 +488,10 @@ int sg_scsi_ioctl(struct request_queue *q, struct >> gendisk *disk, fmode_t mode, >> break; >> } >> - if (bytes && blk_rq_map_kern(q, rq, buffer, bytes, GFP_NOIO)) { >> - err = DRIVER_ERROR << 24; >> - goto error; >> + if (bytes) { >> + err = blk_rq_map_kern(q, rq, buffer, bytes, GFP_NOIO); >> + if (err) >> + goto error; >> } >> blk_execute_rq(disk, rq, 0); > > Is this perhaps a backwards-incompatible user space ABI change since the > sg_scsi_ioctl() return value is reported to user space? > Possibly; but we're dealing with semantics here. All callers to sg_scsi_ioctl() _must_ be prepared to handle negative values already, so userspace should not break. We arguably _do_ return a different error here, but experience showed that basically no-one checks for driver byte values (except for DRIVER_SENSE, that is). So I don't think it really matters; we do return an error, and for userspace it doesn't matter as this command cannot be retried either way. So while this arguably is a change visible to userspace, I doubt it matters in the end. Cheers, Hannes
diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c index 6599bac0a78c..99d58786e0d5 100644 --- a/block/scsi_ioctl.c +++ b/block/scsi_ioctl.c @@ -488,9 +488,10 @@ int sg_scsi_ioctl(struct request_queue *q, struct gendisk *disk, fmode_t mode, break; } - if (bytes && blk_rq_map_kern(q, rq, buffer, bytes, GFP_NOIO)) { - err = DRIVER_ERROR << 24; - goto error; + if (bytes) { + err = blk_rq_map_kern(q, rq, buffer, bytes, GFP_NOIO); + if (err) + goto error; } blk_execute_rq(disk, rq, 0);
The callers of sg_scsi_ioctl() already check for negative return values, so we can drop the usage of DRIVER_ERROR and return the error from blk_rq_map_kern() instead. Signed-off-by: Hannes Reinecke <hare@suse.de> --- block/scsi_ioctl.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)