[libvirt] [PATCH 0/3 v3] cleanup: About header including
These 3 patches are the left ones in v2. 6/10 ~ 9/10 in v2 are merged into one single patch. Osier Yang (3): syntax-check: Don't include public headers in internal source syntax-check: Only allows to include public headers in external tools docs: Update HACKING cfg.mk | 21 +++++++++++++++++++++ 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/libvirt-qemu.c | 1 - tests/shunloadhelper.c | 5 ++--- tools/virsh.c | 4 ++-- 12 files changed, 39 insertions(+), 22 deletions(-) -- 1.8.1.4
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 | 10 ++++++++++ daemon/remote.c | 2 -- src/libvirt-qemu.c | 1 - 3 files changed, 10 insertions(+), 3 deletions(-) diff --git a/cfg.mk b/cfg.mk index dd67816..5f422de 100644 --- a/cfg.mk +++ b/cfg.mk @@ -746,6 +746,13 @@ sc_prohibit_duplicate_header: { echo "$(ME)": avoid duplicate headers >&2; exit 1; } \ fi; +# Don't include "libvirt/*.h" in "" form. +sc_prohibit_include_public_headers: + @prohibit='# *include *"libvirt/.*\.h"' \ + in_vc_files='\.[chx]$$' \ + halt='Do not include libvirt/*.h in internal source' \ + $(_sc_search_regexp) + # We don't use this feature of maint.mk. prev_version_file = /dev/null @@ -899,3 +906,6 @@ 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_public_headers = \ + ^(src/internal\.h$$|python/|tools/|examples/|include/libvirt/libvirt-(qemu|lxc)\.h$$) 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
On 04/17/2013 04:19 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.
Looks like a bit of a mishap while sending; this comes after the email titled: [1/4] syntax-check: Don't include duplicate header
--- cfg.mk | 10 ++++++++++ daemon/remote.c | 2 -- src/libvirt-qemu.c | 1 - 3 files changed, 10 insertions(+), 3 deletions(-)
The syntax check is okay.
+++ 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"
Fine; indirectly included by libvirt_internal.h which pulls in internal.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"
Also fine; virerror.h includes internal.h. ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
With this patch, include public headers in "" form is only allowed for "internal.h". And only the external tools (examples|tools|python |include/libvirt) can include the public headers in <> form. --- cfg.mk | 17 ++++++++++++++--- 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 | 5 ++--- tools/virsh.c | 4 ++-- 9 files changed, 28 insertions(+), 18 deletions(-) diff --git a/cfg.mk b/cfg.mk index 5f422de..4128ebb 100644 --- a/cfg.mk +++ b/cfg.mk @@ -747,12 +747,20 @@ sc_prohibit_duplicate_header: fi; # Don't include "libvirt/*.h" in "" form. -sc_prohibit_include_public_headers: +sc_prohibit_include_public_headers_quote: @prohibit='# *include *"libvirt/.*\.h"' \ in_vc_files='\.[chx]$$' \ halt='Do not include libvirt/*.h in internal source' \ $(_sc_search_regexp) +# Don't include "libvirt/*.h" in <> form. Except for external tools, +# e.g. Python binding, examples and tools subdirectories. +sc_prohibit_include_public_headers_brackets: + @prohibit='# *include *<libvirt/.*\.h>' \ + in_vc_files='\.[chx]$$' \ + halt='Do not include libvirt/*.h in internal source' \ + $(_sc_search_regexp) + # We don't use this feature of maint.mk. prev_version_file = /dev/null @@ -907,5 +915,8 @@ 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_public_headers = \ - ^(src/internal\.h$$|python/|tools/|examples/|include/libvirt/libvirt-(qemu|lxc)\.h$$) +exclude_file_name_regexp--sc_prohibit_include_public_headers_quote = \ + ^src/internal\.h$$ + +exclude_file_name_regexp--sc_prohibit_include_public_headers_brackets = \ + ^(python/|tools/|examples/|include/libvirt/(virterror|libvirt-(qemu|lxc))\.h$$) 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..a8f5aef 100644 --- a/tests/shunloadhelper.c +++ b/tests/shunloadhelper.c @@ -26,12 +26,11 @@ */ #include <config.h> -#include "internal.h" -#include <libvirt/libvirt.h> -#include <libvirt/virterror.h> #include <stdlib.h> +#include "internal.h" + static void shunloadError(void *userData ATTRIBUTE_UNUSED, virErrorPtr error ATTRIBUTE_UNUSED) { 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
On 04/17/2013 04:19 AM, Osier Yang wrote:
With this patch, include public headers in "" form is only allowed for "internal.h". And only the external tools (examples|tools|python |include/libvirt) can include the public headers in <> form. --- cfg.mk | 17 ++++++++++++++--- 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 | 5 ++--- tools/virsh.c | 4 ++-- 9 files changed, 28 insertions(+), 18 deletions(-)
diff --git a/cfg.mk b/cfg.mk index 5f422de..4128ebb 100644 --- a/cfg.mk +++ b/cfg.mk @@ -747,12 +747,20 @@ sc_prohibit_duplicate_header: fi;
# Don't include "libvirt/*.h" in "" form. -sc_prohibit_include_public_headers: +sc_prohibit_include_public_headers_quote: @prohibit='# *include *"libvirt/.*\.h"' \ in_vc_files='\.[chx]$$' \ halt='Do not include libvirt/*.h in internal source' \ $(_sc_search_regexp)
+# Don't include "libvirt/*.h" in <> form. Except for external tools, +# e.g. Python binding, examples and tools subdirectories. +sc_prohibit_include_public_headers_brackets: + @prohibit='# *include *<libvirt/.*\.h>' \ + in_vc_files='\.[chx]$$' \
Change this to [ch] (we found out this morning that [chx] is too strict, if 'dwarves' is installed). ACK with that tweak. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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
On 04/17/2013 04:19 AM, Osier Yang wrote:
To tell libvirt-{qemu,lxc}.h shouldn't be included either. --- docs/hacking.html.in | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
You also need to run 'make HACKING' and check that in at the same time. (Yes, that's one of the few generated files we actually version control).
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.
s/and/or/
+ It is included by "internal.h" already and there are some special reasons
s/It is/They are/
+ why you cannot include these files explicitly.
Is it worth mentioning the visibility of *_LAST enums as one of these reasons? ACK with the grammar fix and generated file; up to you whether to further document the "why". -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 18/04/13 04:25, Eric Blake wrote:
On 04/17/2013 04:19 AM, Osier Yang wrote:
To tell libvirt-{qemu,lxc}.h shouldn't be included either. --- docs/hacking.html.in | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) You also need to run 'make HACKING' and check that in at the same time. (Yes, that's one of the few generated files we actually version control).
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. s/and/or/
+ It is included by "internal.h" already and there are some special reasons s/It is/They are/
+ why you cannot include these files explicitly. Is it worth mentioning the visibility of *_LAST enums as one of these reasons?
It's actually an exception (special case), not a reason. But good to list it here. I added it like: One of special cases, "libvirt/libvirt.h" is included prior to "internal.h" in "remote_protocol.x", to avoid exposing *_LAST enum elements.
ACK with the grammar fix and generated file; up to you whether to further document the "why".
gnulib is excluded. --- cfg.mk | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/cfg.mk b/cfg.mk index 71f7ee4..dd67816 100644 --- a/cfg.mk +++ b/cfg.mk @@ -724,25 +724,27 @@ sc_prohibit_exit_in_tests: # 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]++; \ + @fail=0; for i in $$($(VC_LIST_EXCEPT) | grep '\.[chx]$$'); do \ + awk '/# *include.*\.h/ { \ + match($$0, /[<"][^>"]*[">]/); \ + arr[substr($$0, RSTART + 1, RLENGTH - 2)]++; \ } \ END { \ for (key in arr) { \ - if (arr[key] > 1) { \ + if (arr[key] > 1) { \ fail=1; \ printf("%d %s are included\n", arr[key], key); \ } \ } \ - if (fail == 1) \ + if (fail == 1) { \ + printf("duplicate header(s) in " FILENAME "\n"); \ exit 1; \ - }' $$i || { echo "Duplicate header(s) in $$i"; exit 1; }; \ - done + } \ + }' $$i || fail=1; \ + done; \ + if test $$fail -eq 1; then \ + { echo "$(ME)": avoid duplicate headers >&2; exit 1; } \ + fi; # We don't use this feature of maint.mk. prev_version_file = /dev/null -- 1.8.1.4
Oops, ignore this please, it's on top of the old commit. On 17/04/13 18:21, Osier Yang wrote:
gnulib is excluded. --- cfg.mk | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/cfg.mk b/cfg.mk index 71f7ee4..dd67816 100644 --- a/cfg.mk +++ b/cfg.mk @@ -724,25 +724,27 @@ sc_prohibit_exit_in_tests:
# 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]++; \ + @fail=0; for i in $$($(VC_LIST_EXCEPT) | grep '\.[chx]$$'); do \ + awk '/# *include.*\.h/ { \ + match($$0, /[<"][^>"]*[">]/); \ + arr[substr($$0, RSTART + 1, RLENGTH - 2)]++; \ } \ END { \ for (key in arr) { \ - if (arr[key] > 1) { \ + if (arr[key] > 1) { \ fail=1; \ printf("%d %s are included\n", arr[key], key); \ } \ } \ - if (fail == 1) \ + if (fail == 1) { \ + printf("duplicate header(s) in " FILENAME "\n"); \ exit 1; \ - }' $$i || { echo "Duplicate header(s) in $$i"; exit 1; }; \ - done + } \ + }' $$i || fail=1; \ + done; \ + if test $$fail -eq 1; then \ + { echo "$(ME)": avoid duplicate headers >&2; exit 1; } \ + fi;
# We don't use this feature of maint.mk. prev_version_file = /dev/null
gnulib is excluded. --- cfg.mk | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/cfg.mk b/cfg.mk index e60c4e3..dd67816 100644 --- a/cfg.mk +++ b/cfg.mk @@ -722,6 +722,30 @@ 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: + @fail=0; for i in $$($(VC_LIST_EXCEPT) | grep '\.[chx]$$'); do \ + awk '/# *include.*\.h/ { \ + match($$0, /[<"][^>"]*[">]/); \ + arr[substr($$0, RSTART + 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) { \ + printf("duplicate header(s) in " FILENAME "\n"); \ + exit 1; \ + } \ + }' $$i || fail=1; \ + done; \ + if test $$fail -eq 1; then \ + { echo "$(ME)": avoid duplicate headers >&2; exit 1; } \ + fi; + # We don't use this feature of maint.mk. prev_version_file = /dev/null -- 1.8.1.4
On 04/17/2013 04:25 AM, Osier Yang wrote:
gnulib is excluded. --- cfg.mk | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)
Looks like this patch is supposed to come before the patch named [1/3] syntax-check: Don't include public headers in internal source
diff --git a/cfg.mk b/cfg.mk index e60c4e3..dd67816 100644 --- a/cfg.mk +++ b/cfg.mk @@ -722,6 +722,30 @@ 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: + @fail=0; for i in $$($(VC_LIST_EXCEPT) | grep '\.[chx]$$'); do \ + awk '/# *include.*\.h/ { \ + match($$0, /[<"][^>"]*[">]/); \ + arr[substr($$0, RSTART + 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) { \ + printf("duplicate header(s) in " FILENAME "\n"); \ + exit 1; \ + } \ + }' $$i || fail=1; \ + done; \ + if test $$fail -eq 1; then \ + { echo "$(ME)": avoid duplicate headers >&2; exit 1; } \ + fi; +
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 17/04/13 18:19, Osier Yang wrote:
These 3 patches are the left ones in v2. 6/10 ~ 9/10 in v2 are merged into one single patch.
Osier Yang (3): syntax-check: Don't include public headers in internal source syntax-check: Only allows to include public headers in external tools docs: Update HACKING
cfg.mk | 21 +++++++++++++++++++++ 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/libvirt-qemu.c | 1 - tests/shunloadhelper.c | 5 ++--- tools/virsh.c | 4 ++-- 12 files changed, 39 insertions(+), 22 deletions(-)
Pushed the set with nits fixed, thanks for the reviewing.. Osier
participants (2)
-
Eric Blake -
Osier Yang