Re: [libvirt PATCH 23/23] build: add syntax-check rules for undesirable terms
Is this patch really necessary?
Manually removing the now undesirable terms it's okay (I mean, not quite,
but I'm too tired of this same discussion over and over again everywhere,
so I give up), because at least it requires a bit of thought and analysis.
Prohibiting the words altogether will eliminate valid use. With this patch
I'll now be forbid to write a comment such as
/* This is code is needed because module X is in the blacklist */
Unless I'm doing a comment in qemu_domain.c or virkmod.c. This is extreme, and
I don't think the existing Libvirt syntax-check mechanics will be able to detect
allowed usage of these terms.
IMO this patch should be dropped. Instead, these new "undesirable terms"
directions
should (in really, must) be included as documentation in hacking.rst, since we're
now serious about all the offending terms stuff, and each new eventual case evaluated
separately during code review. Yes, something will eventually go through, but then
one that cares enough can simply remove the undesirable word with a patch. This is
way saner than annoying the developer with syntax-check rules for uses of a word
that we actually allow, but it happened to be in a different file.
Thanks,
DHB
On 6/19/20 6:33 AM, Daniel P. Berrangé wrote:
We don't check for "master", because there are too
many
cases that we're not trying to eliminate at this time.
Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com>
---
build-aux/syntax-check.mk | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk
index 2e49b5172e..50b15e114a 100644
--- a/build-aux/syntax-check.mk
+++ b/build-aux/syntax-check.mk
@@ -1922,6 +1922,16 @@ sc_prohibit_path_max_allocation:
halt='Avoid stack allocations of size PATH_MAX' \
$(_sc_search_regexp)
+sc_prohibit_whitelist_blacklist:
+ @prohibit='(white-?list|black-?list)' \
+ halt='Avoid the terms "whitelist" and "blacklist"' \
+ $(_sc_search_regexp)
+
+sc_prohibit_slave:
+ @prohibit='slave' \
+ halt='Avoid the term "slave"' \
+ $(_sc_search_regexp)
+
ifneq ($(_gl-Makefile),)
syntax-check: spacing-check test-wrap-argv \
prohibit-duplicate-header mock-noinline group-qemu-caps \
@@ -2128,3 +2138,9 @@ exclude_file_name_regexp--sc_prohibit_backslash_alignment = \
exclude_file_name_regexp--sc_prohibit_select = \
^build-aux/syntax-check\.mk|src/util/vireventglibwatch\.c$$
+
+exclude_file_name_regexp--sc_prohibit_whitelist_blacklist = \
+
^(build-aux/syntax-check\.mk|po/.*\.pot?|src/util/virkmod\.c|src/qemu/qemu_domain\.c)$$
+
+exclude_file_name_regexp--sc_prohibit_slave = \
+
^build-aux/syntax-check\.mk|po/.*\.pot?|tests/qemucapabilitiesdata/.*\.replies|docs/apps.html.in|tests/bhyve.*|docs/drvbhyve.html.in|docs/formatdomain.html.in|docs/schemas/domaincommon.rng|src/conf/domain_conf\.c|src/interface/interface_backend_udev\.c|src/security/apparmor/usr\.sbin\.libvirtd\.in$$