[libvirt] [PATCH] util: remove unneeded #include in virrandom.c
Commit 7c90026 added #include "conf/domain_conf.h" to util/virrandom.c. Fortunately it didn't actually use anything from domain_conf.h, since as far as I'm aware, files in util aren't allowed to reference anything in conf (although the opposite is allowed). So this #include is unnecessary. I verified it still compiles with the line removed, but have placed a one day moratorium on me doing any "trivial rule" pushes, so will wait for someone else to verify/ACK before pushing. --- src/util/virrandom.c | 1 - 1 files changed, 0 insertions(+), 1 deletions(-) diff --git a/src/util/virrandom.c b/src/util/virrandom.c index 151cf4b..9092fd2 100644 --- a/src/util/virrandom.c +++ b/src/util/virrandom.c @@ -29,7 +29,6 @@ #include "count-one-bits.h" #include "util.h" #include "virterror_internal.h" -#include "conf/domain_conf.h" #define VIR_FROM_THIS VIR_FROM_NONE -- 1.7.7.6
On Thu, Mar 01, 2012 at 11:43:04AM -0500, Laine Stump wrote:
Commit 7c90026 added #include "conf/domain_conf.h" to util/virrandom.c. Fortunately it didn't actually use anything from domain_conf.h, since as far as I'm aware, files in util aren't allowed to reference anything in conf (although the opposite is allowed). So this #include is unnecessary.
That is correct. util/ must be self-contained
I verified it still compiles with the line removed, but have placed a one day moratorium on me doing any "trivial rule" pushes, so will wait for someone else to verify/ACK before pushing. --- src/util/virrandom.c | 1 - 1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/src/util/virrandom.c b/src/util/virrandom.c index 151cf4b..9092fd2 100644 --- a/src/util/virrandom.c +++ b/src/util/virrandom.c @@ -29,7 +29,6 @@ #include "count-one-bits.h" #include "util.h" #include "virterror_internal.h" -#include "conf/domain_conf.h"
#define VIR_FROM_THIS VIR_FROM_NONE
ACK, this is definitely a bogus include 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 03/01/2012 09:53 AM, Daniel P. Berrange wrote:
On Thu, Mar 01, 2012 at 11:43:04AM -0500, Laine Stump wrote:
Commit 7c90026 added #include "conf/domain_conf.h" to util/virrandom.c. Fortunately it didn't actually use anything from domain_conf.h, since as far as I'm aware, files in util aren't allowed to reference anything in conf (although the opposite is allowed). So this #include is unnecessary.
That is correct. util/ must be self-contained
Guess what - that sounds like a great syntax rule to write, so we don't slip up in the future. Give me a few minutes, to see what I can come up with. Any other directories that should be avoiding particular includes? -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Thu, Mar 01, 2012 at 10:25:11AM -0700, Eric Blake wrote:
On 03/01/2012 09:53 AM, Daniel P. Berrange wrote:
On Thu, Mar 01, 2012 at 11:43:04AM -0500, Laine Stump wrote:
Commit 7c90026 added #include "conf/domain_conf.h" to util/virrandom.c. Fortunately it didn't actually use anything from domain_conf.h, since as far as I'm aware, files in util aren't allowed to reference anything in conf (although the opposite is allowed). So this #include is unnecessary.
That is correct. util/ must be self-contained
Guess what - that sounds like a great syntax rule to write, so we don't slip up in the future. Give me a few minutes, to see what I can come up with. Any other directories that should be avoiding particular includes?
conf/ should only depend on util/ locking/ & security/ should only depend on conf/ & util/ And esx/hyperv/interface/network/etc should not depend on each other Regards, 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 :|
Make it easier to detect invalid cross-directory includes, by adding a syntax check. The check is designed to be extensible: the default case lists only the non-driver directories, and specific directories can list a different set (for example, util/ can only use itself, network/ can only use itself, util/, or conf/). * .gnulib: Update to latest, for syntax check improvment. * cfg.mk (sc_prohibit_cross_inclusion): New check. (sc_prohibit_strncmp, sc_libvirt_unmarked_diagnostics): Simplify. --- * .gnulib b856fad...44de969 (31):
maint.mk: avoid spurious failure of _sc_search_regexp-using tests maint.mk: add per-line exclusions to prohibitions Tests for module 'expl-ieee'. New module 'expl-ieee'. Tests for module 'exp-ieee'. New module 'exp-ieee'. Tests for module 'expf-ieee'. New module 'expf-ieee'. cbrtl-ieee: Work around test failure on IRIX 6.5. Tests for module 'cbrtl-ieee'. New module 'cbrtl-ieee'. Tests for module 'cbrt-ieee'. New module 'cbrt-ieee'. Tests for module 'cbrtf-ieee'. New module 'cbrtf-ieee'. cbrtf: Work around bug in IRIX 6.5 system function. Tests for module 'cbrtl'. New module 'cbrtl'. Tests for module 'cbrtf'. New module 'cbrtf'. cbrt: Provide replacement on MSVC and Minix. hypotl-ieee: Work around test failure on OSF/1 and native Windows. hypotf-ieee: Work around test failure on OSF/1 and native Windows. hypot-ieee: Work around test failure on OSF/1 and native Windows. Tests for module 'hypotl-ieee'. New module 'hypotl-ieee'. Tests for module 'hypot-ieee'. New module 'hypot-ieee'. Tests for module 'hypotf-ieee'. New module 'hypotf-ieee'. Remove unused variables.
.gnulib | 2 +- cfg.mk | 38 ++++++++++++++++++++++++++++---------- 2 files changed, 29 insertions(+), 11 deletions(-) diff --git a/.gnulib b/.gnulib index b856fad..44de969 160000 --- a/.gnulib +++ b/.gnulib @@ -1 +1 @@ -Subproject commit b856fadc1c8dcb53e7efcbb2d0ae7edc022fdb6a +Subproject commit 44de969cd62abbfe3e7cc7641a8dea7673fd2d6d diff --git a/cfg.mk b/cfg.mk index ac6c527..95994df 100644 --- a/cfg.mk +++ b/cfg.mk @@ -348,11 +348,10 @@ sc_prohibit_access_xok: # Use STREQLEN or STRPREFIX rather than comparing strncmp == 0, or != 0. snp_ = strncmp *\(.+\) sc_prohibit_strncmp: - @grep -nE '! *strncmp *\(|\<$(snp_) *[!=]=|[!=]= *$(snp_)' \ - $$($(VC_LIST_EXCEPT)) \ - | grep -vE ':# *define STR(N?EQLEN|PREFIX)\(' && \ - { echo '$(ME): use STREQLEN or STRPREFIX instead of str''ncmp' \ - 1>&2; exit 1; } || : + @prohibit='! *strncmp *\(|\<$(snp_) *[!=]=|[!=]= *$(snp_)' \ + exclude=':# *define STR(N?EQLEN|PREFIX)\(' \ + halt='$(ME): use STREQLEN or STRPREFIX instead of str''ncmp' \ + $(_sc_search_regexp) # Use virAsprintf rather than as'printf since *strp is undefined on error. sc_prohibit_asprintf: @@ -569,11 +568,10 @@ func_re := ($(func_or)) # _("...: " # "%s", _("no storage vol w..." sc_libvirt_unmarked_diagnostics: - @grep -nE \ - '\<$(func_re) *\([^"]*"[^"]*[a-z]{3}' $$($(VC_LIST_EXCEPT)) \ - | grep -v '_''(' && \ - { echo '$(ME): found unmarked diagnostic(s)' 1>&2; \ - exit 1; } || : + @prohibit='\<$(func_re) *\([^"]*"[^"]*[a-z]{3}' \ + exclude='_\(' \ + halt='$(ME): found unmarked diagnostic(s)' \ + $(_sc_search_regexp) @{ grep -nE '\<$(func_re) *\(.*;$$' $$($(VC_LIST_EXCEPT)); \ grep -A1 -nE '\<$(func_re) *\(.*,$$' $$($(VC_LIST_EXCEPT)); } \ | sed 's/_("[^"][^"]*"//;s/[ ]"%s"//' \ @@ -624,6 +622,26 @@ sc_prohibit_gettext_markup: halt='do not mark these strings for translation' \ $(_sc_search_regexp) +# Our code is divided into modular subdirectories for a reason, and +# lower-level code must not include higher-level headers. +cross_dirs=$(patsubst $(srcdir)/src/%.,%,$(wildcard $(srcdir)/src/*/.)) +cross_dirs_re=($(subst / ,/|,$(cross_dirs))) +sc_prohibit_cross_inclusion: + @for dir in $(cross_dirs); do \ + case $$dir in \ + util/) safe="util";; \ + cpu/ | locking/ | network/ | rpc/ | security/) \ + safe="($$dir|util|conf)";; \ + xenapi/ | xenxs/ ) safe="($$dir|util|conf|xen)";; \ + *) safe="($$dir|util|conf|cpu|network|locking|rpc|security)";; \ + esac; \ + in_vc_files="^src/$$dir" \ + prohibit='^# *include .$(cross_dirs_re)' \ + exclude="# *include .$$safe" \ + halt='unsafe cross-directory include' \ + $(_sc_search_regexp) \ + done + # When converting an enum to a string, make sure that we track any new # elements added to the enum by using a _LAST marker. sc_require_enum_last_marker: -- 1.7.7.6
On Thu, Mar 01, 2012 at 05:40:19PM -0700, Eric Blake wrote:
Make it easier to detect invalid cross-directory includes, by adding a syntax check. The check is designed to be extensible: the default case lists only the non-driver directories, and specific directories can list a different set (for example, util/ can only use itself, network/ can only use itself, util/, or conf/).
* .gnulib: Update to latest, for syntax check improvment. * cfg.mk (sc_prohibit_cross_inclusion): New check. (sc_prohibit_strncmp, sc_libvirt_unmarked_diagnostics): Simplify. ---
* .gnulib b856fad...44de969 (31):
maint.mk: avoid spurious failure of _sc_search_regexp-using tests maint.mk: add per-line exclusions to prohibitions Tests for module 'expl-ieee'. New module 'expl-ieee'. Tests for module 'exp-ieee'. New module 'exp-ieee'. Tests for module 'expf-ieee'. New module 'expf-ieee'. cbrtl-ieee: Work around test failure on IRIX 6.5. Tests for module 'cbrtl-ieee'. New module 'cbrtl-ieee'. Tests for module 'cbrt-ieee'. New module 'cbrt-ieee'. Tests for module 'cbrtf-ieee'. New module 'cbrtf-ieee'. cbrtf: Work around bug in IRIX 6.5 system function. Tests for module 'cbrtl'. New module 'cbrtl'. Tests for module 'cbrtf'. New module 'cbrtf'. cbrt: Provide replacement on MSVC and Minix. hypotl-ieee: Work around test failure on OSF/1 and native Windows. hypotf-ieee: Work around test failure on OSF/1 and native Windows. hypot-ieee: Work around test failure on OSF/1 and native Windows. Tests for module 'hypotl-ieee'. New module 'hypotl-ieee'. Tests for module 'hypot-ieee'. New module 'hypot-ieee'. Tests for module 'hypotf-ieee'. New module 'hypotf-ieee'. Remove unused variables.
.gnulib | 2 +- cfg.mk | 38 ++++++++++++++++++++++++++++---------- 2 files changed, 29 insertions(+), 11 deletions(-)
diff --git a/.gnulib b/.gnulib index b856fad..44de969 160000 --- a/.gnulib +++ b/.gnulib @@ -1 +1 @@ -Subproject commit b856fadc1c8dcb53e7efcbb2d0ae7edc022fdb6a +Subproject commit 44de969cd62abbfe3e7cc7641a8dea7673fd2d6d diff --git a/cfg.mk b/cfg.mk index ac6c527..95994df 100644 --- a/cfg.mk +++ b/cfg.mk @@ -348,11 +348,10 @@ sc_prohibit_access_xok: # Use STREQLEN or STRPREFIX rather than comparing strncmp == 0, or != 0. snp_ = strncmp *\(.+\) sc_prohibit_strncmp: - @grep -nE '! *strncmp *\(|\<$(snp_) *[!=]=|[!=]= *$(snp_)' \ - $$($(VC_LIST_EXCEPT)) \ - | grep -vE ':# *define STR(N?EQLEN|PREFIX)\(' && \ - { echo '$(ME): use STREQLEN or STRPREFIX instead of str''ncmp' \ - 1>&2; exit 1; } || : + @prohibit='! *strncmp *\(|\<$(snp_) *[!=]=|[!=]= *$(snp_)' \ + exclude=':# *define STR(N?EQLEN|PREFIX)\(' \ + halt='$(ME): use STREQLEN or STRPREFIX instead of str''ncmp' \ + $(_sc_search_regexp)
# Use virAsprintf rather than as'printf since *strp is undefined on error. sc_prohibit_asprintf: @@ -569,11 +568,10 @@ func_re := ($(func_or)) # _("...: " # "%s", _("no storage vol w..." sc_libvirt_unmarked_diagnostics: - @grep -nE \ - '\<$(func_re) *\([^"]*"[^"]*[a-z]{3}' $$($(VC_LIST_EXCEPT)) \ - | grep -v '_''(' && \ - { echo '$(ME): found unmarked diagnostic(s)' 1>&2; \ - exit 1; } || : + @prohibit='\<$(func_re) *\([^"]*"[^"]*[a-z]{3}' \ + exclude='_\(' \ + halt='$(ME): found unmarked diagnostic(s)' \ + $(_sc_search_regexp) @{ grep -nE '\<$(func_re) *\(.*;$$' $$($(VC_LIST_EXCEPT)); \ grep -A1 -nE '\<$(func_re) *\(.*,$$' $$($(VC_LIST_EXCEPT)); } \ | sed 's/_("[^"][^"]*"//;s/[ ]"%s"//' \ @@ -624,6 +622,26 @@ sc_prohibit_gettext_markup: halt='do not mark these strings for translation' \ $(_sc_search_regexp)
+# Our code is divided into modular subdirectories for a reason, and +# lower-level code must not include higher-level headers. +cross_dirs=$(patsubst $(srcdir)/src/%.,%,$(wildcard $(srcdir)/src/*/.)) +cross_dirs_re=($(subst / ,/|,$(cross_dirs))) +sc_prohibit_cross_inclusion: + @for dir in $(cross_dirs); do \ + case $$dir in \ + util/) safe="util";; \ + cpu/ | locking/ | network/ | rpc/ | security/) \ + safe="($$dir|util|conf)";; \ + xenapi/ | xenxs/ ) safe="($$dir|util|conf|xen)";; \ + *) safe="($$dir|util|conf|cpu|network|locking|rpc|security)";; \ + esac; \ + in_vc_files="^src/$$dir" \ + prohibit='^# *include .$(cross_dirs_re)' \ + exclude="# *include .$$safe" \ + halt='unsafe cross-directory include' \ + $(_sc_search_regexp) \ + done + # When converting an enum to a string, make sure that we track any new # elements added to the enum by using a _LAST marker. sc_require_enum_last_marker:
ACK this looks good to me 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 03/02/2012 03:01 AM, Daniel P. Berrange wrote:
On Thu, Mar 01, 2012 at 05:40:19PM -0700, Eric Blake wrote:
Make it easier to detect invalid cross-directory includes, by adding a syntax check. The check is designed to be extensible: the default case lists only the non-driver directories, and specific directories can list a different set (for example, util/ can only use itself, network/ can only use itself, util/, or conf/).
* .gnulib: Update to latest, for syntax check improvment. * cfg.mk (sc_prohibit_cross_inclusion): New check. (sc_prohibit_strncmp, sc_libvirt_unmarked_diagnostics): Simplify. ---
+# Our code is divided into modular subdirectories for a reason, and +# lower-level code must not include higher-level headers. +cross_dirs=$(patsubst $(srcdir)/src/%.,%,$(wildcard $(srcdir)/src/*/.)) +cross_dirs_re=($(subst / ,/|,$(cross_dirs))) +sc_prohibit_cross_inclusion: + @for dir in $(cross_dirs); do \ + case $$dir in \ + util/) safe="util";; \ + cpu/ | locking/ | network/ | rpc/ | security/) \ + safe="($$dir|util|conf)";; \ + xenapi/ | xenxs/ ) safe="($$dir|util|conf|xen)";; \ + *) safe="($$dir|util|conf|cpu|network|locking|rpc|security)";; \ + esac; \ + in_vc_files="^src/$$dir" \ + prohibit='^# *include .$(cross_dirs_re)' \ + exclude="# *include .$$safe" \ + halt='unsafe cross-directory include' \ + $(_sc_search_regexp) \ + done + # When converting an enum to a string, make sure that we track any new # elements added to the enum by using a _LAST marker. sc_require_enum_last_marker:
ACK this looks good to me
Thanks; pushed. Hmm, should we change things to drop the -Iutil and instead use #include "util/util.h", rather than the current #include "util.h", as a way to make things even more obvious which submodule various headers are coming from? But that would be a future patch, and doesn't need to hold up this one. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Fri, Mar 02, 2012 at 06:40:57AM -0700, Eric Blake wrote:
On 03/02/2012 03:01 AM, Daniel P. Berrange wrote:
On Thu, Mar 01, 2012 at 05:40:19PM -0700, Eric Blake wrote:
Make it easier to detect invalid cross-directory includes, by adding a syntax check. The check is designed to be extensible: the default case lists only the non-driver directories, and specific directories can list a different set (for example, util/ can only use itself, network/ can only use itself, util/, or conf/).
* .gnulib: Update to latest, for syntax check improvment. * cfg.mk (sc_prohibit_cross_inclusion): New check. (sc_prohibit_strncmp, sc_libvirt_unmarked_diagnostics): Simplify. ---
+# Our code is divided into modular subdirectories for a reason, and +# lower-level code must not include higher-level headers. +cross_dirs=$(patsubst $(srcdir)/src/%.,%,$(wildcard $(srcdir)/src/*/.)) +cross_dirs_re=($(subst / ,/|,$(cross_dirs))) +sc_prohibit_cross_inclusion: + @for dir in $(cross_dirs); do \ + case $$dir in \ + util/) safe="util";; \ + cpu/ | locking/ | network/ | rpc/ | security/) \ + safe="($$dir|util|conf)";; \ + xenapi/ | xenxs/ ) safe="($$dir|util|conf|xen)";; \ + *) safe="($$dir|util|conf|cpu|network|locking|rpc|security)";; \ + esac; \ + in_vc_files="^src/$$dir" \ + prohibit='^# *include .$(cross_dirs_re)' \ + exclude="# *include .$$safe" \ + halt='unsafe cross-directory include' \ + $(_sc_search_regexp) \ + done + # When converting an enum to a string, make sure that we track any new # elements added to the enum by using a _LAST marker. sc_require_enum_last_marker:
ACK this looks good to me
Thanks; pushed.
Hmm, should we change things to drop the -Iutil and instead use #include "util/util.h", rather than the current #include "util.h", as a way to make things even more obvious which submodule various headers are coming from? But that would be a future patch, and doesn't need to hold up this one.
Yeah, I think that could be nice - we've already been doing that with some of the newer dirs like locking/ and security/. 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 03/01/2012 07:40 PM, Eric Blake wrote:
Make it easier to detect invalid cross-directory includes, by adding a syntax check. The check is designed to be extensible: the default case lists only the non-driver directories, and specific directories can list a different set (for example, util/ can only use itself, network/ can only use itself, util/, or conf/).
* .gnulib: Update to latest, for syntax check improvment. * cfg.mk (sc_prohibit_cross_inclusion): New check. (sc_prohibit_strncmp, sc_libvirt_unmarked_diagnostics): Simplify. ---
+# Our code is divided into modular subdirectories for a reason, and +# lower-level code must not include higher-level headers. +cross_dirs=$(patsubst $(srcdir)/src/%.,%,$(wildcard $(srcdir)/src/*/.)) +cross_dirs_re=($(subst / ,/|,$(cross_dirs))) +sc_prohibit_cross_inclusion: + @for dir in $(cross_dirs); do \ + case $$dir in \ + util/) safe="util";; \ + cpu/ | locking/ | network/ | rpc/ | security/) \ + safe="($$dir|util|conf)";; \ + xenapi/ | xenxs/ ) safe="($$dir|util|conf|xen)";; \ + *) safe="($$dir|util|conf|cpu|network|locking|rpc|security)";; \ + esac; \ + in_vc_files="^src/$$dir" \ + prohibit='^# *include .$(cross_dirs_re)' \ + exclude="# *include .$$safe" \ + halt='unsafe cross-directory include' \
Should this maybe say "prohibited" instead of "un-safe"? BTW, I just did a full build with the new gnulib, and tried syntax-check - it did properly catch the problem that I had removed in the patch that started this discussion.
Eric Blake wrote:
On 03/01/2012 09:53 AM, Daniel P. Berrange wrote:
On Thu, Mar 01, 2012 at 11:43:04AM -0500, Laine Stump wrote:
Commit 7c90026 added #include "conf/domain_conf.h" to util/virrandom.c. Fortunately it didn't actually use anything from domain_conf.h, since as far as I'm aware, files in util aren't allowed to reference anything in conf (although the opposite is allowed). So this #include is unnecessary.
That is correct. util/ must be self-contained
Guess what - that sounds like a great syntax rule to write, so we don't slip up in the future. Give me a few minutes, to see what I can come up with. Any other directories that should be avoiding particular includes?
I noticed Laine's message, admired his control and wrote this, came back to reply and found your message. Laine, you're welcome to merge this into your commit. diff --git a/cfg.mk b/cfg.mk index ac6c527..ca6fe65 100644 --- a/cfg.mk +++ b/cfg.mk @@ -624,6 +624,13 @@ sc_prohibit_gettext_markup: halt='do not mark these strings for translation' \ $(_sc_search_regexp) +# One must not include conf/ headers from src/util/. +sc_prohibit_conf_inclusion_from_util: + @in_vc_files='^src/util/' \ + prohibit='^# *include "conf/' \ + halt='do not include conf/*.h from src/util/*' \ + $(_sc_search_regexp) + # When converting an enum to a string, make sure that we track any new # elements added to the enum by using a _LAST marker. sc_require_enum_last_marker:
On 03/01/2012 11:53 AM, Daniel P. Berrange wrote:
On Thu, Mar 01, 2012 at 11:43:04AM -0500, Laine Stump wrote:
Commit 7c90026 added #include "conf/domain_conf.h" to util/virrandom.c. Fortunately it didn't actually use anything from domain_conf.h, since as far as I'm aware, files in util aren't allowed to reference anything in conf (although the opposite is allowed). So this #include is unnecessary. That is correct. util/ must be self-contained
I verified it still compiles with the line removed, but have placed a one day moratorium on me doing any "trivial rule" pushes, so will wait for someone else to verify/ACK before pushing. --- src/util/virrandom.c | 1 - 1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/src/util/virrandom.c b/src/util/virrandom.c index 151cf4b..9092fd2 100644 --- a/src/util/virrandom.c +++ b/src/util/virrandom.c @@ -29,7 +29,6 @@ #include "count-one-bits.h" #include "util.h" #include "virterror_internal.h" -#include "conf/domain_conf.h"
#define VIR_FROM_THIS VIR_FROM_NONE ACK, this is definitely a bogus include
Thanks, pushed.
Daniel
participants (4)
-
Daniel P. Berrange -
Eric Blake -
Jim Meyering -
Laine Stump