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" <[email protected]> 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 <[email protected]> > > > >> > Reviewed-by: Philippe Mathieu-Daudé <[email protected]> > > > >> > Reviewed-by: Bin Meng <[email protected]> > > > >> > Reviewed-by: Taylor Simpson <[email protected]> > > > >> > Reviewed-by: Alistair Francis <[email protected]> > > > >> > > > >> 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 <[email protected]> > > > >> > > > >> --- > > > >> > > > >> 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
