[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH for-4.0 v7 01/27] qapi: make sure osdep.h is inc
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH for-4.0 v7 01/27] qapi: make sure osdep.h is included in type headers |
Date: |
Wed, 12 Dec 2018 07:47:59 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) |
Marc-André Lureau <address@hidden> writes:
> Hi
>
> On Mon, Dec 10, 2018 at 5:28 PM Markus Armbruster <address@hidden> wrote:
>>
>> Marc-André Lureau <address@hidden> writes:
>>
>> > Hi
>> >
>> > On Mon, Dec 10, 2018 at 1:52 PM Markus Armbruster <address@hidden> wrote:
>> >>
>> >> Marc-André Lureau <address@hidden> writes:
>> >>
>> >> > Now that the schema can be configured, it is crucial that all types
>> >> > are configured the same. Make sure config-host.h is included, by
>> >> > checking osdep.h inclusion. The build-sys tracks the dependency and
>> >> > rebuilds the types if the configuration changed.
>> >> >
>> >> > Signed-off-by: Marc-André Lureau <address@hidden>
>> >> > ---
>> >> > scripts/qapi/types.py | 3 +++
>> >> > 1 file changed, 3 insertions(+)
>> >> >
>> >> > diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
>> >> > index fd7808103c..3bb9cb6d47 100644
>> >> > --- a/scripts/qapi/types.py
>> >> > +++ b/scripts/qapi/types.py
>> >> > @@ -201,6 +201,9 @@ class
>> >> > QAPISchemaGenTypeVisitor(QAPISchemaModularCVisitor):
>> >> > ''',
>> >> > types=types, visit=visit))
>> >> > self._genh.preamble_add(mcgen('''
>> >> > +#ifndef QEMU_OSDEP_H
>> >> > +#error Please include osdep.h first!
>> >> > +#endif
>> >> > #include "qapi/qapi-builtin-types.h"
>> >> > '''))
>> >>
>> >> I understand why you propose this patch. The QAPI-generated headers use
>> >> #ifdef CONFIG_FOO. The configuration header "qemu/osdep.h" must be
>> >> consistently included before the generated headers, or else you end up
>> >> with nasty bugs, such as the same enum having different values in
>> >> different .o, or the same struct having a different layout.
>> >>
>> >> But this applies to *all* headers that use #ifdef. Why check it here,
>> >> but not there? What makes the QAPI-generated headers special?
>> >>
>> >
>> > It's the discussion about #if in headers (and enums in particular)
>> > that started this. We want to make sure all compilation units share
>> > the same data structure/ABI. I proposed to include osdep.h in qapi
>> > headers, but that was rejected.
>> > The warning is a different approach. I agree it could apply to all
>> > headers. Do you think I should update all headers as well?
>>
>> That would replace the rule 'all .c files must include "qemu/osdep.h"
>> first' by 'all .h files must check "qemu/osdep.h" has been included
>> already', which is not an improvement.
>
> That would be an improvement since headers may also be impacted by osdep.h
>
> Alternatively, why not use -include ?
Requiring .c to include the configuration header first follows
established Autoconf practice. The "first" part covers all headers.
I've seen plenty of autoconfiscated code, yet none where the headers
double-check the configuration header has been included already. Such a
check is neither sufficient nor really necessary. Checking the .c is
both.
I have no strong feelings about using -include instead. I'd prefer to
avoid the difference to what other projects do, though.
Circling back to the issue that made you propose this patch.
I think I'm (belatedly) coming around to your initial view that the use
of conditionals in generated QAPI code is safe enough.
I shouldn't be worried about #if defined(CONFIG_HOST_THING) where
CONFIG_HOST_THING belongs to config-host.h. That's always pulled in via
qemu/osdep.h.
I also shouldn't be worried about #if defined(CONFIG_TARGET_THING) where
CONFIG_TARGET_THING belongs to config-target.h. qemu/osdep.h pulls in
either that or exec/poison.h.
I even shouldn't be worried about #if defined(RANDOM_MACRO) as long as
qemu/osdep.h pulls in its owner or poisons it. I might legitimately be
worried about the "as long as" part.
We could perhaps extract the ifconds and look for macros that belong
neither to config-host.h nor config-target.h. But I'm not sure it's
worth the bother.
[Qemu-devel] [PATCH for-4.0 v7 02/27] qapi: do not define enumeration value explicitly, Marc-André Lureau, 2018/12/08
[Qemu-devel] [PATCH for-4.0 v7 03/27] qapi: rename QAPISchemaEnumType.values to .members, Marc-André Lureau, 2018/12/08
[Qemu-devel] [PATCH for-4.0 v7 04/27] qapi: change enum visitor and gen_enum* to take QAPISchemaMember, Marc-André Lureau, 2018/12/08
[Qemu-devel] [PATCH for-4.0 v7 05/27] tests: print enum type members more like object type members, Marc-André Lureau, 2018/12/08
[Qemu-devel] [PATCH for-4.0 v7 06/27] qapi: factor out checking for keys, Marc-André Lureau, 2018/12/08