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

[Michael S. Tsirkin]

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.

> >>  #define QEMU_OSDEP_H
> >>  
> >>  #include "config-host.h"
> >> @@ -714,5 +719,3 @@ static inline int 
> >> platform_does_not_support_system(const char *command)
> >>  #ifdef __cplusplus
> >>  }
> >>  #endif
> >> -
> >> -#endif