Re: [Qemu-devel] [PATCH 00/12] qemu-common.h/osdep.h refactoring

[Markus Armbruster]

Peter Maydell <address@hidden> writes:

> This series makes a start at cleaning up some of our headers
> to avoid the common problem of header files including qemu-common.h
> (which then in turn can lead to awkward circular includes).

Yes, please!

> One common cause of this is that we don't have any header
> which will include the basic things most header files require,
> except for qemu-common.h. This series fixes that by making
> 'osdep.h' that "common basic stuff" header. The idea is that:
>  * osdep.h can be included from anywhere, since it doesn't
>    include any other QEMU headers itself except a few very
>    restricted special purpose ones (config-host.h, compiler.h, etc)
>  * osdep.h provides:
>    + things everybody needs, like NULL, int32_t, container_of,
>      the CONFIG_* defines, etc
>    + things that will cause subtle problems if they're not
>      present everywhere (eg directly using system headers and
>      not getting the portability fixups will result in something
>      that builds on most but not all hosts)

I'd file the CONFIG_FOO under "things that may cause subtle problems if
they're not present everywhere" rather than "things everybody needs",
because

1. loads of code doesn't need any, and

2. code that does need it is prone to compile fine but misbehave when
the header providing its CONFIG_FOOs isn't included.

> So most places can just include osdep.h, not the full qemu-common.h.
>
> (I initially thought about defining a new header for this
> purpose, "qemu/basics.h" or some such, but in fact osdep.h
> was already very close to what I wanted so it didn't really
> seem like it was very useful to switch everything over.)
>
> The series has some minor cleanups, some shuffling around of
> things between qemu-common.h, osdep.h and compiler.h, and
> a couple of patches at the end that use osdep.h to allow
> dropping a qemu-common.h include from various header files,
> as a demonstration.
>
> The real question here I guess is whether people like the
> direction I'm trying to go with this. If so, we can further
> reduce the number of qemu-common.h includes without too much
> difficulty with further patches.

For what it's worth, here's how Autoconf wants things done: every .c
file includes config.h *first*, and nothing else includes it.  Easy to
check mechanically, safely avoids the subtle miscompiles mentioned
above.

config.h normally contains just #defines computed by configure, but you
can add whatever else you like, with AH_TOP() or AH_BOTTOM().

It normally is the "things that may cause subtle problems if they're not
present everywhere", but not "things everybody needs (but doesn't want
to include again and again)".  However, nothing stops you from adding
the latter to config.h as well.

My point is:

* Your envisaged role of osdep.h is a common, proven technique.

* Consider requiring every .c to include it before anything else, with
  no other includes allowed.

[...]