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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

> Now, I have to say that for the Origin tag I used the SHA of the commit 
> introducing the macro
> and the SHA doing this modification is different, how are these cases handled?

I'd say the hash of the original commit is enough even if the 3rd instance
needs keeping for whatever reason.

Jan