Re: [PATCH 1/2] xen/kernel.h: Import __struct_group from Linux

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.05.2024 08:23, Luca Fancellu wrote:
> 
> 
>> On 2 May 2024, at 07:09, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>
>> On 01.05.2024 08:54, Luca Fancellu wrote:
>>>> On 30 Apr 2024, at 12:43, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>>> On 30.04.2024 13:09, Luca Fancellu wrote:
>>>>> --- a/xen/include/xen/kernel.h
>>>>> +++ b/xen/include/xen/kernel.h
>>>>> @@ -54,6 +54,27 @@
>>>>>        typeof_field(type, member) *__mptr = (ptr);             \
>>>>>        (type *)( (char *)__mptr - offsetof(type,member) );})
>>>>>
>>>>> +/**
>>>>> + * __struct_group() - Create a mirrored named and anonyomous struct
>>>>> + *
>>>>> + * @TAG: The tag name for the named sub-struct (usually empty)
>>>>> + * @NAME: The identifier name of the mirrored sub-struct
>>>>> + * @ATTRS: Any struct attributes (usually empty)
>>>>> + * @MEMBERS: The member declarations for the mirrored structs
>>>>> + *
>>>>> + * Used to create an anonymous union of two structs with identical layout
>>>>> + * and size: one anonymous and one named. The former's members can be 
>>>>> used
>>>>> + * normally without sub-struct naming, and the latter can be used to
>>>>> + * reason about the start, end, and size of the group of struct members.
>>>>> + * The named struct can also be explicitly tagged for layer reuse, as 
>>>>> well
>>>>> + * as both having struct attributes appended.
>>>>> + */
>>>>> +#define __struct_group(TAG, NAME, ATTRS, MEMBERS...) \
>>>>> +    union { \
>>>>> +        struct { MEMBERS } ATTRS; \
>>>>> +        struct TAG { MEMBERS } ATTRS NAME; \
>>>>> +    } ATTRS
>>>>
>>>> Besides my hesitance towards having this construct, can you explain why
>>>> ATTR needs using 3 times, i.e. also on the wrapping union?
>>>
>>> The original commit didn’t have the third ATTRS, but afterwards it was 
>>> introduced due to
>>> this:
>>>
>>> https://patchwork.kernel.org/project/linux-wireless/patch/20231120110607.98956-1-dmantipov@xxxxxxxxx/#25610045
>>
>> Hmm. Since generally packing propagates to containing compound types, I'd
>> say this cannot go without a code comment, or at the very least a mention
>> in the description. But: Do we use this old ABI in Xen at all? IOW can we
>> get away without this 3rd instance?
> 
> Yes, I think it won’t be a problem for Xen, is it something that can be done 
> on commit?

Don't know, maybe. First you need an ack, and I remain unconvinced that we
actually need this construct.

Jan

> Anyway in case of comments on the second patch, I’ll push the change also for 
> this one.
> 
> Cheers,
> Luca
>