10:32 a.m.
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
10:32 a.m.
New subject: [libvirt] [PATCH 1/4] util: Fix 'exempt from syntax-check' comment
---
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
10:32 a.m.
New subject: [libvirt] [PATCH 2/4] cfg.mk: Remove spurious whitespace
---
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
1:54 a.m.
New subject: [libvirt] [PATCH 2/4] cfg.mk: Remove spurious whitespace
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
4:51 a.m.
New subject: [libvirt] [PATCH 2/4] cfg.mk: Remove spurious whitespace
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
10:32 a.m.
New subject: [libvirt] [PATCH 3/4] cfg.mk: Use single quotes wherever possible
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
1:50 a.m.
New subject: [libvirt] [PATCH 3/4] cfg.mk: Use single quotes wherever possible
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
10:32 a.m.
New subject: [libvirt] [PATCH 4/4] cfg.mk: Always enclose syntax-check error messages in quotes
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
1:47 a.m.
New subject: [libvirt] [PATCH 4/4] cfg.mk: Always enclose syntax-check error messages in quotes
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.
4:37 a.m.
New subject: [libvirt] [PATCH 4/4] cfg.mk: Always enclose syntax-check error messages in quotes
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
4:40 a.m.
New subject: [libvirt] [PATCH] cfg.mk: Get rid of quotation tricks
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
9 a.m.
New subject: [libvirt] [PATCH] cfg.mk: Get rid of quotation tricks
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
9:06 a.m.
New subject: [libvirt] [PATCH] cfg.mk: Get rid of quotation tricks
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
3145
days inactive
3149
days old
13 comments
2 participants
participants (2)
-
Andrea Bolognani -
Peter Krempa