On 29.11.2023 12:58, Juergen Gross wrote: > On 09.08.23 11:42, Juergen Gross wrote: >> On 26.07.23 07:52, Jan Beulich wrote: >>> On 25.07.2023 18:59, Andrew Cooper wrote: >>>> On 25/07/2023 5:16 pm, Jan Beulich wrote: >>>>> On 25.07.2023 15:55, Juergen Gross wrote: >>>>>> Flexible arrays in public headers can be problematic with some >>>>>> compilers. >>>>>> >>>>>> Replace them with arr[XEN_FLEX_ARRAY_DIM] in order to avoid compilation >>>>>> errors. >>>>>> >>>>>> This includes arrays defined as "arr[1]", as seen with a recent Linux >>>>>> kernel [1]. >>>>>> >>>>>> [1]: https://bugzilla.kernel.org/show_bug.cgi?id=217693 >>>>>> >>>>>> Signed-off-by: Juergen Gross <[email protected]> >>>>> I think we need to be careful here: What if someone somewhere applies >>>>> sizeof() to any of the types you alter? >>>> >>>> Then the code was most likely wrong already. >>> >>> That's possible to judge only when seeing the code in question. >>> >>>>> The resulting value would >>>>> change with the changes you propose, which we cannot allow to happen >>>>> in a stable interface. Therefore imo it can only be an opt-in feature >>>>> to have these arrays no longer be one-element ones. >>>> >>>> I don't consider this an issue. >>>> >>>> If people take an update to the headers and their code stops compiling, >>>> then of course they fix the compilation issue. That's normal. >>> >>> The code may continue to compile fine, and even appear to work initially. >>> >>>> It's unreasonable to take opt-in features to a set of headers intended >>>> to be vendored in the first place, to work around a corner case that's >>>> likely buggy already. >>> >>> The original intention clearly was to allow use of these headers as is. >>> Anyway, I've voiced my view, yet if there are enough people agreeing >>> with you, then so be it. >> >> Any further thoughts? >> >> I have checked the code in the Linux kernel meanwhile. There should be no >> fallout resulting from this change, but I think there are some user mode >> backends outside of qemu which are probably using affected structs. > > I've received another mail regarding the report [1] above. I think we should > _really_ come to a conclusion. > > I'm still in favor of applying my suggested patch.
I think the change would be fine to make when adjusted to be conditional upon (suitably bumped) __XEN_LATEST_INTERFACE_VERSION__. Yet while looking at the patch and the headers again, it also looks as if there might be another small issue: ring.h uses XEN_FLEX_ARRAY_DIM without itself including xen.h. That's probably okay considering that all headers including ring.h also include grant_table.h (which in turn includes xen.h), but this dependency may still want making explicit. Finally - is the change actually going to help everywhere (not just in Linux)? It effectively depends on people enabling C99 mode. Older gcc for example didn't even define __STDC_VERSION__ when -std wasn't used. Linux doesn't permit use of such old gcc versions anymore, but recall we're aiming to be C89 compatible. Therefore I think that in addition we'd need a way for consumers of the headers to indicate that the C99 form of XEN_FLEX_ARRAY_DIM can be used even when __STDC_VERSION__ isn't defined. (This may as well simply be done by allowing people to pre-define XEN_FLEX_ARRAY_DIM before including any Xen headers.) Jan
