Re: [PATCH] xen/public: fix flexible array definitions

[Jan Beulich <jbeulich@xxxxxxxx>]

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 <jgross@xxxxxxxx>
>>>>> 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