Re: [Qemu-devel] [PATCH for-4.0 v7 01/27] qapi: make sure osdep.h is included in type headers

[Daniel P . Berrangé]

On Mon, Dec 10, 2018 at 02:28:26PM +0100, Markus Armbruster 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.
> 
> 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: <address@hidden>
>     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.

You can get this logic from GNULIBs syntax-check make rules. It uses
this perl code to check that config.h is included first:

    while (<>) {
        if (/^# *include\b/) {
            if (not m{^# *include <config\.h>}) {
                print "$ARGV\n";
                $$ret = 1;
            }
            close ARGV;
        }
    }


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|