Re: [PATCH 075/117] nfsd: Convert to the scsi_status union

Bart Van Assche
2021-04-21T16:12:12Z

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.