> On 2 May 2024, at 07:43, Jan Beulich <[email protected]> wrote: > > On 02.05.2024 08:33, Luca Fancellu wrote: >> >> >>> On 2 May 2024, at 07:14, Jan Beulich <[email protected]> wrote: >>> >>> On 01.05.2024 08:57, Luca Fancellu wrote: >>>> Hi Jan, >>>> >>>>> On 30 Apr 2024, at 12:37, Jan Beulich <[email protected]> wrote: >>>>> >>>>> On 30.04.2024 13:09, Luca Fancellu wrote: >>>>>> --- a/xen/arch/arm/include/asm/setup.h >>>>>> +++ b/xen/arch/arm/include/asm/setup.h >>>>>> @@ -64,18 +64,20 @@ struct membank { >>>>>> }; >>>>>> >>>>>> struct membanks { >>>>>> - unsigned int nr_banks; >>>>>> - unsigned int max_banks; >>>>>> + __struct_group(membanks_hdr, common, , >>>>>> + unsigned int nr_banks; >>>>>> + unsigned int max_banks; >>>>>> + ); >>>>>> struct membank bank[]; >>>>>> }; >>>>> >>>>> I'm afraid I can't spot why __struct_group() is needed here. Why would >>>>> just >>>>> one of the two more straightforward >>>>> >>>>> struct membanks { >>>>> struct membanks_hdr { >>>>> unsigned int nr_banks; >>>>> unsigned int max_banks; >>>>> ); >>>>> struct membank bank[]; >>>>> }; >>>>> >>>> >>>> At the first sight I thought this solution could have worked, however GCC >>>> brought me back down to earth >>>> remembering me that flexible array members can’t be left alone in an empty >>>> structure: >>>> >>>> /data_sdc/lucfan01/gitlab_mickledore_xen/xen/xen/arch/arm/include/asm/setup.h:70:6: >>>> error: declaration does not declare anything [-Werror] >>>> 70 | }; >>>> | ^ >>>> /data_sdc/lucfan01/gitlab_mickledore_xen/xen/xen/arch/arm/include/asm/setup.h:71:20: >>>> error: flexible array member in a struct with no named members >>>> 71 | struct membank bank[]; >>>> | ^~~~ >>>> [...] >>> >>> Since for patch 1 you looked at Linux'es uapi/linux/stddef.h, the solution >>> to this lies there, in __DECLARE_FLEX_ARRAY(). Alongside or instead of >>> borrowing __struct_group(), we could consider borrowing this as well. Or >>> open-code it just here, for the time being (perhaps my preference). Yet >>> it's not clear to me that doing so will actually be enough to make things >>> work for you. >> >> I looked also into __DECLARE_FLEX_ARRAY(), but then decided __struct_group() >> was enough for my purpose, can I ask the technical reasons why it would be >> your >> preference? Is there something in that construct that is a concern for you? > > I don't like either construct very much, but of the two __DECLARE_FLEX_ARRAY() > looks slightly more "natural" for what is wanted and how it's done. > __struct_group() introducing twice the (effectively) same structure feels > pretty odd, for now at least. It's not even entirely clear to me whether there > aren't pitfalls, seeing that the C spec differentiates named and unnamed > struct fields in a few cases. For __DECLARE_FLEX_ARRAY(), otoh, I can't > presently see any reason to suspect possible corner cases. > > Yet as said before - I'm not sure __DECLARE_FLEX_ARRAY() alone would be enough > for what you want to achieve.
Mmm yes, I think I would still have problems because container_of wants a named member, so __DECLARE_FLEX_ARRAY() doesn’t help much alone, if I’m not missing something obvious. If you believe however that importing __struct_group() only for this instance is not enough to justify its presence in the codebase, I could open-code it, provided that maintainers are ok with that. In any case it would be used soon also for other architectures using bootinfo.
