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

[Markus Armbruster]

"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?

>>  #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