Re: [PATCH v3 1/1] include: Don't include qemu/osdep.h

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Jan 12, 2023 at 05:44:31PM +0000, Daniel P. Berrangé wrote:
> On Thu, Jan 12, 2023 at 12:37:46PM -0500, Michael S. Tsirkin wrote:
> > On Thu, Jan 12, 2023 at 03:47:19PM +0100, Markus Armbruster wrote:
> > > "Michael S. Tsirkin" <mst@redhat.com> writes:
> > > 
> > > > On Thu, Jan 12, 2023 at 08:51:32AM -0500, Michael S. Tsirkin wrote:
> > > >> On Thu, Jan 12, 2023 at 12:50:05PM +0100, Markus Armbruster wrote:
> > > >> > docs/devel/style.rst mandates:
> > > >> > 
> > > >> >     The "qemu/osdep.h" header contains preprocessor macros that affect
> > > >> >     the behavior of core system headers like <stdint.h>.  It must be
> > > >> >     the first include so that core system headers included by external
> > > >> >     libraries get the preprocessor macros that QEMU depends on.
> > > >> > 
> > > >> >     Do not include "qemu/osdep.h" from header files since the .c file
> > > >> >     will have already included it.
> > > >> > 
> > > >> > A few violations have crept in.  Fix them.
> > > >> > 
> > > >> > Signed-off-by: Markus Armbruster <armbru@redhat.com>
> > > >> > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > > >> > Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
> > > >> > Reviewed-by: Taylor Simpson <tsimpson@quicinc.com>
> > > >> > Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> > > >> 
> > > >> With my awesome grep skillz I found one more:
> > > >> $ grep -r --include='*.h' qemu/osdep.h
> > > >> include/block/graph-lock.h:#include "qemu/osdep.h"
> > > 
> > > Crept in after I prepared my v1.  I neglected to re-check.
> > > 
> > > > Also:
> > > > $ grep -r --include='*.inc' qemu/osdep.h
> > > > ui/vnc-enc-zrle.c.inc:#include "qemu/osdep.h"
> > > > crypto/akcipher-nettle.c.inc:#include "qemu/osdep.h"
> > > > crypto/akcipher-gcrypt.c.inc:#include "qemu/osdep.h"
> > > > crypto/rsakey-nettle.c.inc:#include "qemu/osdep.h"
> > > > crypto/cipher-gnutls.c.inc:#include "qemu/osdep.h"
> > > > target/xtensa/core-dc233c/xtensa-modules.c.inc:#include "qemu/osdep.h"
> > > > target/xtensa/core-sample_controller/xtensa-modules.c.inc:#include "qemu/osdep.h"
> > > > target/xtensa/core-de212/xtensa-modules.c.inc:#include "qemu/osdep.h"
> > > > target/xtensa/core-dc232b/xtensa-modules.c.inc:#include "qemu/osdep.h"
> > > > target/xtensa/core-fsf/xtensa-modules.c.inc:#include "qemu/osdep.h"
> > > > target/cris/translate_v10.c.inc:#include "qemu/osdep.h"
> > > 
> > > Good point.  Looks like I successfully supressed all memory of .inc.
> > > 
> > > >> Looks like all C files must include qemu/osdep.h, no?
> > > 
> > > I remember there are a few exceptions, but I don't remember which .c
> > > they are.  Hmm... see commit 4bd802b209cff612d1a99674a91895b735be8630.
> > > 
> > > >> How about
> > > >> 
> > > >> 1- add -include qemu/osdep.h on compile command line
> > > >>    drop #include "qemu/osdep.h" from C files
> > > 
> > > Then you need to encode the exceptions in the build system.  Which might
> > > not be a bad thing.
> > > 
> > > >> 2- drop double include guards, replace with a warning.
> > > >> 
> > > >> following patch implements part 2:
> > > >> 
> > > >> 
> > > >> qemu/osdep: don't include it from headers
> > > >> 
> > > >> doing so will lead to trouble eventually - instead of
> > > >> working around such cases make it more likely it will fail.
> > > >> 
> > > >> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > >> 
> > > >> ---
> > > >> 
> > > >> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> > > >> index 7d059ad526..e4a60f911c 100644
> > > >> --- a/include/qemu/osdep.h
> > > >> +++ b/include/qemu/osdep.h
> > > >> @@ -24,7 +24,12 @@
> > > >>   * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > > >>   * See the COPYING file in the top-level directory.
> > > >>   */
> > > >> -#ifndef QEMU_OSDEP_H
> > > >> +#ifdef QEMU_OSDEP_H
> > > >> +#warning "Never include qemu/osdep.h from a header!"
> > > >> +#endif
> > > >> +
> > > >> +static inline void qemu_osdep_never_include_from_header(void) {}
> > > >> +
> > > 
> > > Why do you need the function, too?
> > 
> > This seems to give a bit more info if header does get included
> > twice: instead of just a warning on the second include compiler says
> > definition is duplicated and then shows where the first definition was.
> > OTOH first one was almost for sure from the proper first include so
> > maybe we don't care. Let me drop this.
> 
> FWIW, if we want to simplify our header guards, we could replace the
> 
>   #ifndef FOO_H
>   #define FOO_H
> 
>   ....
> 
>   #endif /* FOO_H */
> 
> with merely
> 
>   #pragma once
> 
> at the top of each header.
> 
> With regards,
> Daniel

Will break this trick, won't it?

-- 
MST