> On Apr 21, 2021, at 12:12 PM, Bart Van Assche <bvanassche@xxxxxxx> wrote: > > On 4/21/21 7:22 AM, Chuck Lever III wrote: >>> On Apr 20, 2021, at 12:44 PM, Bart Van Assche <bvanassche@xxxxxxx> wrote: >>> >>> On 4/20/21 7:36 AM, Chuck Lever III wrote: >>>>> On Apr 19, 2021, at 8:08 PM, Bart Van Assche <bvanassche@xxxxxxx> 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@xxxxxxxxxxxx> >>>>> Cc: Chuck Lever <chuck.lever@xxxxxxxxxx> >>>>> Signed-off-by: Bart Van Assche <bvanassche@xxxxxxx> >>>> >>>> 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@xxxxxxx/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@xxxxxxxxxx> -- Chuck Lever
