Re: [libvirt PATCH 23/23] build: add syntax-check rules for undesirable terms

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx>
---
  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$$





[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux