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

Chuck Lever III
2021-04-21T16:27:21Z

> 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