[libvirt] [PATCH 00/10 v2] Cleanup: About header including
The public headers libvirt/libvirt.h, libvirt/virterror.h, libvirt/libvirt-qemu.h, and libvirt/libvirt-lxc.h shouldn't be included in internal source directly. Osier Yang (10): cleanup: Remove the duplicate header cleanup: Only include testutils.h once cleanup: Don't include libvirt/libvirt.h cleanup: Don't include libvirt/virterror.h syntax-check: Don't include duplicate header syntax-check: Don't include "libvirt.h" and "virterror.h" in "" form syntax-check: Include libvirt.h and virterror.h in <> form only in external tools syntax-check: Don't include libvirt-{qemu,lxc}.h internally syntax-check: Include libvirt-{qemu,lxc}.h in <> form only in external tools docs: Update hacking cfg.mk | 106 ++++++++++++++++++++++++++++++++++++++ daemon/libvirtd.c | 1 - daemon/remote.c | 2 - docs/hacking.html.in | 8 +-- include/libvirt/libvirt-lxc.h | 2 +- include/libvirt/libvirt-qemu.h | 2 +- python/libvirt-lxc-override.c | 4 +- python/libvirt-override.c | 4 +- python/libvirt-qemu-override.c | 4 +- python/typewrappers.h | 4 +- src/conf/node_device_conf.c | 1 - src/libvirt-qemu.c | 1 - src/network/bridge_driver.c | 1 - src/nodeinfo.h | 1 - src/openvz/openvz_conf.c | 1 - src/openvz/openvz_driver.c | 1 - src/parallels/parallels_driver.c | 1 - src/phyp/phyp_driver.c | 2 - src/qemu/qemu_capabilities.h | 1 - src/qemu/qemu_driver.c | 1 - src/remote/remote_driver.h | 2 - src/remote/remote_protocol.x | 1 - src/security/security_selinux.c | 1 - src/uml/uml_driver.c | 1 - src/util/virkeycode.h | 1 - src/util/virnetdevtap.c | 1 - src/xen/xen_hypervisor.c | 1 - tests/domainsnapshotxml2xmltest.c | 4 +- tests/esxutilstest.c | 4 +- tests/lxcxml2xmltest.c | 4 +- tests/openvzutilstest.c | 4 +- tests/qemuargv2xmltest.c | 4 +- tests/qemuhelptest.c | 4 +- tests/qemumonitortest.c | 4 +- tests/qemuxml2argvtest.c | 4 +- tests/qemuxml2xmltest.c | 4 +- tests/qemuxmlnstest.c | 4 +- tests/shunloadhelper.c | 2 - tests/shunloadtest.c | 4 +- tests/vmx2xmltest.c | 4 +- tests/xml2vmxtest.c | 4 +- tools/virsh.c | 4 +- 42 files changed, 148 insertions(+), 66 deletions(-) -- 1.8.1.4
Detected by a simple Shell script: for i in $(find -type f -name "*.[ch]" | grep -v gnulib); do awk 'BEGIN { FS=" " fail=0 } /^# *include.*\.h[">]$/{ arr[$NF]++ } END { for (key in arr) { if (arr[key] > 1) { fail=1 printf("%d %s\n", arr[key], key) } } if (fail == 1) exit 1 }' $i if test $? != 0; then echo "Duplicate header(s) in $i" fi done; A later patch will add the syntax-check to avoid duplicate headers. --- daemon/libvirtd.c | 1 - src/conf/node_device_conf.c | 1 - src/network/bridge_driver.c | 1 - src/openvz/openvz_conf.c | 1 - src/openvz/openvz_driver.c | 1 - src/parallels/parallels_driver.c | 1 - src/phyp/phyp_driver.c | 1 - src/qemu/qemu_capabilities.h | 1 - src/qemu/qemu_driver.c | 1 - src/security/security_selinux.c | 1 - src/uml/uml_driver.c | 1 - src/util/virnetdevtap.c | 1 - src/xen/xen_hypervisor.c | 1 - 13 files changed, 13 deletions(-) diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 34a8737..38b7346 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -51,7 +51,6 @@ #include "virnetlink.h" #include "virnetserver.h" #include "remote.h" -#include "remote_driver.h" #include "virhook.h" #include "viraudit.h" #include "locking/lock_manager.h" diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index b4d8cb3..8fe4e03 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -31,7 +31,6 @@ #include "viralloc.h" #include "node_device_conf.h" -#include "viralloc.h" #include "virxml.h" #include "virutil.h" #include "virbuffer.h" diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index e8b314a..3b27980 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -39,7 +39,6 @@ #include <signal.h> #include <paths.h> #include <pwd.h> -#include <stdio.h> #include <sys/wait.h> #include <sys/ioctl.h> #include <net/if.h> diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c index 4b21c4e..a6f96c7 100644 --- a/src/openvz/openvz_conf.c +++ b/src/openvz/openvz_conf.c @@ -36,7 +36,6 @@ #include <dirent.h> #include <time.h> #include <sys/stat.h> -#include <unistd.h> #include <limits.h> #include <errno.h> #include <string.h> diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index 9b10624..5d22c63 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -41,7 +41,6 @@ #include <fcntl.h> #include <paths.h> #include <pwd.h> -#include <stdio.h> #include <sys/wait.h> #include "virerror.h" diff --git a/src/parallels/parallels_driver.c b/src/parallels/parallels_driver.c index 5b8d85f..0c0bfcb 100644 --- a/src/parallels/parallels_driver.c +++ b/src/parallels/parallels_driver.c @@ -35,7 +35,6 @@ #include <fcntl.h> #include <paths.h> #include <pwd.h> -#include <stdio.h> #include <sys/wait.h> #include <sys/time.h> #include <sys/statvfs.h> diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index 50e8216..3057345 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -33,7 +33,6 @@ #include <stdlib.h> #include <unistd.h> #include <errno.h> -#include <stdio.h> #include <libssh2.h> #include <netinet/in.h> #include <arpa/inet.h> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 7a0f191..d903f6f 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -27,7 +27,6 @@ # include "virobject.h" # include "capabilities.h" # include "vircommand.h" -# include "virobject.h" # include "qemu_monitor.h" /* Internal flags to keep track of qemu command line capabilities */ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 7c167b7..cee5557 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -38,7 +38,6 @@ #include <fcntl.h> #include <signal.h> #include <paths.h> -#include <stdio.h> #include <sys/wait.h> #include <sys/ioctl.h> #include <sys/un.h> diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 7333a1f..c620a2e 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -43,7 +43,6 @@ #include "virfile.h" #include "virhash.h" #include "virrandom.h" -#include "virutil.h" #include "virconf.h" #include "virtpm.h" diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 4fc2315..498d1d9 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -38,7 +38,6 @@ #include <signal.h> #include <paths.h> #include <pwd.h> -#include <stdio.h> #include <sys/wait.h> #include <sys/ioctl.h> #include <sys/inotify.h> diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c index 871376e..e4ce223 100644 --- a/src/util/virnetdevtap.c +++ b/src/util/virnetdevtap.c @@ -29,7 +29,6 @@ #include "virnetdevopenvswitch.h" #include "virerror.h" #include "virfile.h" -#include "virerror.h" #include "viralloc.h" #include "virlog.h" #include "virutil.h" diff --git a/src/xen/xen_hypervisor.c b/src/xen/xen_hypervisor.c index e16fffe..9dbbe07 100644 --- a/src/xen/xen_hypervisor.c +++ b/src/xen/xen_hypervisor.c @@ -33,7 +33,6 @@ #include <sys/mman.h> #include <sys/ioctl.h> #include <limits.h> -#include <stdint.h> #include <regex.h> #include <errno.h> -- 1.8.1.4
On 04/16/2013 07:41 AM, Osier Yang wrote:
Detected by a simple Shell script:
for i in $(find -type f -name "*.[ch]" | grep -v gnulib); do
You can limit things to version-controlled files a bit faster with: for i in $(git ls-files -- '*.[ch]'); do
awk 'BEGIN { FS=" " fail=0 } /^# *include.*\.h[">]$/{
This pattern misses files that include a header with a comment saying why, such as this one: src/util/virxml.c:#include <math.h> /* for isnan() */
arr[$NF]++
This doesn't catch duplication between "" and <> in the same file, since it includes the delimiter as part of the key name. Slightly more powerful is: /# *include/{ match($0, /["<][^">]*[">]/) arr[substr($0, RSTART+1, RLENGTH-2)]++ } which uses the array name of just the filename regardless of delimiters.
} END { for (key in arr) { if (arr[key] > 1) { fail=1 printf("%d %s\n", arr[key], key) } } if (fail == 1) exit 1 }' $i
if test $? != 0; then echo "Duplicate header(s) in $i" fi done;
A later patch will add the syntax-check to avoid duplicate headers. ---
What you have works, but with my improvements to the script, I still see this after your patch: 2 testutils.h Duplicate header(s) in ./tests/qemumonitortest.c 2 testutils.h Duplicate header(s) in ./tests/qemuxmlnstest.c 2 testutils.h Duplicate header(s) in ./tests/qemuxml2argvtest.c 2 testutils.h Duplicate header(s) in ./tests/qemuargv2xmltest.c 2 testutils.h Duplicate header(s) in ./tests/shunloadtest.c 2 testutils.h Duplicate header(s) in ./tests/vmx2xmltest.c 2 testutils.h Duplicate header(s) in ./tests/domainsnapshotxml2xmltest.c 2 testutils.h Duplicate header(s) in ./tests/qemuhelptest.c 2 testutils.h Duplicate header(s) in ./tests/lxcxml2xmltest.c 2 testutils.h Duplicate header(s) in ./tests/xml2vmxtest.c 2 testutils.h Duplicate header(s) in ./tests/qemuxml2xmltest.c 2 testutils.h Duplicate header(s) in ./tests/esxutilstest.c 2 testutils.h Duplicate header(s) in ./tests/openvzutilstest.c 2 libxl.h Duplicate header(s) in ./src/libxl/libxl_driver.c testutils.h is cleaned up in patch 2; but the duplicate libxl.h in libxl_driver.c is still buggy. ACK with libxl_driver.c also fixed (you can touch up the commit message if you like, but that's not required; as long as we later get the syntax check right). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 17/04/13 10:50, Eric Blake wrote:
On 04/16/2013 07:41 AM, Osier Yang wrote:
Detected by a simple Shell script:
for i in $(find -type f -name "*.[ch]" | grep -v gnulib); do You can limit things to version-controlled files a bit faster with:
for i in $(git ls-files -- '*.[ch]'); do
Nice.
awk 'BEGIN { FS=" " fail=0 } /^# *include.*\.h[">]$/{
This pattern misses files that include a header with a comment saying why, such as this one:
src/util/virxml.c:#include <math.h> /* for isnan() */
arr[$NF]++
This doesn't catch duplication between "" and <> in the same file, since it includes the delimiter as part of the key name. Slightly more powerful is:
/# *include/{ match($0, /["<][^">]*[">]/) arr[substr($0, RSTART+1, RLENGTH-2)]++ }
Nice too. But it detects: 2 "virsh-edit.c" are included Duplicate header(s) in virsh-domain.c I changed it a bit to avoid it: /# *include.*\.h/{ match($0, /["<][^">]*[">]/) arr[substr($0, RSTART+1, RLENGTH-2)]++ } Osier
No reason to include it in both "if" and "else" branches. --- tests/domainsnapshotxml2xmltest.c | 4 ++-- tests/esxutilstest.c | 4 ++-- tests/lxcxml2xmltest.c | 4 ++-- tests/openvzutilstest.c | 4 ++-- tests/qemuargv2xmltest.c | 4 ++-- tests/qemuhelptest.c | 4 ++-- tests/qemumonitortest.c | 4 ++-- tests/qemuxml2argvtest.c | 4 ++-- tests/qemuxml2xmltest.c | 4 ++-- tests/qemuxmlnstest.c | 4 ++-- tests/shunloadtest.c | 4 ++-- tests/vmx2xmltest.c | 4 ++-- tests/xml2vmxtest.c | 4 ++-- 13 files changed, 26 insertions(+), 26 deletions(-) diff --git a/tests/domainsnapshotxml2xmltest.c b/tests/domainsnapshotxml2xmltest.c index 6f1c600..694c382 100644 --- a/tests/domainsnapshotxml2xmltest.c +++ b/tests/domainsnapshotxml2xmltest.c @@ -8,10 +8,11 @@ #include <sys/types.h> #include <fcntl.h> +#include "testutils.h" + #ifdef WITH_QEMU # include "internal.h" -# include "testutils.h" # include "qemu/qemu_conf.h" # include "qemu/qemu_domain.h" # include "testutilsqemu.h" @@ -125,7 +126,6 @@ mymain(void) VIRT_TEST_MAIN(mymain) #else -# include "testutils.h" int main(void) diff --git a/tests/esxutilstest.c b/tests/esxutilstest.c index 55b3623..e2baea2 100644 --- a/tests/esxutilstest.c +++ b/tests/esxutilstest.c @@ -1,5 +1,7 @@ #include <config.h> +#include "testutils.h" + #ifdef WITH_ESX # include <stdio.h> @@ -8,7 +10,6 @@ # include "internal.h" # include "viralloc.h" -# include "testutils.h" # include "virutil.h" # include "vmx/vmx.h" # include "esx/esx_util.h" @@ -279,7 +280,6 @@ mymain(void) VIRT_TEST_MAIN(mymain) #else -# include "testutils.h" int main(void) { diff --git a/tests/lxcxml2xmltest.c b/tests/lxcxml2xmltest.c index e0f99f4..4870f42 100644 --- a/tests/lxcxml2xmltest.c +++ b/tests/lxcxml2xmltest.c @@ -8,10 +8,11 @@ #include <sys/types.h> #include <fcntl.h> +#include "testutils.h" + #ifdef WITH_LXC # include "internal.h" -# include "testutils.h" # include "lxc/lxc_conf.h" # include "testutilslxc.h" @@ -136,7 +137,6 @@ mymain(void) VIRT_TEST_MAIN(mymain) #else -# include "testutils.h" int main(void) diff --git a/tests/openvzutilstest.c b/tests/openvzutilstest.c index 9fb7178..616b6be 100644 --- a/tests/openvzutilstest.c +++ b/tests/openvzutilstest.c @@ -1,5 +1,7 @@ #include <config.h> +#include "testutils.h" + #ifdef WITH_OPENVZ # include <stdio.h> @@ -8,7 +10,6 @@ # include "internal.h" # include "viralloc.h" -# include "testutils.h" # include "virutil.h" # include "openvz/openvz_conf.h" @@ -160,7 +161,6 @@ mymain(void) VIRT_TEST_MAIN(mymain) #else -# include "testutils.h" int main(void) { diff --git a/tests/qemuargv2xmltest.c b/tests/qemuargv2xmltest.c index 9167d88..ee6c7a9 100644 --- a/tests/qemuargv2xmltest.c +++ b/tests/qemuargv2xmltest.c @@ -8,10 +8,11 @@ #include <sys/types.h> #include <fcntl.h> +#include "testutils.h" + #ifdef WITH_QEMU # include "internal.h" -# include "testutils.h" # include "qemu/qemu_command.h" # include "testutilsqemu.h" @@ -260,7 +261,6 @@ mymain(void) VIRT_TEST_MAIN(mymain) #else -# include "testutils.h" int main(void) diff --git a/tests/qemuhelptest.c b/tests/qemuhelptest.c index 43774f4..7290183 100644 --- a/tests/qemuhelptest.c +++ b/tests/qemuhelptest.c @@ -1,11 +1,12 @@ #include <config.h> +#include "testutils.h" + #ifdef WITH_QEMU # include <stdio.h> # include <stdlib.h> -# include "testutils.h" # include "qemu/qemu_capabilities.h" # include "viralloc.h" @@ -1034,7 +1035,6 @@ mymain(void) VIRT_TEST_MAIN(mymain) #else -# include "testutils.h" int main(void) { diff --git a/tests/qemumonitortest.c b/tests/qemumonitortest.c index 285dfa8..a8fc57a 100644 --- a/tests/qemumonitortest.c +++ b/tests/qemumonitortest.c @@ -5,11 +5,12 @@ #include <string.h> #include <unistd.h> +#include "testutils.h" + #ifdef WITH_QEMU # include "internal.h" # include "viralloc.h" -# include "testutils.h" # include "virutil.h" # include "qemu/qemu_monitor.h" @@ -108,7 +109,6 @@ mymain(void) VIRT_TEST_MAIN(mymain) #else -# include "testutils.h" int main(void) { diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index be6c759..4bf13f0 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -8,10 +8,11 @@ #include <sys/types.h> #include <fcntl.h> +#include "testutils.h" + #ifdef WITH_QEMU # include "internal.h" -# include "testutils.h" # include "viralloc.h" # include "qemu/qemu_capabilities.h" # include "qemu/qemu_command.h" @@ -951,7 +952,6 @@ mymain(void) VIRT_TEST_MAIN(mymain) #else -# include "testutils.h" int main(void) { diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 3a1683f..7434190 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -8,10 +8,11 @@ #include <sys/types.h> #include <fcntl.h> +#include "testutils.h" + #ifdef WITH_QEMU # include "internal.h" -# include "testutils.h" # include "qemu/qemu_conf.h" # include "qemu/qemu_domain.h" # include "testutilsqemu.h" @@ -285,7 +286,6 @@ mymain(void) VIRT_TEST_MAIN(mymain) #else -# include "testutils.h" int main(void) diff --git a/tests/qemuxmlnstest.c b/tests/qemuxmlnstest.c index e6b42dc..4e9144b 100644 --- a/tests/qemuxmlnstest.c +++ b/tests/qemuxmlnstest.c @@ -8,10 +8,11 @@ #include <sys/types.h> #include <fcntl.h> +#include "testutils.h" + #ifdef WITH_QEMU # include "internal.h" -# include "testutils.h" # include "qemu/qemu_capabilities.h" # include "qemu/qemu_command.h" # include "qemu/qemu_domain.h" @@ -263,7 +264,6 @@ mymain(void) VIRT_TEST_MAIN(mymain) #else -# include "testutils.h" int main(void) { diff --git a/tests/shunloadtest.c b/tests/shunloadtest.c index eddb818..8271b93 100644 --- a/tests/shunloadtest.c +++ b/tests/shunloadtest.c @@ -40,6 +40,8 @@ #include <config.h> +#include "testutils.h" + #ifdef linux # include <dlfcn.h> @@ -50,7 +52,6 @@ # include <signal.h> # include "internal.h" -# include "testutils.h" pthread_cond_t cond = PTHREAD_COND_INITIALIZER; pthread_mutex_t lock = PTHREAD_MUTEX_INITIALIZER; @@ -143,7 +144,6 @@ int main(int argc ATTRIBUTE_UNUSED, char **argv) } #else -# include "testutils.h" int main(void) { diff --git a/tests/vmx2xmltest.c b/tests/vmx2xmltest.c index 2d04da7..0003f5f 100644 --- a/tests/vmx2xmltest.c +++ b/tests/vmx2xmltest.c @@ -1,5 +1,7 @@ #include <config.h> +#include "testutils.h" + #ifdef WITH_VMX # include <stdio.h> @@ -8,7 +10,6 @@ # include "internal.h" # include "viralloc.h" -# include "testutils.h" # include "vmx/vmx.h" static virCapsPtr caps; @@ -298,7 +299,6 @@ mymain(void) VIRT_TEST_MAIN(mymain) #else -# include "testutils.h" int main(void) { diff --git a/tests/xml2vmxtest.c b/tests/xml2vmxtest.c index 1957659..fa4f6f9 100644 --- a/tests/xml2vmxtest.c +++ b/tests/xml2vmxtest.c @@ -1,5 +1,7 @@ #include <config.h> +#include "testutils.h" + #ifdef WITH_VMX # include <stdio.h> @@ -8,7 +10,6 @@ # include "internal.h" # include "viralloc.h" -# include "testutils.h" # include "vmx/vmx.h" static virCapsPtr caps; @@ -311,7 +312,6 @@ mymain(void) VIRT_TEST_MAIN(mymain) #else -# include "testutils.h" int main(void) { -- 1.8.1.4
On 04/16/2013 07:41 AM, Osier Yang wrote:
No reason to include it in both "if" and "else" branches. --- tests/domainsnapshotxml2xmltest.c | 4 ++-- tests/esxutilstest.c | 4 ++-- tests/lxcxml2xmltest.c | 4 ++-- tests/openvzutilstest.c | 4 ++-- tests/qemuargv2xmltest.c | 4 ++-- tests/qemuhelptest.c | 4 ++-- tests/qemumonitortest.c | 4 ++-- tests/qemuxml2argvtest.c | 4 ++-- tests/qemuxml2xmltest.c | 4 ++-- tests/qemuxmlnstest.c | 4 ++-- tests/shunloadtest.c | 4 ++-- tests/vmx2xmltest.c | 4 ++-- tests/xml2vmxtest.c | 4 ++-- 13 files changed, 26 insertions(+), 26 deletions(-)
ACK. If you rebase this one before 1/10, then the script in 1/10 (after my touchups and a final cleanup of libxl) has no hits. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Which is already included by "internal.h", later patch will add syntax-check to avoid it. --- src/nodeinfo.h | 1 - src/phyp/phyp_driver.c | 1 - src/remote/remote_protocol.x | 1 - src/util/virkeycode.h | 1 - 4 files changed, 4 deletions(-) diff --git a/src/nodeinfo.h b/src/nodeinfo.h index 350f3c3..b0e5fbc 100644 --- a/src/nodeinfo.h +++ b/src/nodeinfo.h @@ -24,7 +24,6 @@ #ifndef __VIR_NODEINFO_H__ # define __VIR_NODEINFO_H__ -# include "libvirt/libvirt.h" # include "capabilities.h" int nodeGetInfo(virConnectPtr conn, virNodeInfoPtr nodeinfo); diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index 3057345..d6d9b0c 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -49,7 +49,6 @@ #include "viralloc.h" #include "virlog.h" #include "driver.h" -#include "libvirt/libvirt.h" #include "virerror.h" #include "viruuid.h" #include "domain_conf.h" diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index b957b8e..d384225 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -36,7 +36,6 @@ * 'REMOTE_'. This makes names quite long. */ -%#include <libvirt/libvirt.h> %#include "internal.h" %#include <arpa/inet.h> diff --git a/src/util/virkeycode.h b/src/util/virkeycode.h index a2e1391..6947cfe 100644 --- a/src/util/virkeycode.h +++ b/src/util/virkeycode.h @@ -23,7 +23,6 @@ # define __VIR_UTIL_VIRTKEYCODE_H__ # include "virutil.h" -# include "libvirt/libvirt.h" VIR_ENUM_DECL(virKeycodeSet); int virKeycodeValueFromString(virKeycodeSet codeset, const char *keyname); -- 1.8.1.4
On 04/16/2013 07:41 AM, Osier Yang wrote:
Which is already included by "internal.h", later patch will add syntax-check to avoid it. ---
ACK -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 04/16/2013 07:41 AM, Osier Yang wrote:
Which is already included by "internal.h", later patch will add syntax-check to avoid it. ---
+++ b/src/nodeinfo.h @@ -24,7 +24,6 @@ #ifndef __VIR_NODEINFO_H__ # define __VIR_NODEINFO_H__
-# include "libvirt/libvirt.h" # include "capabilities.h"
I took a second look at this after 4/10; thankfully, here you are still self-contained, via indirection, since capabilities.h includes internal.h.
int nodeGetInfo(virConnectPtr conn, virNodeInfoPtr nodeinfo); diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index 3057345..d6d9b0c 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -49,7 +49,6 @@ #include "viralloc.h" #include "virlog.h" #include "driver.h" -#include "libvirt/libvirt.h"
This is clean (internal.h is several lines earlier)
#include "virerror.h" #include "viruuid.h" #include "domain_conf.h" diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index b957b8e..d384225 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -36,7 +36,6 @@ * 'REMOTE_'. This makes names quite long. */
-%#include <libvirt/libvirt.h> %#include "internal.h"
This is clean.
%#include <arpa/inet.h>
diff --git a/src/util/virkeycode.h b/src/util/virkeycode.h index a2e1391..6947cfe 100644 --- a/src/util/virkeycode.h +++ b/src/util/virkeycode.h @@ -23,7 +23,6 @@ # define __VIR_UTIL_VIRTKEYCODE_H__
# include "virutil.h" -# include "libvirt/libvirt.h"
This relies on indirect inclusion via virutil.h. We've used indirect inclusion before (in fact, internal.h lets us indirectly include common headers like <errno.h> instead of repeating it everywhere), so that's fine (I know some other projects like all .h files to be self-contained without indirect inclusion, but that's a style choice; I'm not proposing that libvirt adopt it). So my ACK still stands. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Tue, Apr 16, 2013 at 09:41:45PM +0800, Osier Yang wrote:
Which is already included by "internal.h", later patch will add syntax-check to avoid it. --- src/nodeinfo.h | 1 - src/phyp/phyp_driver.c | 1 - src/remote/remote_protocol.x | 1 - src/util/virkeycode.h | 1 - 4 files changed, 4 deletions(-)
diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index b957b8e..d384225 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -36,7 +36,6 @@ * 'REMOTE_'. This makes names quite long. */
-%#include <libvirt/libvirt.h> %#include "internal.h" %#include <arpa/inet.h>
This has broken make check GEN remote_protocol-struct --- remote_protocol-structs 2013-04-11 18:02:02.445662659 +0100 +++ remote_protocol-struct-t3 2013-04-17 11:57:29.472083827 +0100 @@ -7,6 +7,7 @@ VIR_TYPED_PARAM_DOUBLE = 5, VIR_TYPED_PARAM_BOOLEAN = 6, VIR_TYPED_PARAM_STRING = 7, + VIR_TYPED_PARAM_LAST = 8, }; struct remote_nonnull_domain { remote_nonnull_string name; make[2]: *** [remote_protocol-struct] Error 1 The reason is that previously the remote driver would not see the sentinel values from enums, but now it does. 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 :|
On 17/04/13 18:58, Daniel P. Berrange wrote:
On Tue, Apr 16, 2013 at 09:41:45PM +0800, Osier Yang wrote:
Which is already included by "internal.h", later patch will add syntax-check to avoid it. --- src/nodeinfo.h | 1 - src/phyp/phyp_driver.c | 1 - src/remote/remote_protocol.x | 1 - src/util/virkeycode.h | 1 - 4 files changed, 4 deletions(-) diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index b957b8e..d384225 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -36,7 +36,6 @@ * 'REMOTE_'. This makes names quite long. */
-%#include <libvirt/libvirt.h> %#include "internal.h" %#include <arpa/inet.h>
This has broken make check
GEN remote_protocol-struct --- remote_protocol-structs 2013-04-11 18:02:02.445662659 +0100 +++ remote_protocol-struct-t3 2013-04-17 11:57:29.472083827 +0100 @@ -7,6 +7,7 @@ VIR_TYPED_PARAM_DOUBLE = 5, VIR_TYPED_PARAM_BOOLEAN = 6, VIR_TYPED_PARAM_STRING = 7, + VIR_TYPED_PARAM_LAST = 8, }; struct remote_nonnull_domain { remote_nonnull_string name; make[2]: *** [remote_protocol-struct] Error 1
I'm wondering why it works on my FC18. Osier
On 04/17/2013 06:38 AM, Osier Yang wrote:
*/ -%#include <libvirt/libvirt.h> %#include "internal.h" %#include <arpa/inet.h>
This has broken make check
Let's revert this hunk, and stop enforcing the checks on .x files, then.
GEN remote_protocol-struct --- remote_protocol-structs 2013-04-11 18:02:02.445662659 +0100 +++ remote_protocol-struct-t3 2013-04-17 11:57:29.472083827 +0100 @@ -7,6 +7,7 @@ VIR_TYPED_PARAM_DOUBLE = 5, VIR_TYPED_PARAM_BOOLEAN = 6, VIR_TYPED_PARAM_STRING = 7, + VIR_TYPED_PARAM_LAST = 8,
We _don't_ want the wire protocol to be exposing *_LAST enum elements. That's _why_ we included <libvirt/libvirt.h> first (to get only public names), then "internal.h" second (to get everything else). Including "internal.h" first (or in isolation) turns on private names such as *_LAST elements. I forgot about that, and hadn't actually tested your patch (I was just reviewing it based on content), or I would have spotted it sooner.
}; struct remote_nonnull_domain { remote_nonnull_string name; make[2]: *** [remote_protocol-struct] Error 1
I'm wondering why it works on my FC18.
Probably because you don't have dwarves installed, or if you do, because there have been known-buggy versions of dwarves where pdwtags fails to parse files compiled by newer gcc versions; in either of those situations, 'make check' no longer validates the remote_protocol-struct file. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 17/04/13 23:07, Eric Blake wrote:
On 04/17/2013 06:38 AM, Osier Yang wrote:
*/ -%#include <libvirt/libvirt.h> %#include "internal.h" %#include <arpa/inet.h>
This has broken make check Let's revert this hunk, and stop enforcing the checks on .x files, then.
GEN remote_protocol-struct --- remote_protocol-structs 2013-04-11 18:02:02.445662659 +0100 +++ remote_protocol-struct-t3 2013-04-17 11:57:29.472083827 +0100 @@ -7,6 +7,7 @@ VIR_TYPED_PARAM_DOUBLE = 5, VIR_TYPED_PARAM_BOOLEAN = 6, VIR_TYPED_PARAM_STRING = 7, + VIR_TYPED_PARAM_LAST = 8,
We _don't_ want the wire protocol to be exposing *_LAST enum elements. That's _why_ we included <libvirt/libvirt.h> first (to get only public names), then "internal.h" second (to get everything else). Including "internal.h" first (or in isolation) turns on private names such as *_LAST elements. I forgot about that, and hadn't actually tested your patch (I was just reviewing it based on content), or I would have spotted it sooner.
Hum, I see.
}; struct remote_nonnull_domain { remote_nonnull_string name; make[2]: *** [remote_protocol-struct] Error 1
I'm wondering why it works on my FC18. Probably because you don't have dwarves installed, or if you do, because there have been known-buggy versions of dwarves where pdwtags fails to parse files compiled by newer gcc versions; in either of those situations, 'make check' no longer validates the remote_protocol-struct file.
Right, I have not installed it, :( Pushed the reverting patch in build-breaker rule: commit f043199413c8263a17802cf2c0636bc1fe348772 Author: Osier Yang <jyang@redhat.com> Date: Wed Apr 17 23:14:52 2013 +0800 remote: Revert removing "libvirt/libvirt.h" in remote_protocol.x Commit 2d25fd4f410f removed the including of "libvirt/libvirt.h", which breaks the build. Pushed under build-breaker rule. diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index d384225..b957b8e 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -36,6 +36,7 @@ * 'REMOTE_'. This makes names quite long. */ +%#include <libvirt/libvirt.h> %#include "internal.h" %#include <arpa/inet.h> Osier
Which is already included in "internal.h", later patch will add syntax-check to avoid it. --- src/remote/remote_driver.h | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/remote/remote_driver.h b/src/remote/remote_driver.h index 7288381..bd907c0 100644 --- a/src/remote/remote_driver.h +++ b/src/remote/remote_driver.h @@ -24,8 +24,6 @@ #ifndef __VIR_REMOTE_INTERNAL_H__ # define __VIR_REMOTE_INTERNAL_H__ -# include "libvirt/virterror.h" - # include "configmake.h" int remoteRegister (void); -- 1.8.1.4
On 04/16/2013 07:41 AM, Osier Yang wrote:
Which is already included in "internal.h", later patch will add syntax-check to avoid it. --- src/remote/remote_driver.h | 2 -- 1 file changed, 2 deletions(-)
diff --git a/src/remote/remote_driver.h b/src/remote/remote_driver.h index 7288381..bd907c0 100644 --- a/src/remote/remote_driver.h +++ b/src/remote/remote_driver.h @@ -24,8 +24,6 @@ #ifndef __VIR_REMOTE_INTERNAL_H__ # define __VIR_REMOTE_INTERNAL_H__
-# include "libvirt/virterror.h" - # include "configmake.h"
Except that none of the remaining includes pull in internal.h, so this change would make the header not self-contained. I'd be happier if you did s#libvirt/virterror.h#internal.h# instead of deleting the line. ACK with that change. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 17/04/13 11:16, Eric Blake wrote:
On 04/16/2013 07:41 AM, Osier Yang wrote:
Which is already included in "internal.h", later patch will add syntax-check to avoid it. --- src/remote/remote_driver.h | 2 -- 1 file changed, 2 deletions(-)
diff --git a/src/remote/remote_driver.h b/src/remote/remote_driver.h index 7288381..bd907c0 100644 --- a/src/remote/remote_driver.h +++ b/src/remote/remote_driver.h @@ -24,8 +24,6 @@ #ifndef __VIR_REMOTE_INTERNAL_H__ # define __VIR_REMOTE_INTERNAL_H__
-# include "libvirt/virterror.h" - # include "configmake.h" Except that none of the remaining includes pull in internal.h, so this change would make the header not self-contained. I'd be happier if you did s#libvirt/virterror.h#internal.h# instead of deleting the line.
ACK with that change.
I pushed the 1/4 ~ 4/4. with this change; the shell script improved with your suggestion in 1/4's commit log; 2/4 is rebased before 1/4. Thanks. Osier
Except gnulib... --- cfg.mk | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/cfg.mk b/cfg.mk index e60c4e3..71f7ee4 100644 --- a/cfg.mk +++ b/cfg.mk @@ -722,6 +722,28 @@ sc_prohibit_exit_in_tests: halt='use return, not exit(), in tests' \ $(_sc_search_regexp) +# Don't include duplicate header in the source (either *.c or *.h) +sc_prohibit_duplicate_header: + @for i in $$($(VC_LIST_EXCEPT) | grep '\.[ch]$$'); do \ + awk 'BEGIN { \ + FS=" "; \ + fail=0; \ + } \ + /^# *include.*\.h[">]$$/ { \ + arr[$$NF]++; \ + } \ + END { \ + for (key in arr) { \ + if (arr[key] > 1) { \ + fail=1; \ + printf("%d %s are included\n", arr[key], key); \ + } \ + } \ + if (fail == 1) \ + exit 1; \ + }' $$i || { echo "Duplicate header(s) in $$i"; exit 1; }; \ + done + # We don't use this feature of maint.mk. prev_version_file = /dev/null -- 1.8.1.4
On 04/16/2013 07:41 AM, Osier Yang wrote:
Except gnulib... --- cfg.mk | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
diff --git a/cfg.mk b/cfg.mk index e60c4e3..71f7ee4 100644 --- a/cfg.mk +++ b/cfg.mk @@ -722,6 +722,28 @@ sc_prohibit_exit_in_tests: halt='use return, not exit(), in tests' \ $(_sc_search_regexp)
+# Don't include duplicate header in the source (either *.c or *.h) +sc_prohibit_duplicate_header: + @for i in $$($(VC_LIST_EXCEPT) | grep '\.[ch]$$'); do \
You also cleaned up .x files, so use [chx] instead of [ch].
+ awk 'BEGIN { \ + FS=" "; \
I don't think you need to set FS, if you use my match/substr change.
+ fail=0; \ + } \ + /^# *include.*\.h[">]$$/ { \ + arr[$$NF]++; \ + } \
Here's where my comments on patch 1 should be incorporated: /# *include/ { \ match($0, /[<"][^>"]*[">]/) \ arr[substr($0, RMATCH + 1, RLENGTH - 2)]++ \ }
+ END { \ + for (key in arr) { \ + if (arr[key] > 1) { \ + fail=1; \ + printf("%d %s are included\n", arr[key], key); \ + } \ + } \ + if (fail == 1) \ + exit 1; \ + }' $$i || { echo "Duplicate header(s) in $$i"; exit 1; }; \
This exits on first failure, instead of collecting all failures in one go. It also misses the prefix $(ME): used in other error messages, and should be sent to stderr. I would do: @fail=0; for i in ... awk '{ ... if (fail == 1) { printf("duplicate header(s) in " FILENAME } }' $$i || fail=1 done; if test $$fail = 1; then { echo "$(ME): avoid duplicate headers" >&2; exit 1; } fi -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Directories python/tools/examples should include them in <> form, though this patch allows "" form in these directories by excluding them, a later patch will do the cleanup. --- cfg.mk | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/cfg.mk b/cfg.mk index 71f7ee4..cb8079c 100644 --- a/cfg.mk +++ b/cfg.mk @@ -744,6 +744,20 @@ sc_prohibit_duplicate_header: }' $$i || { echo "Duplicate header(s) in $$i"; exit 1; }; \ done +# Don't include "libvirt/libvirt.h" in "" form. +sc_prohibit_include_libvirt_h: + @prohibit='^# *include *"libvirt/libvirt\.h"' \ + in_vc_files='\.[ch]$$' \ + halt='Do not include libvirt/libvirt.h' \ + $(_sc_search_regexp) + +# Don't include "libvirt/virterror.h" in "" form. +sc_prohibit_include_virterror_h: + @prohibit='^# *include *"libvirt/virterror\.h"' \ + in_vc_files='\.[ch]$$' \ + halt='Do not include libvirt/virterror.h' \ + $(_sc_search_regexp) + # We don't use this feature of maint.mk. prev_version_file = /dev/null @@ -897,3 +911,9 @@ exclude_file_name_regexp--sc_correct_id_types = \ (^src/locking/lock_protocol.x$$) exclude_file_name_regexp--sc_m4_quote_check = m4/virt-lib.m4 + +exclude_file_name_regexp--sc_prohibit_include_libvirt_h = \ + ^(src/internal\.h)|(include/libvirt/libvirt-(lxc|qemu)\.h)|(python/libvirt-override\.c)|(python/typewrappers\.h)$$ + +exclude_file_name_regexp--sc_prohibit_include_virterror_h = \ + ^(src/internal\.h)|(python/libvirt-|(lxc|qemu)-override\.c)|(python/typewrappers\.h)$$ -- 1.8.1.4
On 04/16/2013 07:41 AM, Osier Yang wrote:
Directories python/tools/examples should include them in <> form, though this patch allows "" form in these directories by excluding them, a later patch will do the cleanup. --- cfg.mk | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
+# Don't include "libvirt/libvirt.h" in "" form. +sc_prohibit_include_libvirt_h: + @prohibit='^# *include *"libvirt/libvirt\.h"' \ + in_vc_files='\.[ch]$$' \ + halt='Do not include libvirt/libvirt.h' \ + $(_sc_search_regexp) + +# Don't include "libvirt/virterror.h" in "" form. +sc_prohibit_include_virterror_h: + @prohibit='^# *include *"libvirt/virterror\.h"' \
Combine these into one rule, for faster execution of 'make syntax-check'. Also, while we generally use #include with # in column 1, C allows it in later columns and a stronger test avoids an anchor (but if that opens up false negatives, then adding the anchor back in is fine). @prohibit='# *include *"libvirt/*\.h"'
+ in_vc_files='\.[ch]$$' \
Another case where .[chx] might be better, since we cleaned up .x files.
+ halt='Do not include libvirt/virterror.h' \
This halt message needs alteration when you merge the two checks into one.
+ +exclude_file_name_regexp--sc_prohibit_include_libvirt_h = \ + ^(src/internal\.h)|(include/libvirt/libvirt-(lxc|qemu)\.h)|(python/libvirt-override\.c)|(python/typewrappers\.h)$$ + +exclude_file_name_regexp--sc_prohibit_include_virterror_h = \ + ^(src/internal\.h)|(python/libvirt-|(lxc|qemu)-override\.c)|(python/typewrappers\.h)$$
and if you merge the rules, come up with a common name, and then you only need one exclude rule. If we are excluding entire directories, we could write this more compact as: ^(src/internal\.h$$\|python/\|tools/\|examples/\|include/libvirt/libvirt-*\.h$$) -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
With this patch, include "libvirt.h" and "virterror.h" in "" form is only allowed for "internal.h". And only the external tools (examples|tools|python|include/libvirt) can include the two headers in <> form. --- cfg.mk | 30 ++++++++++++++++++++++++++---- include/libvirt/libvirt-lxc.h | 2 +- include/libvirt/libvirt-qemu.h | 2 +- python/libvirt-lxc-override.c | 4 ++-- python/libvirt-override.c | 4 ++-- python/libvirt-qemu-override.c | 4 ++-- python/typewrappers.h | 4 ++-- tests/shunloadhelper.c | 2 -- 8 files changed, 36 insertions(+), 16 deletions(-) diff --git a/cfg.mk b/cfg.mk index cb8079c..98c7e40 100644 --- a/cfg.mk +++ b/cfg.mk @@ -748,14 +748,30 @@ sc_prohibit_duplicate_header: sc_prohibit_include_libvirt_h: @prohibit='^# *include *"libvirt/libvirt\.h"' \ in_vc_files='\.[ch]$$' \ - halt='Do not include libvirt/libvirt.h' \ + halt='Do not include libvirt/libvirt.h in internal source' \ $(_sc_search_regexp) # Don't include "libvirt/virterror.h" in "" form. sc_prohibit_include_virterror_h: @prohibit='^# *include *"libvirt/virterror\.h"' \ in_vc_files='\.[ch]$$' \ - halt='Do not include libvirt/virterror.h' \ + halt='Do not include libvirt/virterror.h in internal source' \ + $(_sc_search_regexp) + +# Don't include "libvirt/libvirt.h" in <> form. Except external tools, e.g. +# python binding, examples and tools subdirectories. +sc_prohibit_include_libvirt_h_1: + @prohibit='^# *include *<libvirt/libvirt\.h>' \ + in_vc_files='\.[ch]$$' \ + halt='Do not include libvirt/libvirt.h in internal source' \ + $(_sc_search_regexp) + +# Don't include "libvirt/virterror.h" in <> form. Except external tools, e.g. +# python binding, examples and tools subdirectories. +sc_prohibit_include_virterror_h_1: + @prohibit='^# *include *<libvirt/virterror\.h>' \ + in_vc_files='\.[ch]$$' \ + halt='Do not include libvirt/virterror.h in internal source' \ $(_sc_search_regexp) # We don't use this feature of maint.mk. @@ -913,7 +929,13 @@ exclude_file_name_regexp--sc_correct_id_types = \ exclude_file_name_regexp--sc_m4_quote_check = m4/virt-lib.m4 exclude_file_name_regexp--sc_prohibit_include_libvirt_h = \ - ^(src/internal\.h)|(include/libvirt/libvirt-(lxc|qemu)\.h)|(python/libvirt-override\.c)|(python/typewrappers\.h)$$ + ^src/internal\.h$$ exclude_file_name_regexp--sc_prohibit_include_virterror_h = \ - ^(src/internal\.h)|(python/libvirt-|(lxc|qemu)-override\.c)|(python/typewrappers\.h)$$ + ^src/internal\.h$$ + +exclude_file_name_regexp--sc_prohibit_include_libvirt_h_1 = \ + ^(examples/|tools/|python/|include/libvirt/) + +exclude_file_name_regexp--sc_prohibit_include_virterror_h_1 = \ + ^(examples/|tools/|python/|include/libvirt/) diff --git a/include/libvirt/libvirt-lxc.h b/include/libvirt/libvirt-lxc.h index 5021813..1901fce 100644 --- a/include/libvirt/libvirt-lxc.h +++ b/include/libvirt/libvirt-lxc.h @@ -26,7 +26,7 @@ #ifndef __VIR_LXC_H__ # define __VIR_LXC_H__ -# include "libvirt/libvirt.h" +# include <libvirt/libvirt.h> # ifdef __cplusplus extern "C" { diff --git a/include/libvirt/libvirt-qemu.h b/include/libvirt/libvirt-qemu.h index 8ec12b4..3e79a8c 100644 --- a/include/libvirt/libvirt-qemu.h +++ b/include/libvirt/libvirt-qemu.h @@ -26,7 +26,7 @@ #ifndef __VIR_QEMU_H__ # define __VIR_QEMU_H__ -# include "libvirt/libvirt.h" +# include <libvirt/libvirt.h> # ifdef __cplusplus extern "C" { diff --git a/python/libvirt-lxc-override.c b/python/libvirt-lxc-override.c index c80668e..ead175f 100644 --- a/python/libvirt-lxc-override.c +++ b/python/libvirt-lxc-override.c @@ -17,8 +17,8 @@ #undef HAVE_PTHREAD_H #include <Python.h> -#include "libvirt/libvirt-lxc.h" -#include "libvirt/virterror.h" +#include <libvirt/libvirt-lxc.h> +#include <libvirt/virterror.h> #include "typewrappers.h" #include "libvirt-lxc.h" #include "viralloc.h" diff --git a/python/libvirt-override.c b/python/libvirt-override.c index f6573e1..3d8490c 100644 --- a/python/libvirt-override.c +++ b/python/libvirt-override.c @@ -20,8 +20,8 @@ #define VIR_ENUM_SENTINELS #include <Python.h> -#include "libvirt/libvirt.h" -#include "libvirt/virterror.h" +#include <libvirt/libvirt.h> +#include <libvirt/virterror.h> #include "typewrappers.h" #include "libvirt.h" #include "viralloc.h" diff --git a/python/libvirt-qemu-override.c b/python/libvirt-qemu-override.c index 243692a..8f1ce5e 100644 --- a/python/libvirt-qemu-override.c +++ b/python/libvirt-qemu-override.c @@ -17,8 +17,8 @@ #undef HAVE_PTHREAD_H #include <Python.h> -#include "libvirt/libvirt-qemu.h" -#include "libvirt/virterror.h" +#include <libvirt/libvirt-qemu.h> +#include <libvirt/virterror.h> #include "typewrappers.h" #include "libvirt-qemu.h" diff --git a/python/typewrappers.h b/python/typewrappers.h index af68bce..d871d3f 100644 --- a/python/typewrappers.h +++ b/python/typewrappers.h @@ -8,8 +8,8 @@ #include <Python.h> #include <stdbool.h> -#include "libvirt/libvirt.h" -#include "libvirt/virterror.h" +#include <libvirt/libvirt.h> +#include <libvirt/virterror.h> #ifdef __GNUC__ # ifdef ATTRIBUTE_UNUSED diff --git a/tests/shunloadhelper.c b/tests/shunloadhelper.c index 1b025ee..a7bc2cc 100644 --- a/tests/shunloadhelper.c +++ b/tests/shunloadhelper.c @@ -28,8 +28,6 @@ #include <config.h> #include "internal.h" -#include <libvirt/libvirt.h> -#include <libvirt/virterror.h> #include <stdlib.h> static void shunloadError(void *userData ATTRIBUTE_UNUSED, -- 1.8.1.4
On 04/16/2013 07:41 AM, Osier Yang wrote:
With this patch, include "libvirt.h" and "virterror.h" in "" form is only allowed for "internal.h". And only the external tools (examples|tools|python|include/libvirt) can include the two headers in <> form. ---
Hmm. It sounds like you want two syntax checks after all, in order to allow <> but not "" in the subdirectories; but it can still be done in two rules instead of four.
cfg.mk | 30 ++++++++++++++++++++++++++---- include/libvirt/libvirt-lxc.h | 2 +- include/libvirt/libvirt-qemu.h | 2 +- python/libvirt-lxc-override.c | 4 ++-- python/libvirt-override.c | 4 ++-- python/libvirt-qemu-override.c | 4 ++-- python/typewrappers.h | 4 ++-- tests/shunloadhelper.c | 2 -- 8 files changed, 36 insertions(+), 16 deletions(-)
diff --git a/cfg.mk b/cfg.mk index cb8079c..98c7e40 100644 --- a/cfg.mk +++ b/cfg.mk @@ -748,14 +748,30 @@ sc_prohibit_duplicate_header: sc_prohibit_include_libvirt_h:
I'd name this sc_prohibit_include_libvirt_quote (that affects patch 6); see below for why...
@prohibit='^# *include *"libvirt/libvirt\.h"' \ in_vc_files='\.[ch]$$' \ - halt='Do not include libvirt/libvirt.h' \ + halt='Do not include libvirt/libvirt.h in internal source' \
Squash this wording into patch 6.
$(_sc_search_regexp)
# Don't include "libvirt/virterror.h" in "" form. sc_prohibit_include_virterror_h: @prohibit='^# *include *"libvirt/virterror\.h"' \ in_vc_files='\.[ch]$$' \ - halt='Do not include libvirt/virterror.h' \ + halt='Do not include libvirt/virterror.h in internal source' \ + $(_sc_search_regexp) + +# Don't include "libvirt/libvirt.h" in <> form. Except external tools, e.g. +# python binding, examples and tools subdirectories. +sc_prohibit_include_libvirt_h_1:
...here's why. The _1 suffix doesn't describe anything. So I'd name this sc_prohibit_include_libvirt_brackets
+ @prohibit='^# *include *<libvirt/libvirt\.h>' \ + in_vc_files='\.[ch]$$' \ + halt='Do not include libvirt/libvirt.h in internal source' \ + $(_sc_search_regexp) + +# Don't include "libvirt/virterror.h" in <> form. Except external tools, e.g. +# python binding, examples and tools subdirectories. +sc_prohibit_include_virterror_h_1: + @prohibit='^# *include *<libvirt/virterror\.h>' \ + in_vc_files='\.[ch]$$' \ + halt='Do not include libvirt/virterror.h in internal source' \ $(_sc_search_regexp)
Again, these two can be merged into one.
# We don't use this feature of maint.mk. @@ -913,7 +929,13 @@ exclude_file_name_regexp--sc_correct_id_types = \ exclude_file_name_regexp--sc_m4_quote_check = m4/virt-lib.m4
exclude_file_name_regexp--sc_prohibit_include_libvirt_h = \ - ^(src/internal\.h)|(include/libvirt/libvirt-(lxc|qemu)\.h)|(python/libvirt-override\.c)|(python/typewrappers\.h)$$ + ^src/internal\.h$$
exclude_file_name_regexp--sc_prohibit_include_virterror_h = \ - ^(src/internal\.h)|(python/libvirt-|(lxc|qemu)-override\.c)|(python/typewrappers\.h)$$ + ^src/internal\.h$$ + +exclude_file_name_regexp--sc_prohibit_include_libvirt_h_1 = \ + ^(examples/|tools/|python/|include/libvirt/) + +exclude_file_name_regexp--sc_prohibit_include_virterror_h_1 = \ + ^(examples/|tools/|python/|include/libvirt/)
Maybe it's even worth merging 6 and 7 into a single patch, or splitting the cleanup into a first patch, and the syntax check into a second.
+++ b/tests/shunloadhelper.c @@ -28,8 +28,6 @@ #include <config.h> #include "internal.h"
-#include <libvirt/libvirt.h> -#include <libvirt/virterror.h> #include <stdlib.h>
static void shunloadError(void *userData ATTRIBUTE_UNUSED,
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Like libvirt.h, these two public headers also shouldn't be included in internal source, except "internal.h". A later patch will force tools to use <> form. --- cfg.mk | 20 ++++++++++++++++++++ daemon/remote.c | 2 -- src/libvirt-qemu.c | 1 - 3 files changed, 20 insertions(+), 3 deletions(-) diff --git a/cfg.mk b/cfg.mk index 98c7e40..4b84bde 100644 --- a/cfg.mk +++ b/cfg.mk @@ -758,6 +758,20 @@ sc_prohibit_include_virterror_h: halt='Do not include libvirt/virterror.h in internal source' \ $(_sc_search_regexp) +# Don't include "libvirt/libvirt-qemu.h" in "" form. +sc_prohibit_include_libvirt_qemu_h: + @prohibit='^# *include *"libvirt/libvirt-qemu\.h"' \ + in_vc_files='\.[ch]$$' \ + halt='Do not include libvirt/libvirt-qemu.h in internal source' \ + $(_sc_search_regexp) + +# Don't include "libvirt/libvirt-lxc.h" in "" form. +sc_prohibit_include_libvirt_lxc_h: + @prohibit='^# *include *"libvirt/libvirt-lxc\.h"' \ + in_vc_files='\.[ch]$$' \ + halt='Do not include libvirt/libvirt-lxc.h in internal source' \ + $(_sc_search_regexp) + # Don't include "libvirt/libvirt.h" in <> form. Except external tools, e.g. # python binding, examples and tools subdirectories. sc_prohibit_include_libvirt_h_1: @@ -939,3 +953,9 @@ exclude_file_name_regexp--sc_prohibit_include_libvirt_h_1 = \ exclude_file_name_regexp--sc_prohibit_include_virterror_h_1 = \ ^(examples/|tools/|python/|include/libvirt/) + +exclude_file_name_regexp--sc_prohibit_include_libvirt_qemu_h = \ + ^(src/internal\.h$$|tools/) + +exclude_file_name_regexp--sc_prohibit_include_libvirt_lxc_h = \ + ^(src/internal\.h$$|tools/) diff --git a/daemon/remote.c b/daemon/remote.c index 45c50f3..c559d6f 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -38,8 +38,6 @@ #include "virutil.h" #include "stream.h" #include "viruuid.h" -#include "libvirt/libvirt-qemu.h" -#include "libvirt/libvirt-lxc.h" #include "vircommand.h" #include "intprops.h" #include "virnetserverservice.h" diff --git a/src/libvirt-qemu.c b/src/libvirt-qemu.c index 11da2f3..fb19584 100644 --- a/src/libvirt-qemu.c +++ b/src/libvirt-qemu.c @@ -26,7 +26,6 @@ #include "virerror.h" #include "virlog.h" #include "datatypes.h" -#include "libvirt/libvirt-qemu.h" #define VIR_FROM_THIS VIR_FROM_NONE -- 1.8.1.4
Including libvirt-{qemu,lxc}.h in "" form is only allowed for "internal.h" now, and <> form is only allowed for external tools. --- cfg.mk | 38 ++++++++++++++++++++++++++++++-------- tools/virsh.c | 4 ++-- 2 files changed, 32 insertions(+), 10 deletions(-) diff --git a/cfg.mk b/cfg.mk index 4b84bde..4736d75 100644 --- a/cfg.mk +++ b/cfg.mk @@ -760,34 +760,50 @@ sc_prohibit_include_virterror_h: # Don't include "libvirt/libvirt-qemu.h" in "" form. sc_prohibit_include_libvirt_qemu_h: - @prohibit='^# *include *"libvirt/libvirt-qemu\.h"' \ + @prohibit='^# *include *"libvirt/libvirt-qemu\.h"' \ in_vc_files='\.[ch]$$' \ halt='Do not include libvirt/libvirt-qemu.h in internal source' \ $(_sc_search_regexp) # Don't include "libvirt/libvirt-lxc.h" in "" form. sc_prohibit_include_libvirt_lxc_h: - @prohibit='^# *include *"libvirt/libvirt-lxc\.h"' \ + @prohibit='^# *include *"libvirt/libvirt-lxc\.h"' \ in_vc_files='\.[ch]$$' \ halt='Do not include libvirt/libvirt-lxc.h in internal source' \ $(_sc_search_regexp) -# Don't include "libvirt/libvirt.h" in <> form. Except external tools, e.g. -# python binding, examples and tools subdirectories. +# Don't include "libvirt/libvirt.h" in <> form. Except for external tools, +# e.g. python binding, examples and tools subdirectories. sc_prohibit_include_libvirt_h_1: @prohibit='^# *include *<libvirt/libvirt\.h>' \ in_vc_files='\.[ch]$$' \ halt='Do not include libvirt/libvirt.h in internal source' \ $(_sc_search_regexp) -# Don't include "libvirt/virterror.h" in <> form. Except external tools, e.g. -# python binding, examples and tools subdirectories. +# Don't include "libvirt/virterror.h" in <> form. Except for external tools, +# e.g. python binding, examples and tools subdirectories. sc_prohibit_include_virterror_h_1: @prohibit='^# *include *<libvirt/virterror\.h>' \ in_vc_files='\.[ch]$$' \ halt='Do not include libvirt/virterror.h in internal source' \ $(_sc_search_regexp) +# Don't include "libvirt/libvirt-qemu.h" in <> form. Except external tools, +# e.g. python binding, examples and tools subdirectories. +sc_prohibit_include_libvirt_qemu_h_1: + @prohibit='^# *include *<libvirt/libvirt-qemu\.h>' \ + in_vc_files='\.[ch]$$' \ + halt='Do not include libvirt/libvirt-qemu.h in internal source' \ + $(_sc_search_regexp) + +# Don't include "libvirt/libvirt-lxc.h" in <> form. Except external tools, +# e.g. python binding, examples and tools subdirectories. +sc_prohibit_include_libvirt_lxc_h_1: + @prohibit='^# *include *<libvirt/libvirt-lxc\.h>' \ + in_vc_files='\.[ch]$$' \ + halt='Do not include libvirt/libvirt-lxc.h in internal source' \ + $(_sc_search_regexp) + # We don't use this feature of maint.mk. prev_version_file = /dev/null @@ -955,7 +971,13 @@ exclude_file_name_regexp--sc_prohibit_include_virterror_h_1 = \ ^(examples/|tools/|python/|include/libvirt/) exclude_file_name_regexp--sc_prohibit_include_libvirt_qemu_h = \ - ^(src/internal\.h$$|tools/) + ^src/internal\.h$$ exclude_file_name_regexp--sc_prohibit_include_libvirt_lxc_h = \ - ^(src/internal\.h$$|tools/) + ^src/internal\.h$$ + +exclude_file_name_regexp--sc_prohibit_include_libvirt_qemu_h_1 = \ + ^(examples/|tools/|python/|include/libvirt/) + +exclude_file_name_regexp--sc_prohibit_include_libvirt_lxc_h_1 = \ + ^(examples/|tools/|python/|include/libvirt/) diff --git a/tools/virsh.c b/tools/virsh.c index b7a5cc1..4cd93ca 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -60,8 +60,8 @@ #include "virutil.h" #include "viralloc.h" #include "virxml.h" -#include "libvirt/libvirt-qemu.h" -#include "libvirt/libvirt-lxc.h" +#include <libvirt/libvirt-qemu.h> +#include <libvirt/libvirt-lxc.h> #include "virfile.h" #include "configmake.h" #include "virthread.h" -- 1.8.1.4
To tell libvirt-{qemu,lxc}.h shouldn't be included either. --- docs/hacking.html.in | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/hacking.html.in b/docs/hacking.html.in index 7ef826c..67af5c2 100644 --- a/docs/hacking.html.in +++ b/docs/hacking.html.in @@ -929,10 +929,10 @@ </pre> <p> - Of particular note: <b>Do not</b> include libvirt/libvirt.h or - libvirt/virterror.h. It is included by "internal.h" already and there - are some special reasons why you cannot include these files - explicitly. + Of particular note: <b>Do not</b> include libvirt/libvirt.h, + libvirt/virterror.h, libvirt/libvirt-qemu.h, and libvirt/libvirt-lxc.h. + It is included by "internal.h" already and there are some special reasons + why you cannot include these files explicitly. </p> -- 1.8.1.4
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Osier Yang