[Qemu-devel] RFC: use of qemu-common.h include

[Qemu-devel] RFC: use of qemu-common.h include

[Daniel P. Berrange]

In fixing the mingw64 problem wrt to localtime_r availability, I relied
on the fact that qemu-common.h is supposed to be included everywhere,
to guarantee that we always have unistd.h included before time.h:

  https://lists.gnu.org/archive/html/qemu-devel/2015-07/msg06202.html

That's nice in theory and it worked ok in practice in this case, but
looking at the QEMU codebase, use of qemu-common.h is less common and
less consistent than I would expect it to be.

The qemu-common.h file itself has this to say:

/* Common header file that is included by all of QEMU.
 *
 * This file is supposed to be included only by .c files. No header file should
 * depend on qemu-common.h, as this would easily lead to circular header
 * dependencies.
 *
 * If a header file uses a definition from qemu-common.h, that definition
 * must be moved to a separate header file, and the header that uses it
 * must include that header.
 */


We have a 1914 .c source files:

$ ./scripts/vc-list-files  | grep -E '\.c$' | wc -l
1914

But only 378 .c files include qemu-common.h

$ vc-list-files  | grep -E '\.c$'  | xargs grep qemu-common.h | wc -l
378

Along with 101 header files

$ vc-list-files  | grep -E '\.h$'  | xargs grep qemu-common.h | wc -l
101

If the qemu-common.h comment is to be believed, those 101 .h files
including qemu-common.h are all mistakes, and another 1500 .c files
should have qemu-common.h added.

For added fun there is inconsistency about when qemu-common.h is
included - some files include it as the first header and others
include it as the last header. Also there are many .h and .c files
which #include files that are already #included by qemu-common.h.
This is mostly harmless but can introduce bugs such as the one
I found with mingw64 localtime_r where order of inclusion is
significant.


I think the key thing if we are to clean up anyting is to add a
rule that can validate correct usage patterns. This is quite
straightforward - gnulib has code which many apps use to validate
that autotools config.h is included by every .c file as the first
#include that I could easily copy into QEMU and hook up to the
'make check' target.

So I'm wondering if there is appetite for cleaning this and and
introducing standard practice for inclusion of qemu-common.h ?

My inclination would be to make the code actually comply with
the state usage pattern in the comment I quoted above. ie

 - Remove #include "qemu-common.h" from all headers in include/
 - Add #include "qemu-common.h" to any .c files missing it
 - Make sure #include "qemu-common.h" is always the first
   #incude in a file.
 - Remove #include of any system headers that are already
   provided by qemu-common.h

Unless popular opinion says we should define a new pattern and
ignore the current comment....

I could easily see an argument that qemu-common.h is a flawed
idea and we should aim to delete it entirely and strive for
minimal neccessary includes. If we did that we might still
wish to say that 'config-host.h' was included in all .c files
so we guarantee configure results are universally available.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] RFC: use of qemu-common.h include

[Peter Maydell]

On 31 July 2015 at 13:06, Daniel P. Berrange <berrange@redhat.com> wrote:
> In fixing the mingw64 problem wrt to localtime_r availability, I relied
> on the fact that qemu-common.h is supposed to be included everywhere,
> to guarantee that we always have unistd.h included before time.h:

It's not really supposed to be included everywhere. It's just a
convenient way for a .c file to get a lot of stuff all at once.
It's a bit of a mess.

> So I'm wondering if there is appetite for cleaning this and and
> introducing standard practice for inclusion of qemu-common.h ?

You might want to have a look at my series which tries to
make osdep.h the "anybody can include this to get the really
key 'breaks if this isn't here' stuff".

http://lists.nongnu.org/archive/html/qemu-devel/2015-07/msg04593.html

thanks
-- PMM

Re: [Qemu-devel] RFC: use of qemu-common.h include

[Daniel P. Berrange]

On Fri, Jul 31, 2015 at 01:19:11PM +0100, Peter Maydell wrote:
> On 31 July 2015 at 13:06, Daniel P. Berrange <berrange@redhat.com> wrote:
> > In fixing the mingw64 problem wrt to localtime_r availability, I relied
> > on the fact that qemu-common.h is supposed to be included everywhere,
> > to guarantee that we always have unistd.h included before time.h:
> 
> It's not really supposed to be included everywhere. It's just a
> convenient way for a .c file to get a lot of stuff all at once.
> It's a bit of a mess.
> 
> > So I'm wondering if there is appetite for cleaning this and and
> > introducing standard practice for inclusion of qemu-common.h ?
> 
> You might want to have a look at my series which tries to
> make osdep.h the "anybody can include this to get the really
> key 'breaks if this isn't here' stuff".
> 
> http://lists.nongnu.org/archive/html/qemu-devel/2015-07/msg04593.html

Ah, interesting, I'll check out that series - it sounds like it is
similar to what i was thinking

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|