| Message ID | 20210420000845.25873-76-bvanassche@acm.org (mailing list archive) |
|---|---|
| State | Deferred |
| Headers | show |
| Series | Make better use of static type checking | expand |
> On Apr 19, 2021, at 8:08 PM, Bart Van Assche <bvanassche@acm.org> wrote: > > An explanation of the purpose of this patch is available in the patch > "scsi: Introduce the scsi_status union". > > Cc: "J. Bruce Fields" <bfields@fieldses.org> > Cc: Chuck Lever <chuck.lever@oracle.com> > Signed-off-by: Bart Van Assche <bvanassche@acm.org> Hi Bart, I assume this is going into v5.13 via the SCSI tree? Do you need an Acked-by: from the NFSD maintainers? > --- > fs/nfsd/blocklayout.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/fs/nfsd/blocklayout.c b/fs/nfsd/blocklayout.c > index 1058659a8d31..f10f559684a6 100644 > --- a/fs/nfsd/blocklayout.c > +++ b/fs/nfsd/blocklayout.c > @@ -255,9 +255,9 @@ static int nfsd4_scsi_identify_device(struct block_device *bdev, > req->cmd_len = COMMAND_SIZE(INQUIRY); > > blk_execute_rq(NULL, rq, 1); > - if (req->result) { > + if (req->status.combined) { > pr_err("pNFS: INQUIRY 0x83 failed with: %x\n", > - req->result); > + req->status.combined); > error = -EIO; > goto out_put_request; > } -- Chuck Lever
On 4/20/21 7:36 AM, Chuck Lever III wrote: >> On Apr 19, 2021, at 8:08 PM, Bart Van Assche <bvanassche@acm.org> wrote: >> An explanation of the purpose of this patch is available in the patch >> "scsi: Introduce the scsi_status union". >> >> Cc: "J. Bruce Fields" <bfields@fieldses.org> >> Cc: Chuck Lever <chuck.lever@oracle.com> >> Signed-off-by: Bart Van Assche <bvanassche@acm.org> > > Hi Bart, I assume this is going into v5.13 via the SCSI tree? > Do you need an Acked-by: from the NFSD maintainers? Hi Chuck, Thanks for having taken a look. In case you would not yet have found the "scsi: Introduce the scsi_status union" patch, it is available here: https://lore.kernel.org/linux-scsi/20210420000845.25873-12-bvanassche@acm.org/T/#u An Acked-by or Reviewed-by from an NFS expert would be great. The names in the Cc: list come from the following entry in the MAINTAINERS file: "KERNEL NFSD, SUNRPC, AND LOCKD SERVERS". Bart.
> On Apr 20, 2021, at 12:44 PM, Bart Van Assche <bvanassche@acm.org> wrote: > > On 4/20/21 7:36 AM, Chuck Lever III wrote: >>> On Apr 19, 2021, at 8:08 PM, Bart Van Assche <bvanassche@acm.org> wrote: >>> An explanation of the purpose of this patch is available in the patch >>> "scsi: Introduce the scsi_status union". >>> >>> Cc: "J. Bruce Fields" <bfields@fieldses.org> >>> Cc: Chuck Lever <chuck.lever@oracle.com> >>> Signed-off-by: Bart Van Assche <bvanassche@acm.org> >> >> Hi Bart, I assume this is going into v5.13 via the SCSI tree? >> Do you need an Acked-by: from the NFSD maintainers? > > Hi Chuck, > > Thanks for having taken a look. In case you would not yet have found the > "scsi: Introduce the scsi_status union" patch, it is available here: > https://lore.kernel.org/linux-scsi/20210420000845.25873-12-bvanassche@acm.org/T/#u > > An Acked-by or Reviewed-by from an NFS expert would be great. The NFSD patch looks OK to me, but I'm hesitating on sending an Acked-by. I went back and looked at the scsi_status union patch, and that looks dodgy to me. AFAIK, "enum" doesn't cause the compiler to reserve any particular size of storage, it just makes a guess. What keeps those enum fields from being 16- or 32-bits wide? Shouldn't those be u8 to enforce the correct field size? I'm not sure where to look for further discussion on that part of the series. > The names in the Cc: list come from the following entry in the > MAINTAINERS file: "KERNEL NFSD, SUNRPC, AND LOCKD SERVERS". > > Bart. -- Chuck Lever
On 4/21/21 7:22 AM, Chuck Lever III wrote: > > >> On Apr 20, 2021, at 12:44 PM, Bart Van Assche <bvanassche@acm.org> wrote: >> >> On 4/20/21 7:36 AM, Chuck Lever III wrote: >>>> On Apr 19, 2021, at 8:08 PM, Bart Van Assche <bvanassche@acm.org> wrote: >>>> An explanation of the purpose of this patch is available in the patch >>>> "scsi: Introduce the scsi_status union". >>>> >>>> Cc: "J. Bruce Fields" <bfields@fieldses.org> >>>> Cc: Chuck Lever <chuck.lever@oracle.com> >>>> Signed-off-by: Bart Van Assche <bvanassche@acm.org> >>> >>> Hi Bart, I assume this is going into v5.13 via the SCSI tree? >>> Do you need an Acked-by: from the NFSD maintainers? >> >> Hi Chuck, >> >> Thanks for having taken a look. In case you would not yet have found the >> "scsi: Introduce the scsi_status union" patch, it is available here: >> https://lore.kernel.org/linux-scsi/20210420000845.25873-12-bvanassche@acm.org/T/#u >> >> An Acked-by or Reviewed-by from an NFS expert would be great. > > The NFSD patch looks OK to me, but I'm hesitating on sending > an Acked-by. > > I went back and looked at the scsi_status union patch, and > that looks dodgy to me. > > AFAIK, "enum" doesn't cause the compiler to reserve any > particular size of storage, it just makes a guess. What > keeps those enum fields from being 16- or 32-bits wide? > Shouldn't those be u8 to enforce the correct field size? > > I'm not sure where to look for further discussion on that > part of the series. Hi Chuck, Although the C standard requires that enums have the same size as an int, gcc and clang support the attribute "packed" for enums. From the gcc documentation about the packed attribute: "When attached to an enum definition, it indicates that the smallest integral type should be used." Additionally, the following BUILD_BUG_ON() statements verify the size and endianness of the members of the scsi_status union (see also https://www.spinics.net/lists/linux-scsi/msg157796.html): +#define TEST_STATUS ((union scsi_status){.combined = 0x01020308}) + static int __init init_scsi(void) { int error; + BUILD_BUG_ON(sizeof(union scsi_status) != 4); + BUILD_BUG_ON(TEST_STATUS.combined != 0x01020308); + BUILD_BUG_ON(driver_byte(TEST_STATUS) != 1); + BUILD_BUG_ON(host_byte(TEST_STATUS) != 2); + BUILD_BUG_ON(msg_byte(TEST_STATUS) != 3); + BUILD_BUG_ON(status_byte(TEST_STATUS) != 4); Does this address your concern? Bart.
> On Apr 21, 2021, at 12:12 PM, Bart Van Assche <bvanassche@acm.org> wrote: > > On 4/21/21 7:22 AM, Chuck Lever III wrote: >>> On Apr 20, 2021, at 12:44 PM, Bart Van Assche <bvanassche@acm.org> wrote: >>> >>> On 4/20/21 7:36 AM, Chuck Lever III wrote: >>>>> On Apr 19, 2021, at 8:08 PM, Bart Van Assche <bvanassche@acm.org> wrote: >>>>> An explanation of the purpose of this patch is available in the patch >>>>> "scsi: Introduce the scsi_status union". >>>>> >>>>> Cc: "J. Bruce Fields" <bfields@fieldses.org> >>>>> Cc: Chuck Lever <chuck.lever@oracle.com> >>>>> Signed-off-by: Bart Van Assche <bvanassche@acm.org> >>>> >>>> Hi Bart, I assume this is going into v5.13 via the SCSI tree? >>>> Do you need an Acked-by: from the NFSD maintainers? >>> >>> Hi Chuck, >>> >>> Thanks for having taken a look. In case you would not yet have found the >>> "scsi: Introduce the scsi_status union" patch, it is available here: >>> https://lore.kernel.org/linux-scsi/20210420000845.25873-12-bvanassche@acm.org/T/#u >>> >>> An Acked-by or Reviewed-by from an NFS expert would be great. >> The NFSD patch looks OK to me, but I'm hesitating on sending >> an Acked-by. >> I went back and looked at the scsi_status union patch, and >> that looks dodgy to me. >> AFAIK, "enum" doesn't cause the compiler to reserve any >> particular size of storage, it just makes a guess. What >> keeps those enum fields from being 16- or 32-bits wide? >> Shouldn't those be u8 to enforce the correct field size? >> I'm not sure where to look for further discussion on that >> part of the series. > > Hi Chuck, > > Although the C standard requires that enums have the same size as an int, gcc and clang support the attribute "packed" for enums. From the gcc documentation about the packed attribute: "When attached to an enum definition, it indicates that the smallest integral type should be used." > > Additionally, the following BUILD_BUG_ON() statements verify the size and endianness of the members of the scsi_status union (see also https://www.spinics.net/lists/linux-scsi/msg157796.html): > > +#define TEST_STATUS ((union scsi_status){.combined = 0x01020308}) > + > static int __init init_scsi(void) > { > int error; > > + BUILD_BUG_ON(sizeof(union scsi_status) != 4); > + BUILD_BUG_ON(TEST_STATUS.combined != 0x01020308); > + BUILD_BUG_ON(driver_byte(TEST_STATUS) != 1); > + BUILD_BUG_ON(host_byte(TEST_STATUS) != 2); > + BUILD_BUG_ON(msg_byte(TEST_STATUS) != 3); > + BUILD_BUG_ON(status_byte(TEST_STATUS) != 4); > > Does this address your concern? Yes: a compile-time check that these assumptions are being met is good enough for me. Acked-by: Chuck Lever <chuck.lever@oracle.com> -- Chuck Lever
diff --git a/fs/nfsd/blocklayout.c b/fs/nfsd/blocklayout.c index 1058659a8d31..f10f559684a6 100644 --- a/fs/nfsd/blocklayout.c +++ b/fs/nfsd/blocklayout.c @@ -255,9 +255,9 @@ static int nfsd4_scsi_identify_device(struct block_device *bdev, req->cmd_len = COMMAND_SIZE(INQUIRY); blk_execute_rq(NULL, rq, 1); - if (req->result) { + if (req->status.combined) { pr_err("pNFS: INQUIRY 0x83 failed with: %x\n", - req->result); + req->status.combined); error = -EIO; goto out_put_request; }
An explanation of the purpose of this patch is available in the patch "scsi: Introduce the scsi_status union". Cc: "J. Bruce Fields" <bfields@fieldses.org> Cc: Chuck Lever <chuck.lever@oracle.com> Signed-off-by: Bart Van Assche <bvanassche@acm.org> --- fs/nfsd/blocklayout.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)