[libvirt] [PATCH 0/6] cleanup: About including header
*** BLURB HERE *** Osier Yang (6): 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 cfg.mk | 34 ++++++++++++++++++++++++++++++++++ daemon/libvirtd.c | 1 - src/conf/node_device_conf.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/shunloadtest.c | 4 ++-- tests/vmx2xmltest.c | 4 ++-- tests/xml2vmxtest.c | 4 ++-- 31 files changed, 60 insertions(+), 45 deletions(-) -- 1.8.1.4
Detected by a simple Shell script. --- 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 728add5..4e76799 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 0d41e39..d15fb59 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/15/2013 12:28 AM, Osier Yang wrote:
Detected by a simple Shell script.
Mentioning the contents of that script in the commit message would make it nicer to rerun the same check later, if we desired. In fact, codifying the script into a syntax-check rule would prevent running into this situation again (I haven't yet checked if you do that later in the series, but if you do, then mentioning in _this_ commit that a later commit will add a syntax-check would be helpful). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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..0c6f3cb 100644 --- a/tests/esxutilstest.c +++ b/tests/esxutilstest.c @@ -1,5 +1,7 @@ #include <config.h> +#include "virutil.h" + #ifdef WITH_ESX # include <stdio.h> @@ -9,7 +11,6 @@ # include "internal.h" # include "viralloc.h" # include "testutils.h" -# include "virutil.h" # include "vmx/vmx.h" # include "esx/esx_util.h" # include "esx/esx_vi_types.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
diff --git a/tests/esxutilstest.c b/tests/esxutilstest.c index 55b3623..0c6f3cb 100644 --- a/tests/esxutilstest.c +++ b/tests/esxutilstest.c @@ -1,5 +1,7 @@ #include <config.h>
+#include "virutil.h" + #ifdef WITH_ESX
# include <stdio.h> @@ -9,7 +11,6 @@ # include "internal.h" # include "viralloc.h" # include "testutils.h" -# include "virutil.h" # include "vmx/vmx.h" # include "esx/esx_util.h" # include "esx/esx_vi_types.h" @@ -279,7 +280,6 @@ mymain(void) VIRT_TEST_MAIN(mymain)
#else -# include "testutils.h"
int main(void) {
You moved "virutil.h" instead of "testutils.h" here. Jan
Which is already included by "internal.h". --- 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
--- 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
--- cfg.mk | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/cfg.mk b/cfg.mk index 394521e..9cf4cff 100644 --- a/cfg.mk +++ b/cfg.mk @@ -722,6 +722,20 @@ sc_prohibit_exit_in_tests: halt='use return, not exit(), in tests' \ $(_sc_search_regexp) +# Don't include duplidate header in the source (either *.c or *.h) +sc_prohibit_duplicate_header: + @if $(VC_LIST_EXCEPT) | grep -l '\.[ch]$$' > /dev/null; then \ + for i in $$($(VC_LIST_EXCEPT) | grep '\.[ch]$$'); do \ + for j in $$(grep '^# *include.*\.h' $$i \ + | awk -F' ' '{print $$NF}'); do \ + test $$(grep "^# *include $$j" $$i | wc -l) -gt 1 && \ + { echo '$(ME): Duplidate header '$$j' in '$$i'' 1>&2; \ + exit 1; } || :; \ + done; \ + done; \ + else :; \ + fi + # We don't use this feature of maint.mk. prev_version_file = /dev/null -- 1.8.1.4
On 04/15/2013 12:28 AM, Osier Yang wrote:
--- cfg.mk | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
diff --git a/cfg.mk b/cfg.mk index 394521e..9cf4cff 100644 --- a/cfg.mk +++ b/cfg.mk @@ -722,6 +722,20 @@ sc_prohibit_exit_in_tests: halt='use return, not exit(), in tests' \ $(_sc_search_regexp)
+# Don't include duplidate header in the source (either *.c or *.h)
s/duplidate/duplicate/
+sc_prohibit_duplicate_header: + @if $(VC_LIST_EXCEPT) | grep -l '\.[ch]$$' > /dev/null; then \
'grep -l' is wrong. You want: grep '\.[ch]$$' instead (see sc_preprocessor_indenation for an example). For that matter, the '@if ...; then check; else :; fi' is overkill; we KNOW we have .c files so the grep will hit (that paradigm is used in maint.mk, because maint.mk is shared among multiple projects where some projects really do ship without C files). I'd simplify this to just the 'check' portion, that is, all you need is from here...
+ for i in $$($(VC_LIST_EXCEPT) | grep '\.[ch]$$'); do \ + for j in $$(grep '^# *include.*\.h' $$i \ + | awk -F' ' '{print $$NF}'); do \ + test $$(grep "^# *include $$j" $$i | wc -l) -gt 1 && \
Not the most efficient way to write this. Your way does one grep/wc pair per header line encountered per file. But since you are already running each file through awk, why not have awk set up a hash of all includes it sees, and then report an error if the hash hits more than once, all on a single awk pass per file instead of 20-30 grep passes per file. Would you like to take a shot at it, or shall I do it since I mentioned it?
+ { echo '$(ME): Duplidate header '$$j' in '$$i'' 1>&2; \
s/Duplidate/Duplicate/
+ exit 1; } || :; \ + done; \ + done; \
...to here.
+ else :; \ + fi + # We don't use this feature of maint.mk. prev_version_file = /dev/null
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 15/04/13 23:11, Eric Blake wrote:
On 04/15/2013 12:28 AM, Osier Yang wrote:
--- cfg.mk | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
diff --git a/cfg.mk b/cfg.mk index 394521e..9cf4cff 100644 --- a/cfg.mk +++ b/cfg.mk @@ -722,6 +722,20 @@ sc_prohibit_exit_in_tests: halt='use return, not exit(), in tests' \ $(_sc_search_regexp)
+# Don't include duplidate header in the source (either *.c or *.h) s/duplidate/duplicate/
+sc_prohibit_duplicate_header: + @if $(VC_LIST_EXCEPT) | grep -l '\.[ch]$$' > /dev/null; then \ 'grep -l' is wrong. You want:
grep '\.[ch]$$'
instead (see sc_preprocessor_indenation for an example).
For that matter, the '@if ...; then check; else :; fi' is overkill; we KNOW we have .c files so the grep will hit (that paradigm is used in maint.mk, because maint.mk is shared among multiple projects where some projects really do ship without C files). I'd simplify this to just the 'check' portion, that is, all you need is from here...
In case of duplicate work, I finished this, but need more work on 6/6 to send out the set.
+ for i in $$($(VC_LIST_EXCEPT) | grep '\.[ch]$$'); do \ + for j in $$(grep '^# *include.*\.h' $$i \ + | awk -F' ' '{print $$NF}'); do \ + test $$(grep "^# *include $$j" $$i | wc -l) -gt 1 && \ Not the most efficient way to write this. Your way does one grep/wc pair per header line encountered per file. But since you are already running each file through awk, why not have awk set up a hash of all includes it sees, and then report an error if the hash hits more than once, all on a single awk pass per file instead of 20-30 grep passes per file. Would you like to take a shot at it, or shall I do it since I mentioned it?
+ { echo '$(ME): Duplidate header '$$j' in '$$i'' 1>&2; \ s/Duplidate/Duplicate/
+ exit 1; } || :; \ + done; \ + done; \ ...to here.
+ else :; \ + fi + # We don't use this feature of maint.mk. prev_version_file = /dev/null
Except for "internal.h" and the public headers --- cfg.mk | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/cfg.mk b/cfg.mk index 9cf4cff..0f07abd 100644 --- a/cfg.mk +++ b/cfg.mk @@ -736,6 +736,20 @@ sc_prohibit_duplicate_header: else :; \ fi +# Don't include "libvirt/libvirt.h". +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". +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 @@ -888,3 +902,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/15/2013 12:28 AM, Osier Yang wrote:
Except for "internal.h" and the public headers --- cfg.mk | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
diff --git a/cfg.mk b/cfg.mk index 9cf4cff..0f07abd 100644 --- a/cfg.mk +++ b/cfg.mk @@ -736,6 +736,20 @@ sc_prohibit_duplicate_header: else :; \ fi
+# Don't include "libvirt/libvirt.h". +sc_prohibit_include_libvirt_h: + @prohibit='^# *include "libvirt/libvirt.h"' \
This prohibits "libvirt/libvirt.h", but allows <libvirt/libvirt.h>. The distinction is important, the "" form searches the current directory first, while the <> form does not. Since we are stating that this header is public, I think you're trying to state that internal files should never include this file directly (no "" or <> form, but get its contents through "internal.h" instead), while external tools (python bindings, tools subdirectory) which should be limited to just the public API should be using the <> form.
+ in_vc_files='\.[ch]$$' \ + halt='Do not include libvirt/libvirt.h' \ + $(_sc_search_regexp) + +# Don't include "libvirt/virterror.h". +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
@@ -888,3 +902,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)$$
I'm wondering if instead of excluding so many files, if we should instead be converting these files to use the <> include form. I'm also wondering if your syntax check needs to check for no "" form anywhere, and no <> form in daemon or src subdirectories (while still allowing <> form in tools and python subdirectories).
+ +exclude_file_name_regexp--sc_prohibit_include_virterror_h = \ + ^(src/internal\.h)|(python/libvirt-|(lxc|qemu)-override\.c)|(python/typewrappers\.h)$$
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 15/04/13 23:18, Eric Blake wrote:
Except for "internal.h" and the public headers --- cfg.mk | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
diff --git a/cfg.mk b/cfg.mk index 9cf4cff..0f07abd 100644 --- a/cfg.mk +++ b/cfg.mk @@ -736,6 +736,20 @@ sc_prohibit_duplicate_header: else :; \ fi
+# Don't include "libvirt/libvirt.h". +sc_prohibit_include_libvirt_h: + @prohibit='^# *include "libvirt/libvirt.h"' \ This prohibits "libvirt/libvirt.h", but allows <libvirt/libvirt.h>. The distinction is important, the "" form searches the current directory first, while the <> form does not. Since we are stating that this
On 04/15/2013 12:28 AM, Osier Yang wrote: header is public, I think you're trying to state that internal files should never include this file directly (no "" or <> form, but get its contents through "internal.h" instead), while external tools (python bindings, tools subdirectory) which should be limited to just the public API should be using the <> form.
Yeah, that was what I tended to do, posted v2, including the syntax-check rules for libvirt-{qemu,lxc}.h too.
+ in_vc_files='\.[ch]$$' \ + halt='Do not include libvirt/libvirt.h' \ + $(_sc_search_regexp) + +# Don't include "libvirt/virterror.h". +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
@@ -888,3 +902,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)$$ I'm wondering if instead of excluding so many files, if we should instead be converting these files to use the <> include form. I'm also wondering if your syntax check needs to check for no "" form anywhere, and no <> form in daemon or src subdirectories (while still allowing <> form in tools and python subdirectories).
+ +exclude_file_name_regexp--sc_prohibit_include_virterror_h = \ + ^(src/internal\.h)|(python/libvirt-|(lxc|qemu)-override\.c)|(python/typewrappers\.h)$$
On 04/15/2013 02:28 AM, Osier Yang wrote:
*** BLURB HERE ***
Osier Yang (6): 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
cfg.mk | 34 ++++++++++++++++++++++++++++++++++ daemon/libvirtd.c | 1 - src/conf/node_device_conf.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/shunloadtest.c | 4 ++-- tests/vmx2xmltest.c | 4 ++-- tests/xml2vmxtest.c | 4 ++-- 31 files changed, 60 insertions(+), 45 deletions(-)
Mechanically the series looks fine to me. Although in 6/6 do the '@prohibit' and 'halt' lines need to have the period escaped for libvirt.h and virterror.h? It seems the prohibit entry might as I see another example "@prohibit='^# *include *<ctype\.h>'" John
On 15/04/13 18:51, John Ferlan wrote:
On 04/15/2013 02:28 AM, Osier Yang wrote:
*** BLURB HERE ***
Osier Yang (6): 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
cfg.mk | 34 ++++++++++++++++++++++++++++++++++ daemon/libvirtd.c | 1 - src/conf/node_device_conf.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/shunloadtest.c | 4 ++-- tests/vmx2xmltest.c | 4 ++-- tests/xml2vmxtest.c | 4 ++-- 31 files changed, 60 insertions(+), 45 deletions(-)
Mechanically the series looks fine to me.
Although in 6/6 do the '@prohibit' and 'halt' lines need to have the period escaped for libvirt.h and virterror.h? It seems the prohibit entry might as I see another example "@prohibit='^# *include *<ctype\.h>'"
Yeah, it needs, I guess it works fine just because there is no strings like (#include "libvirt/libvirt5h") in the source. Will change, thanks. Osier
participants (4)
-
Eric Blake -
John Ferlan -
Ján Tomko -
Osier Yang