[Qemu-devel] [PATCH v2] qemu: replace "" with <> in headers

[Qemu-devel] [PATCH v2] qemu: replace "" with <> in headers

[Michael S. Tsirkin]

Our current scheme is to use
 #include ""
for internal headers, and
 #include <>
for external ones.

Unfortunately this is not based on compiler support: from C point of
view, the "" form merely looks up headers in the current directory
and then falls back on <> directories.

Thus, for example, a system header trace.h - should it be present - will
conflict with our local trace.h

As another example of problems, a header by the same name in the source
directory will always be picked up first - before any headers in
the include directory.

Let's change the scheme: make sure all headers that are not
in the source directory are included through a path
starting with qemu/ , thus:

 #include <>

headers in the same directory as source are included with

 #include ""

as per standard.

This (untested) patch is just to start the discussion and does not
change all of the codebase. If there's agreement, this will be
run on all code to converting code to this scheme.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 scripts/changeheaders.pl | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)
 create mode 100755 scripts/changeheaders.pl

diff --git a/scripts/changeheaders.pl b/scripts/changeheaders.pl
new file mode 100755
index 0000000..22bd5b8
--- /dev/null
+++ b/scripts/changeheaders.pl
@@ -0,0 +1,20 @@
+#!/usr/bin/perl -pi
+
+if (m@^\s*#include\s+"([^"+]"@o) {
+    next;
+}
+
+my $hdr = $1;
+my $file = $ARGV;
+$file =~ s@/[^/]+$@@g;
+$file .= $hdr;
+
+if (-e $file) {
+    next;
+}
+
+if (m@^\s*#include\s+"qemu/@o) {
+    s@^(\s*#include\s+)"qemu/([^"]+)"(.*)$@$1<qemu/common/$2>$3@o) {
+} else {
+    s@^(\s*#include\s+)"([^"]+)"(.*)$@$1<qemu/$2>$3@o) {
+}
-- 
MST

Re: [Qemu-devel] [PATCH v2] qemu: replace "" with <> in headers

[Paolo Bonzini]

On 21/03/2018 15:46, Michael S. Tsirkin wrote:
> +if (m@^\s*#include\s+"qemu/@o) {
> +    s@^(\s*#include\s+)"qemu/([^"]+)"(.*)$@$1<qemu/common/$2>$3@o) {
> +} else {
> +    s@^(\s*#include\s+)"([^"]+)"(.*)$@$1<qemu/$2>$3@o) {
> +}

Can you explain the changes in the source tree layout?

Also, s{}{} and m{} are a bit more readable.

Paolo

Re: [Qemu-devel] [PATCH v2] qemu: replace "" with <> in headers

[Michael S. Tsirkin]

On Wed, Mar 21, 2018 at 04:04:29PM +0100, Paolo Bonzini wrote:
> On 21/03/2018 15:46, Michael S. Tsirkin wrote:
> > +if (m@^\s*#include\s+"qemu/@o) {
> > +    s@^(\s*#include\s+)"qemu/([^"]+)"(.*)$@$1<qemu/common/$2>$3@o) {
> > +} else {
> > +    s@^(\s*#include\s+)"([^"]+)"(.*)$@$1<qemu/$2>$3@o) {
> > +}
> 
> Can you explain the changes in the source tree layout?

include/qemu -> include/qemu/common
include/* -> include/qemu/*

Thus one uses any qemu headers with

#include <qemu/....>

we can do the conversion gradually and avoid a flag day
with some use of softlinks.

> Also, s{}{} and m{} are a bit more readable.
> 
> Paolo

Thanks, will use.


-- 
MST

Re: [Qemu-devel] [PATCH v2] qemu: replace "" with <> in headers

[Paolo Bonzini]

On 21/03/2018 16:11, Michael S. Tsirkin wrote:
>> Can you explain the changes in the source tree layout?
> include/qemu -> include/qemu/common
> include/* -> include/qemu/*

Ok, then perhaps "util" instead of common would match the source layout
more.

Paolo

> Thus one uses any qemu headers with
> 
> #include <qemu/....>

Re: [Qemu-devel] [PATCH v2] qemu: replace "" with <> in headers

[Daniel P. Berrangé]

On Wed, Mar 21, 2018 at 04:46:32PM +0200, Michael S. Tsirkin wrote:
> Our current scheme is to use
>  #include ""
> for internal headers, and
>  #include <>
> for external ones.
> 
> Unfortunately this is not based on compiler support: from C point of
> view, the "" form merely looks up headers in the current directory
> and then falls back on <> directories.
> 
> Thus, for example, a system header trace.h - should it be present - will
> conflict with our local trace.h

If our local "trace.h" is in the current directory, then using ""
is right and you can still use <trace.h> to get the system version.

If our local trace.h is in include/ top level, then it is going to
block use of the system trace.h regardless of whether we use <> or ""

Fortunately our include/ tree uses sub-dirs, so we would typically
use  #include "$subdir/trace.h" and  #include <trace.h> would still
find the system header.

We just have to be careful we don't add stuff at the top level of
our include/ dir with names that are liable to clash. This might
suggest renaming  include/elf.h to include/qemu/elf.h, or just
moving elf.h to the qemu/ subdirectory. Likewise include/glib-compat.h
might be better moved to qemu/ subdirectory.


> As another example of problems, a header by the same name in the source
> directory will always be picked up first - before any headers in
> the include directory.

There's only a couple of headers in the top level of our include/
directory - everything else is pulled in with a named path
eg #include "block/block_int.h", so that would not conflict with
reference to a bare #include "block_int.h" from the current directory.

> Let's change the scheme: make sure all headers that are not
> in the source directory are included through a path
> starting with qemu/ , thus:
> 
>  #include <>
> 
> headers in the same directory as source are included with
> 
>  #include ""
> 
> as per standard.

As stated before, I consider this a step backwards - it is a
good clear standard to use "" for project local includes and
<> for 3rd party / system includes IMHO. The change doesn't
do anything beneficial for the two scenarios described above
AFAICT.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v2] qemu: replace "" with <> in headers

[Michael S. Tsirkin]

On Wed, Mar 21, 2018 at 03:19:22PM +0000, Daniel P. Berrangé wrote:
> On Wed, Mar 21, 2018 at 04:46:32PM +0200, Michael S. Tsirkin wrote:
> > Our current scheme is to use
> >  #include ""
> > for internal headers, and
> >  #include <>
> > for external ones.
> > 
> > Unfortunately this is not based on compiler support: from C point of
> > view, the "" form merely looks up headers in the current directory
> > and then falls back on <> directories.
> > 
> > Thus, for example, a system header trace.h - should it be present - will
> > conflict with our local trace.h
> 
> If our local "trace.h" is in the current directory, then using ""
> is right and you can still use <trace.h> to get the system version.
> 
> If our local trace.h is in include/ top level, then it is going to
> block use of the system trace.h regardless of whether we use <> or ""
> 
> Fortunately our include/ tree uses sub-dirs, so we would typically
> use  #include "$subdir/trace.h" and  #include <trace.h> would still
> find the system header.
> We just have to be careful we don't add stuff at the top level of
> our include/ dir with names that are liable to clash. This might
> suggest renaming  include/elf.h to include/qemu/elf.h, or just
> moving elf.h to the qemu/ subdirectory. Likewise include/glib-compat.h
> might be better moved to qemu/ subdirectory.
> 

This is exactly what this patch proposes, with a uniform scheme:
start everything with qemu/.

> 
> > As another example of problems, a header by the same name in the source
> > directory will always be picked up first - before any headers in
> > the include directory.
> 
> There's only a couple of headers in the top level of our include/
> directory - everything else is pulled in with a named path
> eg #include "block/block_int.h", so that would not conflict with
> reference to a bare #include "block_int.h" from the current directory.

We can not know that there are no system headers that start with block/ on
any current or future systems.

> > Let's change the scheme: make sure all headers that are not
> > in the source directory are included through a path
> > starting with qemu/ , thus:
> > 
> >  #include <>
> > 
> > headers in the same directory as source are included with
> > 
> >  #include ""
> > 
> > as per standard.
> 
> As stated before, I consider this a step backwards - it is a
> good clear standard to use "" for project local includes and
> <> for 3rd party / system includes IMHO. The change doesn't
> do anything beneficial for the two scenarios described above
> AFAICT.

I think you are mistaken on the last point:
1. Everything will be under qemu/ so we never clash with a system file
2. A local stale file anywhere in source directory is completely ignored
   since source is not on -I path.

I hope this clarifies things.

> 
> Regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v2] qemu: replace "" with <> in headers

[Daniel P. Berrangé]

On Wed, Mar 21, 2018 at 05:39:48PM +0200, Michael S. Tsirkin wrote:
> On Wed, Mar 21, 2018 at 03:19:22PM +0000, Daniel P. Berrangé wrote:
> > On Wed, Mar 21, 2018 at 04:46:32PM +0200, Michael S. Tsirkin wrote:
> > > Our current scheme is to use
> > >  #include ""
> > > for internal headers, and
> > >  #include <>
> > > for external ones.
> > > 
> > > Unfortunately this is not based on compiler support: from C point of
> > > view, the "" form merely looks up headers in the current directory
> > > and then falls back on <> directories.
> > > 
> > > Thus, for example, a system header trace.h - should it be present - will
> > > conflict with our local trace.h
> > 
> > If our local "trace.h" is in the current directory, then using ""
> > is right and you can still use <trace.h> to get the system version.
> > 
> > If our local trace.h is in include/ top level, then it is going to
> > block use of the system trace.h regardless of whether we use <> or ""
> > 
> > Fortunately our include/ tree uses sub-dirs, so we would typically
> > use  #include "$subdir/trace.h" and  #include <trace.h> would still
> > find the system header.
> > We just have to be careful we don't add stuff at the top level of
> > our include/ dir with names that are liable to clash. This might
> > suggest renaming  include/elf.h to include/qemu/elf.h, or just
> > moving elf.h to the qemu/ subdirectory. Likewise include/glib-compat.h
> > might be better moved to qemu/ subdirectory.
> > 
> 
> This is exactly what this patch proposes, with a uniform scheme:
> start everything with qemu/.
> 
> > 
> > > As another example of problems, a header by the same name in the source
> > > directory will always be picked up first - before any headers in
> > > the include directory.
> > 
> > There's only a couple of headers in the top level of our include/
> > directory - everything else is pulled in with a named path
> > eg #include "block/block_int.h", so that would not conflict with
> > reference to a bare #include "block_int.h" from the current directory.
> 
> We can not know that there are no system headers that start with block/ on
> any current or future systems.

Ah true, good point.  I guess that's where the benefit of -iquote
comes into play.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v2] qemu: replace "" with <> in headers

[Kevin Wolf]

Am 21.03.2018 um 15:46 hat Michael S. Tsirkin geschrieben:
> Our current scheme is to use
>  #include ""
> for internal headers, and
>  #include <>
> for external ones.
> 
> Unfortunately this is not based on compiler support: from C point of
> view, the "" form merely looks up headers in the current directory
> and then falls back on <> directories.
> 
> Thus, for example, a system header trace.h - should it be present - will
> conflict with our local trace.h

You're right that there is a conflict, even though only in one
direction: "trace.h" is unambiguously the local trace.h in our source
tree, but <trace.h> refers to the same local header rather than the
system header as you would expect.

An easy way to resolve this conflict would be using -iquote rather than
-I for directories in the source tree, so that <trace.h> unambiguously
refers to the system header and "trace.h" unambiguously refers to the
QEMU header.

> As another example of problems, a header by the same name in the source
> directory will always be picked up first - before any headers in
> the include directory.
> 
> Let's change the scheme: make sure all headers that are not
> in the source directory are included through a path
> starting with qemu/ , thus:
> 
>  #include <>
> 
> headers in the same directory as source are included with
> 
>  #include ""
> 
> as per standard.
> 
> This (untested) patch is just to start the discussion and does not
> change all of the codebase. If there's agreement, this will be
> run on all code to converting code to this scheme.

Renaming files is always painful. If that's the fix, the cure might be
worse than the disease. As far as I know, the conflict is only
theoretical, so in that case I'd say: If it ain't broke, don't fix it.

Kevin

Re: [Qemu-devel] [PATCH v2] qemu: replace "" with <> in headers

[Michael S. Tsirkin]

On Wed, Mar 21, 2018 at 04:34:39PM +0100, Kevin Wolf wrote:
> Am 21.03.2018 um 15:46 hat Michael S. Tsirkin geschrieben:
> > Our current scheme is to use
> >  #include ""
> > for internal headers, and
> >  #include <>
> > for external ones.
> > 
> > Unfortunately this is not based on compiler support: from C point of
> > view, the "" form merely looks up headers in the current directory
> > and then falls back on <> directories.
> > 
> > Thus, for example, a system header trace.h - should it be present - will
> > conflict with our local trace.h
> 
> You're right that there is a conflict, even though only in one
> direction: "trace.h" is unambiguously the local trace.h in our source
> tree, but <trace.h> refers to the same local header rather than the
> system header as you would expect.
> 
> An easy way to resolve this conflict would be using -iquote rather than
> -I for directories in the source tree, so that <trace.h> unambiguously
> refers to the system header and "trace.h" unambiguously refers to the
> QEMU header.

I posted patches to that effect for 2.12. It's all still very much
a non-standard convention and so less robust than
prefixing file name with a project-specifix prefix.

> > As another example of problems, a header by the same name in the source
> > directory will always be picked up first - before any headers in
> > the include directory.
> > 
> > Let's change the scheme: make sure all headers that are not
> > in the source directory are included through a path
> > starting with qemu/ , thus:
> > 
> >  #include <>
> > 
> > headers in the same directory as source are included with
> > 
> >  #include ""
> > 
> > as per standard.
> > 
> > This (untested) patch is just to start the discussion and does not
> > change all of the codebase. If there's agreement, this will be
> > run on all code to converting code to this scheme.
> 
> Renaming files is always painful. If that's the fix, the cure might be
> worse than the disease. As far as I know, the conflict is only
> theoretical, so in that case I'd say: If it ain't broke, don't fix it.
> 
> Kevin

It's broke I think, it's very hard for new people to contribute to QEMU.
Look e.g. at rdma which all has messed up includes - and that's from an
experienced conributor who just isn't an experienced maintainer.

Amount of time spent on teaching new people trivia about our
conventions just isn't funny. They should be self-documenting
and violations should cause the build to fail.

-- 
MST

Re: [Qemu-devel] [PATCH v2] qemu: replace "" with <> in headers

[Kevin Wolf]

Am 21.03.2018 um 16:58 hat Michael S. Tsirkin geschrieben:
> On Wed, Mar 21, 2018 at 04:34:39PM +0100, Kevin Wolf wrote:
> > Am 21.03.2018 um 15:46 hat Michael S. Tsirkin geschrieben:
> > > Our current scheme is to use
> > >  #include ""
> > > for internal headers, and
> > >  #include <>
> > > for external ones.
> > > 
> > > Unfortunately this is not based on compiler support: from C point of
> > > view, the "" form merely looks up headers in the current directory
> > > and then falls back on <> directories.
> > > 
> > > Thus, for example, a system header trace.h - should it be present - will
> > > conflict with our local trace.h
> > 
> > You're right that there is a conflict, even though only in one
> > direction: "trace.h" is unambiguously the local trace.h in our source
> > tree, but <trace.h> refers to the same local header rather than the
> > system header as you would expect.
> > 
> > An easy way to resolve this conflict would be using -iquote rather than
> > -I for directories in the source tree, so that <trace.h> unambiguously
> > refers to the system header and "trace.h" unambiguously refers to the
> > QEMU header.
> 
> I posted patches to that effect for 2.12.

Ah, I missed those. That's good (and imho enough).

> It's all still very much a non-standard convention and so less robust
> than prefixing file name with a project-specifix prefix.

I've always had the impression that it's by far the most common
convention, to the point that I'd blindly assume it when joining a new
project.

> > > As another example of problems, a header by the same name in the source
> > > directory will always be picked up first - before any headers in
> > > the include directory.
> > > 
> > > Let's change the scheme: make sure all headers that are not
> > > in the source directory are included through a path
> > > starting with qemu/ , thus:
> > > 
> > >  #include <>
> > > 
> > > headers in the same directory as source are included with
> > > 
> > >  #include ""
> > > 
> > > as per standard.
> > > 
> > > This (untested) patch is just to start the discussion and does not
> > > change all of the codebase. If there's agreement, this will be
> > > run on all code to converting code to this scheme.
> > 
> > Renaming files is always painful. If that's the fix, the cure might be
> > worse than the disease. As far as I know, the conflict is only
> > theoretical, so in that case I'd say: If it ain't broke, don't fix it.
> > 
> > Kevin
> 
> It's broke I think, it's very hard for new people to contribute to QEMU.
> Look e.g. at rdma which all has messed up includes - and that's from an
> experienced conributor who just isn't an experienced maintainer.

I don't think the problem is that the convention is hard to apply (it's
definitely not). It's knowing about the convention. This problem isn't
going away by switching to a different, less common convention. We're
only going to see more offenders then.

> Amount of time spent on teaching new people trivia about our
> conventions just isn't funny. They should be self-documenting
> and violations should cause the build to fail.

Yes, but your proposal doesn't achieve this. You can still use
"qemu/foo.h" instead of <qemu/foo.h> and it will build successfully.
That's something we can't change, as far as I know, because the include
path for "foo.h" is always a superset of <foo.h>.

If anything, this means that we should prefer "foo.h" for local headers
(i.e. the way it currently is) because we can let the compiler enforce
it: <foo.h> for "foo.h" can become a build error, and does so with your
-iquote patch, but the other way round doesn't work.

Then it's only system headers that you can possibly get wrong, but for
those everyone should be used to using <foo.h> anyway.

Kevin

Re: [Qemu-devel] [PATCH v2] qemu: replace "" with <> in headers

[Michael S. Tsirkin]

On Wed, Mar 21, 2018 at 05:22:03PM +0100, Kevin Wolf wrote:
> > It's all still very much a non-standard convention and so less robust
> > than prefixing file name with a project-specifix prefix.
> 
> I've always had the impression that it's by far the most common
> convention, to the point that I'd blindly assume it when joining a new
> project.

Any examples?

> > > > As another example of problems, a header by the same name in the source
> > > > directory will always be picked up first - before any headers in
> > > > the include directory.
> > > > 
> > > > Let's change the scheme: make sure all headers that are not
> > > > in the source directory are included through a path
> > > > starting with qemu/ , thus:
> > > > 
> > > >  #include <>
> > > > 
> > > > headers in the same directory as source are included with
> > > > 
> > > >  #include ""
> > > > 
> > > > as per standard.
> > > > 
> > > > This (untested) patch is just to start the discussion and does not
> > > > change all of the codebase. If there's agreement, this will be
> > > > run on all code to converting code to this scheme.
> > > 
> > > Renaming files is always painful. If that's the fix, the cure might be
> > > worse than the disease. As far as I know, the conflict is only
> > > theoretical, so in that case I'd say: If it ain't broke, don't fix it.
> > > 
> > > Kevin
> > 
> > It's broke I think, it's very hard for new people to contribute to QEMU.
> > Look e.g. at rdma which all has messed up includes - and that's from an
> > experienced conributor who just isn't an experienced maintainer.
> 
> I don't think the problem is that the convention is hard to apply (it's
> definitely not). It's knowing about the convention. This problem isn't
> going away by switching to a different, less common convention. We're
> only going to see more offenders then.

Not if we have some automatic tools to catch violators.

> > Amount of time spent on teaching new people trivia about our
> > conventions just isn't funny. They should be self-documenting
> > and violations should cause the build to fail.
> 
> Yes, but your proposal doesn't achieve this. You can still use
> "qemu/foo.h" instead of <qemu/foo.h> and it will build successfully.
> That's something we can't change, as far as I know, because the include
> path for "foo.h" is always a superset of <foo.h>.

If the rule is that "" is only for files in the current directory
then we can easily code up a checkpatch script to catch violators.

> If anything, this means that we should prefer "foo.h" for local headers
> (i.e. the way it currently is) because we can let the compiler enforce
> it: <foo.h> for "foo.h" can become a build error, and does so with your
> -iquote patch, but the other way round doesn't work.
> 
> Then it's only system headers that you can possibly get wrong, but for
> those everyone should be used to using <foo.h> anyway.
> 
> Kevin

If my proposal to prefix all include directories with qemu/
is accepted, then we can solve the stale file problem
by prohibiting a directory named qemu everywhere in source.


-- 
MST

Re: [Qemu-devel] [PATCH v2] qemu: replace "" with <> in headers

[Paolo Bonzini]

On 22/03/2018 20:29, Michael S. Tsirkin wrote:
> On Wed, Mar 21, 2018 at 05:22:03PM +0100, Kevin Wolf wrote:
>>> It's all still very much a non-standard convention and so less robust
>>> than prefixing file name with a project-specifix prefix.
>> I've always had the impression that it's by far the most common
>> convention, to the point that I'd blindly assume it when joining a new
>> project.
> 
> Any examples?

GCC - https://github.com/gcc-mirror/gcc/blob/master/gcc/reload.c
Libvirt - https://github.com/libvirt/libvirt/blob/master/src/util/virprocess.c
SDL - https://github.com/SDL-mirror/SDL/blob/master/src/core/unix/SDL_poll.c

Anything but Linux really.

I find <qemu/foo/bar.h> verbose and unnecessary.  The only advantage
of your proposal is that files included from the source directory would be
clearly noticeable.  That said, it's easy to add a checkpatch.pl rule that
detects when ".../..." is used on a file not under include/.

Paolo