On 02.05.2024 10:13, Luca Fancellu wrote: > > >> 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.
I'm afraid I've even more strongly against open-coding. If you can get an ack from another maintainer for the introduction of struct_group(), that would still allow it to go in. I didn't NAK the change, I merely have reservations. > In any case it would be used soon also for other architectures using bootinfo. Oh, would it? Jan
