Message ID | 1466777957-5126-2-git-send-email-armbru@redhat.com |
---|---|
State | New |
Headers | show |
On 06/24/2016 08:19 AM, Markus Armbruster wrote: > Signed-off-by: Markus Armbruster <armbru@redhat.com> > Reviewed-by: Peter Maydell <peter.maydell@linaro.org> Worth mentioning how you found the culprits in the commit message, so that someone could repeat the task when backporting this patch or dealing with future cruft that inevitably creeps back in without an automated checkin validation tool? > +++ b/crypto/pbkdf-nettle.c > @@ -19,9 +19,9 @@ > */ > > #include "qemu/osdep.h" > +#include <nettle/pbkdf2.h> > #include "qapi/error.h" > #include "crypto/pbkdf.h" > -#include "nettle/pbkdf2.h" You're not just converting <> to "" (when the header is internal) or "" to <> (when the header is 3rd-party), but also rearranging things to put <> before "" (except for osdep.h which must be first). I like that paradigm, but again, might be worth a mention in the commit message as being intentional.
Eric Blake <eblake@redhat.com> writes: > On 06/24/2016 08:19 AM, Markus Armbruster wrote: >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> Reviewed-by: Peter Maydell <peter.maydell@linaro.org> > > Worth mentioning how you found the culprits in the commit message, so > that someone could repeat the task when backporting this patch or > dealing with future cruft that inevitably creeps back in without an > automated checkin validation tool? > > >> +++ b/crypto/pbkdf-nettle.c >> @@ -19,9 +19,9 @@ >> */ >> >> #include "qemu/osdep.h" >> +#include <nettle/pbkdf2.h> >> #include "qapi/error.h" >> #include "crypto/pbkdf.h" >> -#include "nettle/pbkdf2.h" > > You're not just converting <> to "" (when the header is internal) or "" > to <> (when the header is 3rd-party), but also rearranging things to put > <> before "" (except for osdep.h which must be first). I like that > paradigm, but again, might be worth a mention in the commit message as > being intentional. Our ordering of #include directives looks and smells like an open garbage dump in August. Cleaning this up should become feasible once we manage to make our headers self-contained. If such a cleanup is wanted.
Eric Blake <eblake@redhat.com> writes: > On 06/24/2016 08:19 AM, Markus Armbruster wrote: >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> Reviewed-by: Peter Maydell <peter.maydell@linaro.org> > > Worth mentioning how you found the culprits in the commit message, so > that someone could repeat the task when backporting this patch or > dealing with future cruft that inevitably creeps back in without an > automated checkin validation tool? With an ugly Perl script, of course %-/ If our use of -I was sane, the mapping from #include's file to the actual file would be straightforward. It isn't, because we add different -I for different parts of the code. To spice up things, we also name a few of our headers just like system headers we use elsewhere, e.g. "util.h" in net/ vs. <util.h> in util/qemu-openpty.c. My Perl script tries to match #include directives against source files, and prints its findings. The headers it can't map to source files must either be generated headers or system headers. The headers it can map might be system headers anyway. I'm afraid my script and its usage is too brittle to be of much use later on. I attach it anyway. [...]
On 27 June 2016 at 09:50, Markus Armbruster <armbru@redhat.com> wrote: > To spice up things, we > also name a few of our headers just like system headers we use > elsewhere, e.g. "util.h" in net/ vs. <util.h> in util/qemu-openpty.c. That would be nice to fix at some point I think, it's pretty brittle. (We had one issue recently that boiled down to system header vs local header confusion, though I forget the details.) thanks -- PMM
On 06/27/2016 02:50 AM, Markus Armbruster wrote: > With an ugly Perl script, of course %-/ > > I'm afraid my script and its usage is too brittle to be of much use > later on. I attach it anyway. A zero byte file map-include-to-header.pl~. Infinite compression :)
Eric Blake <eblake@redhat.com> writes: > On 06/27/2016 02:50 AM, Markus Armbruster wrote: > >> With an ugly Perl script, of course %-/ >> > >> I'm afraid my script and its usage is too brittle to be of much use >> later on. I attach it anyway. > > A zero byte file map-include-to-header.pl~. Infinite compression :) Looks like my fingers were even more reluctant to show off their ugly work than my brain was... #!/usr/bin/perl -w use strict; my %ihdr = (); my %idir = (); my @sall = (); my @sinc = (); open(my $fh, "-|", "git grep '^[ \t]*#[ \t]*include[ \t]'") or die "can't run git grep: $!"; while (<$fh>) { m,^([^:/]*)/?[^:/]*:[ \t]*#[ \t]*include[ \t]*(["<])([^">]*),; my $dir = $1 // "."; my $delim = $2; my $h = $3; $ihdr{$h} |= 1 << ($delim eq "<"); if (exists $idir{$h}) { my $aref = $idir{$h}; push @$aref, $dir unless grep($_ eq $dir, @$aref); } else { $idir{$h} = [$dir]; } } close ($fh); open($fh, "-|", "git ls-tree -r --name-only HEAD") or die "can't run git ls-tree: $!"; while (<$fh>) { chomp; push @sall, $_; } close ($fh); @sinc = grep(/^include\//, @sall); sub pr { my ($h, $fn, $src) = @_; print "$h -> $fn"; if ($ihdr{$h} == 3) { print " (included inconsistently)"; } elsif ($src) { print " (included with <>)" if ($ihdr{$h} != 1); } else { print " (included with \"\")" if ($ihdr{$h} != 2); } print "\n"; } for my $h (keys %ihdr) { $h =~ m,^(\.\./)*(include/)?(.*), or die; my $hh = $3; my @fn = grep(/^include\/\Q$hh\E$/, @sinc); if (@fn) { pr($h, $fn[0], 1); next; } @fn = grep(/^\Q$hh\E$/, @sall); if (@fn) { pr($h, $fn[0], 1); next; } for my $dir (@{$idir{$h}}) { next if $dir eq "."; @fn = grep(/^\Q$dir\/$hh\E$/, @sall); if (@fn) { pr($h, $fn[0], 1); } else { pr($h, "? (in $dir)", 0); } } }
Peter Maydell <peter.maydell@linaro.org> writes: > On 27 June 2016 at 09:50, Markus Armbruster <armbru@redhat.com> wrote: >> To spice up things, we >> also name a few of our headers just like system headers we use >> elsewhere, e.g. "util.h" in net/ vs. <util.h> in util/qemu-openpty.c. > > That would be nice to fix at some point I think, it's pretty brittle. > (We had one issue recently that boiled down to system header vs local > header confusion, though I forget the details.) Fixing collisions we're aware of is easy enough, just rename our header. Finding collisions is more interesting. Perhaps I can cook up something for make check-source.
diff --git a/block/iscsi.c b/block/iscsi.c index 7e78ade..74f084ca 100644 --- a/block/iscsi.c +++ b/block/iscsi.c @@ -46,7 +46,6 @@ #ifdef __linux__ #include <scsi/sg.h> -#include <block/scsi.h> #endif typedef struct IscsiLun { diff --git a/crypto/pbkdf-gcrypt.c b/crypto/pbkdf-gcrypt.c index 997b311..34af3a9 100644 --- a/crypto/pbkdf-gcrypt.c +++ b/crypto/pbkdf-gcrypt.c @@ -19,9 +19,9 @@ */ #include "qemu/osdep.h" +#include <gcrypt.h> #include "qapi/error.h" #include "crypto/pbkdf.h" -#include "gcrypt.h" bool qcrypto_pbkdf2_supports(QCryptoHashAlgorithm hash) { diff --git a/crypto/pbkdf-nettle.c b/crypto/pbkdf-nettle.c index db9fc15..d681a60 100644 --- a/crypto/pbkdf-nettle.c +++ b/crypto/pbkdf-nettle.c @@ -19,9 +19,9 @@ */ #include "qemu/osdep.h" +#include <nettle/pbkdf2.h> #include "qapi/error.h" #include "crypto/pbkdf.h" -#include "nettle/pbkdf2.h" bool qcrypto_pbkdf2_supports(QCryptoHashAlgorithm hash) diff --git a/exec.c b/exec.c index 0122ef7..011babd 100644 --- a/exec.c +++ b/exec.c @@ -36,7 +36,7 @@ #include "qemu/config-file.h" #include "qemu/error-report.h" #if defined(CONFIG_USER_ONLY) -#include <qemu.h> +#include "qemu.h" #else /* !CONFIG_USER_ONLY */ #include "hw/hw.h" #include "exec/memory.h" diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c index 1a49f1e..64c263f 100644 --- a/hw/scsi/virtio-scsi-dataplane.c +++ b/hw/scsi/virtio-scsi-dataplane.c @@ -16,7 +16,7 @@ #include "qemu/error-report.h" #include "sysemu/block-backend.h" #include <hw/scsi/scsi.h> -#include <block/scsi.h> +#include "block/scsi.h" #include <hw/virtio/virtio-bus.h> #include "hw/virtio/virtio-access.h" diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c index 71d09d3..a41f3e9 100644 --- a/hw/scsi/virtio-scsi.c +++ b/hw/scsi/virtio-scsi.c @@ -21,7 +21,7 @@ #include "qemu/iov.h" #include "sysemu/block-backend.h" #include <hw/scsi/scsi.h> -#include <block/scsi.h> +#include "block/scsi.h" #include <hw/virtio/virtio-bus.h> #include "hw/virtio/virtio-access.h" diff --git a/linux-user/flatload.c b/linux-user/flatload.c index 48ad1c5..42d1079 100644 --- a/linux-user/flatload.c +++ b/linux-user/flatload.c @@ -37,7 +37,7 @@ #include "qemu.h" #include "flat.h" -#include <target_flat.h> +#include "target_flat.h" //#define DEBUG diff --git a/monitor.c b/monitor.c index 6f960f1..6770c8c 100644 --- a/monitor.c +++ b/monitor.c @@ -62,7 +62,7 @@ #include "qapi/qmp/qjson.h" #include "qapi/qmp/json-streamer.h" #include "qapi/qmp/json-parser.h" -#include <qom/object_interfaces.h> +#include "qom/object_interfaces.h" #include "cpu.h" #include "trace.h" #include "trace/control.h" diff --git a/slirp/bootp.c b/slirp/bootp.c index 7b3232b..5a4646c 100644 --- a/slirp/bootp.c +++ b/slirp/bootp.c @@ -22,7 +22,7 @@ * THE SOFTWARE. */ #include "qemu/osdep.h" -#include <slirp.h> +#include "slirp.h" #if defined(_WIN32) /* Windows ntohl() returns an u_long value. diff --git a/slirp/cksum.c b/slirp/cksum.c index 2ad0e65..6d73abf 100644 --- a/slirp/cksum.c +++ b/slirp/cksum.c @@ -31,7 +31,7 @@ */ #include "qemu/osdep.h" -#include <slirp.h> +#include "slirp.h" /* * Checksum routine for Internet Protocol family headers (Portable Version). diff --git a/slirp/if.c b/slirp/if.c index 9b02180..51ae0d0 100644 --- a/slirp/if.c +++ b/slirp/if.c @@ -6,7 +6,7 @@ */ #include "qemu/osdep.h" -#include <slirp.h> +#include "slirp.h" #include "qemu/timer.h" static void diff --git a/slirp/ip_input.c b/slirp/ip_input.c index 34fba2b..348e1dc 100644 --- a/slirp/ip_input.c +++ b/slirp/ip_input.c @@ -39,7 +39,7 @@ */ #include "qemu/osdep.h" -#include <slirp.h> +#include "slirp.h" #include "ip_icmp.h" static struct ip *ip_reass(Slirp *slirp, struct ip *ip, struct ipq *fp); diff --git a/slirp/ip_output.c b/slirp/ip_output.c index 0d6b3b8..db403f0 100644 --- a/slirp/ip_output.c +++ b/slirp/ip_output.c @@ -39,7 +39,7 @@ */ #include "qemu/osdep.h" -#include <slirp.h> +#include "slirp.h" /* Number of packets queued before we start sending * (to prevent allocing too many mbufs) */ diff --git a/slirp/mbuf.c b/slirp/mbuf.c index d136988..7eddc21 100644 --- a/slirp/mbuf.c +++ b/slirp/mbuf.c @@ -16,7 +16,7 @@ */ #include "qemu/osdep.h" -#include <slirp.h> +#include "slirp.h" #define MBUF_THRESH 30 diff --git a/slirp/misc.c b/slirp/misc.c index 1a0ea1b..88e9d94 100644 --- a/slirp/misc.c +++ b/slirp/misc.c @@ -6,9 +6,8 @@ */ #include "qemu/osdep.h" -#include <slirp.h> -#include <libslirp.h> - +#include "slirp.h" +#include "libslirp.h" #include "monitor/monitor.h" #include "qemu/error-report.h" #include "qemu/main-loop.h" diff --git a/slirp/sbuf.c b/slirp/sbuf.c index dd4cb8c..10119d3 100644 --- a/slirp/sbuf.c +++ b/slirp/sbuf.c @@ -6,8 +6,8 @@ */ #include "qemu/osdep.h" -#include <slirp.h> -#include <qemu/main-loop.h> +#include "slirp.h" +#include "qemu/main-loop.h" static void sbappendsb(struct sbuf *sb, struct mbuf *m); diff --git a/slirp/socket.c b/slirp/socket.c index b336586..2b53cd7 100644 --- a/slirp/socket.c +++ b/slirp/socket.c @@ -7,7 +7,7 @@ #include "qemu/osdep.h" #include "qemu-common.h" -#include <slirp.h> +#include "slirp.h" #include "ip_icmp.h" #ifdef __sun__ #include <sys/filio.h> diff --git a/slirp/tcp_input.c b/slirp/tcp_input.c index e2b5d4e..c5063a9 100644 --- a/slirp/tcp_input.c +++ b/slirp/tcp_input.c @@ -39,7 +39,7 @@ */ #include "qemu/osdep.h" -#include <slirp.h> +#include "slirp.h" #include "ip_icmp.h" #define TCPREXMTTHRESH 3 diff --git a/slirp/tcp_output.c b/slirp/tcp_output.c index 99b0a9b..819db27 100644 --- a/slirp/tcp_output.c +++ b/slirp/tcp_output.c @@ -39,7 +39,7 @@ */ #include "qemu/osdep.h" -#include <slirp.h> +#include "slirp.h" static const u_char tcp_outflags[TCP_NSTATES] = { TH_RST|TH_ACK, 0, TH_SYN, TH_SYN|TH_ACK, diff --git a/slirp/tcp_subr.c b/slirp/tcp_subr.c index 6b9fef2..ed16e18 100644 --- a/slirp/tcp_subr.c +++ b/slirp/tcp_subr.c @@ -39,7 +39,7 @@ */ #include "qemu/osdep.h" -#include <slirp.h> +#include "slirp.h" /* patchable/settable parameters for tcp */ /* Don't do rfc1323 performance enhancements */ diff --git a/slirp/tcp_timer.c b/slirp/tcp_timer.c index 8f5dd77..f9060c7 100644 --- a/slirp/tcp_timer.c +++ b/slirp/tcp_timer.c @@ -31,7 +31,7 @@ */ #include "qemu/osdep.h" -#include <slirp.h> +#include "slirp.h" static struct tcpcb *tcp_timers(register struct tcpcb *tp, int timer); diff --git a/slirp/tftp.c b/slirp/tftp.c index 12b5ff6..8de3bc0 100644 --- a/slirp/tftp.c +++ b/slirp/tftp.c @@ -23,7 +23,7 @@ */ #include "qemu/osdep.h" -#include <slirp.h> +#include "slirp.h" #include "qemu-common.h" #include "qemu/cutils.h" diff --git a/slirp/udp.c b/slirp/udp.c index 247024f..93d7224 100644 --- a/slirp/udp.c +++ b/slirp/udp.c @@ -39,7 +39,7 @@ */ #include "qemu/osdep.h" -#include <slirp.h> +#include "slirp.h" #include "ip_icmp.h" static uint8_t udp_tos(struct socket *so); diff --git a/target-arm/arm-powerctl.c b/target-arm/arm-powerctl.c index d452230..6519d52 100644 --- a/target-arm/arm-powerctl.c +++ b/target-arm/arm-powerctl.c @@ -9,8 +9,8 @@ */ #include "qemu/osdep.h" -#include <cpu.h> -#include <cpu-qom.h> +#include "cpu.h" +#include "cpu-qom.h" #include "internals.h" #include "arm-powerctl.h" #include "qemu/log.h" diff --git a/target-arm/psci.c b/target-arm/psci.c index 4db9b8c..14316eb 100644 --- a/target-arm/psci.c +++ b/target-arm/psci.c @@ -16,10 +16,10 @@ * along with this program; if not, see <http://www.gnu.org/licenses/>. */ #include "qemu/osdep.h" -#include <cpu.h> -#include <exec/helper-proto.h> -#include <kvm-consts.h> -#include <sysemu/sysemu.h> +#include "cpu.h" +#include "exec/helper-proto.h" +#include "kvm-consts.h" +#include "sysemu/sysemu.h" #include "internals.h" #include "arm-powerctl.h" #include "exec/exec-all.h" diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c index ca894ff..49c0728 100644 --- a/target-ppc/translate_init.c +++ b/target-ppc/translate_init.c @@ -21,7 +21,7 @@ #include "qemu/osdep.h" #include "disas/bfd.h" #include "exec/gdbstub.h" -#include <sysemu/kvm.h> +#include "sysemu/kvm.h" #include "kvm_ppc.h" #include "sysemu/arch_init.h" #include "sysemu/cpus.h" diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h index bd6b2e5..c7cc4e1 100644 --- a/target-s390x/cpu.h +++ b/target-s390x/cpu.h @@ -219,7 +219,7 @@ int s390_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg); void s390_cpu_gdb_init(CPUState *cs); void s390x_cpu_debug_excp_handler(CPUState *cs); -#include <sysemu/kvm.h> +#include "sysemu/kvm.h" /* distinguish between 24 bit and 31 bit addressing */ #define HIGH_ORDER_BIT 0x80000000 diff --git a/target-unicore32/softmmu.c b/target-unicore32/softmmu.c index a34026a..e7152e7 100644 --- a/target-unicore32/softmmu.c +++ b/target-unicore32/softmmu.c @@ -13,7 +13,7 @@ #endif #include "qemu/osdep.h" -#include <cpu.h> +#include "cpu.h" #include "exec/exec-all.h" #undef DEBUG_UC32 diff --git a/tests/postcopy-test.c b/tests/postcopy-test.c index 35d5180..16465ab 100644 --- a/tests/postcopy-test.c +++ b/tests/postcopy-test.c @@ -15,11 +15,10 @@ #include "libqtest.h" #include "qemu/option.h" #include "qemu/range.h" +#include "qemu/sockets.h" #include "sysemu/char.h" #include "sysemu/sysemu.h" -#include <qemu/sockets.h> - const unsigned start_address = 1024 * 1024; const unsigned end_address = 100 * 1024 * 1024; bool got_stop; diff --git a/tests/vhost-user-bridge.c b/tests/vhost-user-bridge.c index 45fa2b6..775e031 100644 --- a/tests/vhost-user-bridge.c +++ b/tests/vhost-user-bridge.c @@ -36,8 +36,6 @@ #include <sys/eventfd.h> #include <arpa/inet.h> #include <netdb.h> -#include <qemu/osdep.h> - #include <linux/vhost.h> #include "qemu/atomic.h" diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c index 8b2164b..71e7f42 100644 --- a/tests/vhost-user-test.c +++ b/tests/vhost-user-test.c @@ -13,12 +13,12 @@ #include "libqtest.h" #include "qemu/option.h" #include "qemu/range.h" +#include "qemu/sockets.h" #include "sysemu/char.h" #include "sysemu/sysemu.h" #include <linux/vhost.h> #include <sys/vfs.h> -#include <qemu/sockets.h> /* GLIB version compatibility flags */ #if !GLIB_CHECK_VERSION(2, 26, 0) diff --git a/util/mmap-alloc.c b/util/mmap-alloc.c index 629d97a..5a85aa3 100644 --- a/util/mmap-alloc.c +++ b/util/mmap-alloc.c @@ -9,8 +9,9 @@ * This work is licensed under the terms of the GNU GPL, version 2 or * later. See the COPYING file in the top-level directory. */ + #include "qemu/osdep.h" -#include <qemu/mmap-alloc.h> +#include "qemu/mmap-alloc.h" #define HUGETLBFS_MAGIC 0x958458f6 diff --git a/util/oslib-posix.c b/util/oslib-posix.c index e2e1d4d..d8e5dcf 100644 --- a/util/oslib-posix.c +++ b/util/oslib-posix.c @@ -48,7 +48,7 @@ #include <sys/sysctl.h> #endif -#include <qemu/mmap-alloc.h> +#include "qemu/mmap-alloc.h" int qemu_get_thread_id(void) {