Marc-André Lureau <marcandre.lur...@gmail.com> writes:
> Hi
>
> On Mon, Dec 10, 2018 at 1:52 PM Markus Armbruster <arm...@redhat.com> wrote:
>>
>> Marc-André Lureau <marcandre.lur...@redhat.com> 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 <marcandre.lur...@redhat.com>
>> > ---
>> > 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.
All we have right now to help with enforcing our osdep.h rule is
scripts/clean-includes. We run it periodically to fix rule breakers.
It's manual, because we have a few .c files in the tree where the rule
doesn't apply, and the script doesn't know about them. Fixable, I
guess. Most recent run:
Subject: [PATCH] Clean up includes
Message-Id: <20181204172535.2799-1-arm...@redhat.com>
https://lists.nongnu.org/archive/html/qemu-devel/2018-12/msg00605.html
The obvious improvement would be flagging rule breakers before they get
committed.
checkpatch.pl could flag patches adding .c files that don't include
"qemu/osdep.h" first. The "first" part might be a bit annoying to code.
The "adding files" part already exists: checkpatch.pl warns when you add
a file without updating MAINTAINERS.
checkpatch.pl could also flag patches removing #include "qemu/osdep.h".
Other projects use a syntax-check make target to check sources. If we
decide we like that better than checkpatch.pl for some checks, we can do
that.
