[libvirt] [PATCH 0/4] syntax-check: Minor cleanups
Ran into something confusing while working on something unrelated, ended up making a few tiny cleanups. Cheers. Andrea Bolognani (4): util: Fix 'exempt from syntax-check' comment cfg.mk: Remove spurious whitespace cfg.mk: Use single quotes wherever possible cfg.mk: Always enclose syntax-check error messages in quotes cfg.mk | 36 ++++++++++++++++++------------------ src/util/virutil.c | 4 ++-- 2 files changed, 20 insertions(+), 20 deletions(-) -- 2.5.5
--- src/util/virutil.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/util/virutil.c b/src/util/virutil.c index b401f8d..f596d72 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -2479,7 +2479,7 @@ virParseOwnershipIds(const char *label, uid_t *uidPtr, gid_t *gidPtr) */ const char *virGetEnvBlockSUID(const char *name) { - return secure_getenv(name); /* exempt from syntax-check-rules */ + return secure_getenv(name); /* exempt from syntax-check */ } @@ -2493,7 +2493,7 @@ const char *virGetEnvBlockSUID(const char *name) */ const char *virGetEnvAllowSUID(const char *name) { - return getenv(name); /* exempt from syntax-check-rules */ + return getenv(name); /* exempt from syntax-check */ } -- 2.5.5
--- There's one more potential instance on line 684 (sc_require_whitespace_in_translation), but in that specific case I'm not 100% sure the whitespace is not supposed to be there. cfg.mk | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cfg.mk b/cfg.mk index 8e8586f..4edbd02 100644 --- a/cfg.mk +++ b/cfg.mk @@ -578,7 +578,7 @@ sc_prohibit_int_ijk: sc_prohibit_loop_iijjkk: @prohibit='\<(int|unsigned) ([^=]+ )*(ii|jj|kk)\>(\s|,|;)' \ - halt='use i, j, k for loop iterators, not ii, jj, kk' \ + halt='use i, j, k for loop iterators, not ii, jj, kk' \ $(_sc_search_regexp) # RHEL 5 gcc can't grok "for (int i..." -- 2.5.5
On Mon, Apr 11, 2016 at 17:32:44 +0200, Andrea Bolognani wrote:
--- There's one more potential instance on line 684 (sc_require_whitespace_in_translation), but in that specific case I'm not 100% sure the whitespace is not supposed to be there.
I've tried to delete it and that check still works, so it looks it's not necessary.
cfg.mk | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
ACK
On Tue, 2016-04-12 at 08:54 +0200, Peter Krempa wrote:
On Mon, Apr 11, 2016 at 17:32:44 +0200, Andrea Bolognani wrote:
--- There's one more potential instance on line 684 (sc_require_whitespace_in_translation), but in that specific case I'm not 100% sure the whitespace is not supposed to be there.
I've tried to delete it and that check still works, so it looks it's not necessary.
Yeah, since writing that comment I've convinced myself that I could have safely deleted that whitespace as well. But I've already pushed the patch, so it'll have to be in a separate commit at some point in the future. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
Being consistent is nice, especially when it comes to defining our regular expression, where using single quotes instead of double quotes allows us to leave out a few backslashes. Changing this required altering a few error messages. The only remaining use of double quotes is one where they are actually required for the check to work. --- cfg.mk | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/cfg.mk b/cfg.mk index 4edbd02..cd3b515 100644 --- a/cfg.mk +++ b/cfg.mk @@ -454,20 +454,20 @@ sc_prohibit_nonreentrant: exit $$fail sc_prohibit_select: - @prohibit="\\<select *\\(" \ - halt="use poll(), not se""lect()" \ + @prohibit='\<select *\(' \ + halt='use poll(), not se''lect()' \ $(_sc_search_regexp) # Prohibit the inclusion of <ctype.h>. sc_prohibit_ctype_h: @prohibit='^# *include *<ctype\.h>' \ - halt="don't use ctype.h; instead, use c-ctype.h" \ + halt='use c-ctype.h instead of ctype.h' \ $(_sc_search_regexp) # Insist on correct types for [pug]id. sc_correct_id_types: @prohibit='\<(int|long) *[pug]id\>' \ - halt="use pid_t for pid, uid_t for uid, gid_t for gid" \ + halt='use pid_t for pid, uid_t for uid, gid_t for gid' \ $(_sc_search_regexp) # "const fooPtr a" is the same as "foo * const a", even though it is @@ -503,12 +503,12 @@ ctype_re = isalnum|isalpha|isascii|isblank|iscntrl|isdigit|isgraph|islower\ sc_avoid_ctype_macros: @prohibit='\b($(ctype_re)) *\(' \ - halt="don't use ctype macros (use c-ctype.h)" \ + halt='use c-ctype.h instead of ctype macros' \ $(_sc_search_regexp) sc_avoid_strcase: @prohibit='\bstrn?case(cmp|str) *\(' \ - halt="don't use raw strcase functions (use c-strcase instead)" \ + halt='use c-strcase.h instead of raw strcase functions' \ $(_sc_search_regexp) sc_prohibit_virBufferAdd_with_string_literal: @@ -810,7 +810,7 @@ sc_require_enum_last_marker: sc_prohibit_semicolon_at_eol_in_python: @prohibit='^[^#].*\;$$' \ in_vc_files='\.py$$' \ - halt="Don't use semicolon at eol in python files" \ + halt='python does not require to end lines with a semicolon' \ $(_sc_search_regexp) # mymain() in test files should use return, not exit, for nicer output @@ -917,7 +917,7 @@ sc_prohibit_virConnectOpen_in_virsh: sc_require_space_before_label: @prohibit='^( ?)?[_a-zA-Z0-9]+:$$' \ in_vc_files='\.[ch]$$' \ - halt="Top-level labels should be indented by one space" \ + halt='Top-level labels should be indented by one space' \ $(_sc_search_regexp) # Allow for up to three spaces before the label: this is to avoid running @@ -926,14 +926,14 @@ sc_require_space_before_label: sc_prohibit_space_in_label: @prohibit='^ {0,3}[_a-zA-Z0-9]+ +:$$' \ in_vc_files='\.[ch]$$' \ - halt="There should be no space between label name and colon" \ + halt='There should be no space between label name and colon' \ $(_sc_search_regexp) # Doesn't catch all cases of mismatched braces across if-else, but it helps sc_require_if_else_matching_braces: @prohibit='( else( if .*\))? {|} else( if .*\))?$$)' \ in_vc_files='\.[chx]$$' \ - halt="if one side of if-else uses {}, both sides must use it" \ + halt='if one side of if-else uses {}, both sides must use it' \ $(_sc_search_regexp) sc_curly_braces_style: @@ -993,7 +993,7 @@ sc_prohibit_static_zero_init: sc_prohibit_devname: @prohibit='\bdevname\b' \ exclude='sc_prohibit_devname' \ - halt='avoid using 'devname' as FreeBSD exports the symbol' \ + halt='avoid using devname as FreeBSD exports the symbol' \ $(_sc_search_regexp) sc_prohibit_system_error_with_vir_err: @@ -1007,7 +1007,7 @@ sc_prohibit_system_error_with_vir_err: sc_prohibit_virXXXFree: @prohibit='\bvir(Domain|Network|NodeDevice|StorageVol|StoragePool|Stream|Secret|NWFilter|Interface|DomainSnapshot)Free\b' \ exclude='sc_prohibit_virXXXFree' \ - halt='avoid using 'virXXXFree', use 'virObjectUnref' instead' \ + halt='avoid using virXXXFree, use virObjectUnref instead' \ $(_sc_search_regexp) sc_prohibit_sysconf_pagesize: @@ -1018,7 +1018,7 @@ sc_prohibit_sysconf_pagesize: sc_prohibit_pthread_create: @prohibit='\bpthread_create\b' \ exclude='sc_prohibit_pthread_create' \ - halt="avoid using 'pthread_create', use 'virThreadCreate' instead" \ + halt='avoid using pthread_create, use virThreadCreate instead' \ $(_sc_search_regexp) sc_prohibit_not_streq: -- 2.5.5
On Mon, Apr 11, 2016 at 17:32:45 +0200, Andrea Bolognani wrote:
Being consistent is nice, especially when it comes to defining our regular expression, where using single quotes instead of double quotes allows us to leave out a few backslashes.
Changing this required altering a few error messages.
The only remaining use of double quotes is one where they are actually required for the check to work. --- cfg.mk | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-)
ACK
To prevent the error messages in cfg.mk from triggering the very same rules they're supposed to explain, we split the message in the middle of a symbol name. In some cases we end up with something like 'I am a me'ssage However, using a little more punctuation and ensuring the message is properly enclosed in quotes is nicer. --- A more proper fix is arguably to exclude cfg.mk from these specific checks... It's not like we're going to use asprintf() in a Makefile anyway. cfg.mk | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/cfg.mk b/cfg.mk index cd3b515..15e7c40 100644 --- a/cfg.mk +++ b/cfg.mk @@ -379,7 +379,7 @@ sc_prohibit_strtol: # But for plain %s, virAsprintf is overkill compared to strdup. sc_prohibit_asprintf: @prohibit='\<v?a[s]printf\>' \ - halt='use virAsprintf, not as'printf \ + halt='use virAsprintf, not as''printf' \ $(_sc_search_regexp) @prohibit='virAsprintf.*, *"%s",' \ halt='use VIR_STRDUP instead of virAsprintf with "%s"' \ @@ -406,7 +406,7 @@ sc_prohibit_risky_id_promotion: # since gnulib has more guarantees for snprintf portability sc_prohibit_sprintf: @prohibit='\<[s]printf\>' \ - halt='use snprintf, not s'printf \ + halt='use snprintf, not s''printf' \ $(_sc_search_regexp) sc_prohibit_readlink: @@ -432,12 +432,12 @@ sc_prohibit_gettext_noop: sc_prohibit_VIR_ERR_NO_MEMORY: @prohibit='\<V''IR_ERR_NO_MEMORY\>' \ - halt='use virReportOOMError, not V'IR_ERR_NO_MEMORY \ + halt='use virReportOOMError, not V''IR_ERR_NO_MEMORY' \ $(_sc_search_regexp) sc_prohibit_PATH_MAX: @prohibit='\<P''ATH_MAX\>' \ - halt='dynamically allocate paths, do not use P'ATH_MAX \ + halt='dynamically allocate paths, do not use P''ATH_MAX' \ $(_sc_search_regexp) # Use a subshell for each function, to give the optimal warning message. -- 2.5.5
On Mon, Apr 11, 2016 at 17:32:46 +0200, Andrea Bolognani wrote:
To prevent the error messages in cfg.mk from triggering the very same rules they're supposed to explain, we split the message in the middle of a symbol name.
In some cases we end up with something like
'I am a me'ssage
However, using a little more punctuation and ensuring the message is properly enclosed in quotes is nicer. --- A more proper fix is arguably to exclude cfg.mk from these specific checks... It's not like we're going to use asprintf() in a Makefile anyway.
I've actually thought it's done this way ...
cfg.mk | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/cfg.mk b/cfg.mk index cd3b515..15e7c40 100644 --- a/cfg.mk +++ b/cfg.mk @@ -379,7 +379,7 @@ sc_prohibit_strtol: # But for plain %s, virAsprintf is overkill compared to strdup. sc_prohibit_asprintf: @prohibit='\<v?a[s]printf\>' \ - halt='use virAsprintf, not as'printf \ + halt='use virAsprintf, not as''printf' \
... until I saw this stuff. Yuck. ACK though since it works.
On Tue, 2016-04-12 at 08:47 +0200, Peter Krempa wrote:
On Mon, Apr 11, 2016 at 17:32:46 +0200, Andrea Bolognani wrote:
To prevent the error messages in cfg.mk from triggering the very same rules they're supposed to explain, we split the message in the middle of a symbol name.
In some cases we end up with something like
'I am a me'ssage
However, using a little more punctuation and ensuring the message is properly enclosed in quotes is nicer. --- A more proper fix is arguably to exclude cfg.mk from these specific checks... It's not like we're going to use asprintf() in a Makefile anyway.
I've actually thought it's done this way ...
I've pushed all other patches (thank for reviewing them!) but not this one since I wanted to give the other approach a go. I'll send the alternative version right away, please take a look at it. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
To prevent the error messages in cfg.mk from triggering the very same rules they're supposed to explain, we split the message in the middle of a symbol name, ending up with stuff like 'I am a me'ssage Instead of relying on these quotation tricks, simply exclude cfg.mk from the relevant checks. --- cfg.mk | 44 +++++++++++++++++++++++++++----------------- 1 file changed, 27 insertions(+), 17 deletions(-) diff --git a/cfg.mk b/cfg.mk index cd3b515..62d249a 100644 --- a/cfg.mk +++ b/cfg.mk @@ -308,7 +308,7 @@ sc_flags_usage: | grep -c '\(long\|unsigned\) flags')" != 4 && \ { echo '$(ME): new API should use "unsigned int flags"' 1>&2; \ exit 1; } || : - @prohibit=' flags ''ATTRIBUTE_UNUSED' \ + @prohibit=' flags ATTRIBUTE_UNUSED' \ halt='flags should be checked with virCheckFlags' \ $(_sc_search_regexp) @prohibit='^[^@]*([^d] (int|long long)|[^dg] long) flags[;,)]' \ @@ -351,8 +351,8 @@ sc_prohibit_mkstemp: # access with X_OK accepts directories, but we can't exec() those. # access with F_OK or R_OK is okay, though. sc_prohibit_access_xok: - @prohibit='access''(at)? *\(.*X_OK' \ - halt='use virFileIsExecutable instead of access''(,X_OK)' \ + @prohibit='access(at)? *\(.*X_OK' \ + halt='use virFileIsExecutable instead of access(,X_OK)' \ $(_sc_search_regexp) # Similar to the gnulib maint.mk rule for sc_prohibit_strcmp @@ -361,7 +361,7 @@ snp_ = strncmp *\(.+\) sc_prohibit_strncmp: @prohibit='! *strncmp *\(|\<$(snp_) *[!=]=|[!=]= *$(snp_)' \ exclude=':# *define STR(N?EQLEN|PREFIX)\(' \ - halt='use STREQLEN or STRPREFIX instead of str''ncmp' \ + halt='use STREQLEN or STRPREFIX instead of strncmp' \ $(_sc_search_regexp) # strtol and friends are too easy to misuse @@ -379,7 +379,7 @@ sc_prohibit_strtol: # But for plain %s, virAsprintf is overkill compared to strdup. sc_prohibit_asprintf: @prohibit='\<v?a[s]printf\>' \ - halt='use virAsprintf, not as'printf \ + halt='use virAsprintf, not asprintf' \ $(_sc_search_regexp) @prohibit='virAsprintf.*, *"%s",' \ halt='use VIR_STRDUP instead of virAsprintf with "%s"' \ @@ -406,7 +406,7 @@ sc_prohibit_risky_id_promotion: # since gnulib has more guarantees for snprintf portability sc_prohibit_sprintf: @prohibit='\<[s]printf\>' \ - halt='use snprintf, not s'printf \ + halt='use snprintf, not sprintf' \ $(_sc_search_regexp) sc_prohibit_readlink: @@ -431,13 +431,13 @@ sc_prohibit_gettext_noop: $(_sc_search_regexp) sc_prohibit_VIR_ERR_NO_MEMORY: - @prohibit='\<V''IR_ERR_NO_MEMORY\>' \ - halt='use virReportOOMError, not V'IR_ERR_NO_MEMORY \ + @prohibit='\<VIR_ERR_NO_MEMORY\>' \ + halt='use virReportOOMError, not VIR_ERR_NO_MEMORY' \ $(_sc_search_regexp) sc_prohibit_PATH_MAX: - @prohibit='\<P''ATH_MAX\>' \ - halt='dynamically allocate paths, do not use P'ATH_MAX \ + @prohibit='\<PATH_MAX\>' \ + halt='dynamically allocate paths, do not use PATH_MAX' \ $(_sc_search_regexp) # Use a subshell for each function, to give the optimal warning message. @@ -455,7 +455,7 @@ sc_prohibit_nonreentrant: sc_prohibit_select: @prohibit='\<select *\(' \ - halt='use poll(), not se''lect()' \ + halt='use poll(), not select()' \ $(_sc_search_regexp) # Prohibit the inclusion of <ctype.h>. @@ -743,7 +743,7 @@ sc_copyright_format: @prohibit='Copyright [^(].*Red 'Hat \ halt='consistently use (C) in Red Hat copyright' \ $(_sc_search_regexp) - @prohibit='\<Red''Hat\>' \ + @prohibit='\<RedHat\>' \ halt='spell Red Hat as two words' \ $(_sc_search_regexp) @@ -1135,11 +1135,14 @@ exclude_file_name_regexp--sc_avoid_write = \ exclude_file_name_regexp--sc_bindtextdomain = ^(tests|examples)/ +exclude_file_name_regexp--sc_copyright_format = \ + ^cfg\.mk$$ + exclude_file_name_regexp--sc_copyright_usage = \ ^COPYING(|\.LESSER)$$ exclude_file_name_regexp--sc_flags_usage = \ - ^(docs/|src/util/virnetdevtap\.c$$|tests/(vir(cgroup|pci|usb)|nss|qemuxml2argv)mock\.c$$) + ^(cfg\.mk|docs/|src/util/virnetdevtap\.c$$|tests/(vir(cgroup|pci|usb)|nss|qemuxml2argv)mock\.c$$) exclude_file_name_regexp--sc_libvirt_unmarked_diagnostics = \ ^(src/rpc/gendispatch\.pl$$|tests/) @@ -1147,12 +1150,16 @@ exclude_file_name_regexp--sc_libvirt_unmarked_diagnostics = \ exclude_file_name_regexp--sc_po_check = ^(docs/|src/rpc/gendispatch\.pl$$) exclude_file_name_regexp--sc_prohibit_VIR_ERR_NO_MEMORY = \ - ^(include/libvirt/virterror\.h|daemon/dispatch\.c|src/util/virerror\.c|docs/internals/oomtesting\.html\.in)$$ + ^(cfg\.mk|include/libvirt/virterror\.h|daemon/dispatch\.c|src/util/virerror\.c|docs/internals/oomtesting\.html\.in)$$ + +exclude_file_name_regexp--sc_prohibit_PATH_MAX = \ + ^cfg\.mk$$ -exclude_file_name_regexp--sc_prohibit_access_xok = ^src/util/virutil\.c$$ +exclude_file_name_regexp--sc_prohibit_access_xok = \ + ^(cfg\.mk|src/util/virutil\.c)$$ exclude_file_name_regexp--sc_prohibit_asprintf = \ - ^(bootstrap.conf$$|src/util/virstring\.[ch]$$|tests/vircgroupmock\.c$$) + ^(cfg\.mk|bootstrap.conf$$|src/util/virstring\.[ch]$$|tests/vircgroupmock\.c$$) exclude_file_name_regexp--sc_prohibit_strdup = \ ^(docs/|examples/|src/util/virstring\.c|tests/vir(netserverclient|cgroup)mock.c$$) @@ -1178,6 +1185,9 @@ exclude_file_name_regexp--sc_prohibit_newline_at_end_of_diagnostic = \ exclude_file_name_regexp--sc_prohibit_nonreentrant = \ ^((po|tests)/|docs/.*(py|html\.in)|run.in$$|tools/wireshark/util/genxdrstub\.pl$$) +exclude_file_name_regexp--sc_prohibit_select = \ + ^cfg\.mk$$ + exclude_file_name_regexp--sc_prohibit_raw_allocation = \ ^(docs/hacking\.html\.in|src/util/viralloc\.[ch]|examples/.*|tests/(securityselinuxhelper|vircgroupmock)\.c|tools/wireshark/src/packet-libvirt\.c)$$ @@ -1187,7 +1197,7 @@ exclude_file_name_regexp--sc_prohibit_readlink = \ exclude_file_name_regexp--sc_prohibit_setuid = ^src/util/virutil\.c$$ exclude_file_name_regexp--sc_prohibit_sprintf = \ - (^docs/hacking\.html\.in|\.stp|\.pl)$$ + ^(cfg\.mk|docs/hacking\.html\.in|.*\.stp|.*\.pl)$$ exclude_file_name_regexp--sc_prohibit_strncpy = ^src/util/virstring\.c$$ -- 2.5.5
On Tue, Apr 12, 2016 at 11:40:27 +0200, Andrea Bolognani wrote:
To prevent the error messages in cfg.mk from triggering the very same rules they're supposed to explain, we split the message in the middle of a symbol name, ending up with stuff like
'I am a me'ssage
Instead of relying on these quotation tricks, simply exclude cfg.mk from the relevant checks. --- cfg.mk | 44 +++++++++++++++++++++++++++----------------- 1 file changed, 27 insertions(+), 17 deletions(-)
ACK
On Fri, 2016-04-15 at 16:00 +0200, Peter Krempa wrote:
On Tue, Apr 12, 2016 at 11:40:27 +0200, Andrea Bolognani wrote:
To prevent the error messages in cfg.mk from triggering the very same rules they're supposed to explain, we split the message in the middle of a symbol name, ending up with stuff like
'I am a me'ssage
Instead of relying on these quotation tricks, simply exclude cfg.mk from the relevant checks. --- cfg.mk | 44 +++++++++++++++++++++++++++----------------- 1 file changed, 27 insertions(+), 17 deletions(-)
ACK
Pushed, thanks :) -- Andrea Bolognani Software Engineer - Virtualization Team
participants (2)
-
Andrea Bolognani -
Peter Krempa