On 30.11.23 09:24, Jan Beulich wrote:
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__.
Okay, fine with me.
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.
Yes, I'll add that.
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.)
Will the problem even occur with such an old gcc? I don't think so, as only rather recent compilers showed the "array out of bounds" failure. Otherwise we would have heard complaints much earlier.
OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key
OpenPGP_signature.asc
Description: OpenPGP digital signature
