[libvirt PATCH 00/23] remove use of terms 'whitelist'/'blacklist' and 'master'/'slave'

The terms "whitelist" / "blacklist" perpetuate the notion that white is good and black is bad[1]. Their usage is trivially eliminated from libvirt with a variety of alternative terms. The replacements are more applicable to the usage context in most cases. The only exceptions are • Libvirt needs to keep "seccomp-blacklist" back compat for running guest capabilities on upgrade • kmod uses "blacklist" for modprobe config file key The terms "master" / "slave" have inescapable historical context that makes them a bad choice for metaphors in software[1]. Again there are a variety of alternatives that can be used, many of which are more applicable to the usage context. Eliminating all usage though is not practical, due to libvirt's need to interface with external systems, where the terminology is part of the formal API. The unfixable exceptions are: • Libvirt uses slave=NNN master=NNN in XML schema for FreeBSD nmdm devs • Linux kernel sysfs uses "slave_$NIC" for bonding NIC members • Linux kernel mount options MS_SLAVE for mount propagation • QEMU uses "slave" in many device/property names • Jenkins website calls the libvirt plugin "libvirt-slave" • VirtualBox uses 'Slave' in many config file attributes Note, this doesn't attempt to remove cases of the word "master" which are used in isolation, only those paired with usage of the word "slave". Remaining usage of "master" needs evaluating, as some of these contexts are none the less implicitly associated with the "master/slave" concept. Nothing in the po/ directory is updated. This will be updated when we refresh translations at time of freeze. There should be no functional change in any of these patches with the exception of the patch tweaking matching for NICs in the interface driver. [1] There are many docs on the web covering this in detail, with one fairly clear description being: https://tools.ietf.org/id/draft-knodel-terminology-00.html Daniel P. Berrangé (23): scripts: remove use of the term 'whitelist' from build helpers rpc: remove use of the term 'whitelist' from RPC code cgroup: remove use of the term 'whitelist' from cgroup code qemu: remove use of the terms 'whitelist' and 'blacklist' from CPU code qemu: remove use of the term 'blacklist' in seccomp capability util: use short form -g arg to scsi_id docs: remove use of the term 'whitelist' from documentation util: rename method to virKModIsProhibited nodedev: remove use of the term 'blacklist' from enumeration code build: remove use of the term 'blacklist' from helper files src: remove use of the term 'whitelist' from remaining code interface: use a constant for the sysfs bond device file prefix interface: remove most use of the term 'slave' from bonding code docs: remove use of the term 'enslaved' wrt tap & bridge devices tools: remove use of the term 'slave' in code dealing with bridges util: remove use of the terms 'master' and 'slave' in PTY code qemu: remove use of the terms 'master' and 'slave' when iterating CPUs lxc: remove use of the terms 'master' and 'slave' in PTY setup docs: update link to the libvirt jenkins plugin docs: remove use of the term 'slave' in Jenkins agent docs conf: remove use of the terms 'master' and 'slave' in mndm config lxc: replace use of term 'slave' filesystem mount setup build: add syntax-check rules for undesirable terms build-aux/syntax-check.mk | 18 ++++- docs/apps.html.in | 6 +- docs/drvqemu.html.in | 12 ++-- docs/firewall.html.in | 6 +- docs/formatdomain.html.in | 4 +- docs/internals/rpc.html.in | 9 +-- docs/kbase/qemu-passthrough-security.rst | 3 +- docs/schemas/interface.rng | 2 +- m4/virt-compile-warnings.m4 | 2 +- scripts/check-aclrules.py | 8 +-- scripts/check-file-access.py | 16 ++--- scripts/mock-noinline.py | 1 - src/bhyve/bhyve_driver.c | 2 +- src/bhyve/bhyve_parse_command.c | 20 +++--- src/conf/domain_conf.c | 24 +++---- src/conf/domain_conf.h | 4 +- src/interface/interface_backend_udev.c | 65 +++++++++---------- src/libvirt.c | 2 +- src/libvirt_private.syms | 2 +- src/lxc/lxc_cgroup.c | 2 +- src/lxc/lxc_controller.c | 20 +++--- src/node_device/node_device_udev.c | 6 +- src/qemu/qemu.conf | 4 +- src/qemu/qemu_capabilities.c | 22 +++---- src/qemu/qemu_capabilities.h | 6 +- src/qemu/qemu_cgroup.c | 2 +- src/qemu/qemu_command.c | 6 +- src/qemu/qemu_conf.c | 4 +- src/qemu/qemu_domain.c | 10 ++- src/qemu/qemu_monitor.c | 46 ++++++------- src/remote/libvirtd.conf.in | 6 +- src/remote/remote_daemon_dispatch.c | 4 +- src/rpc/gendispatch.pl | 2 +- src/rpc/virnetsaslcontext.c | 10 +-- src/rpc/virnetsaslcontext.h | 2 +- src/rpc/virnettlscontext.c | 32 ++++----- src/rpc/virnettlscontext.h | 4 +- src/util/vircgroup.c | 2 +- src/util/virfile.c | 42 ++++++------ src/util/virkmod.c | 24 +++---- src/util/virkmod.h | 2 +- src/util/virnetdevtap.c | 2 +- src/util/virpci.c | 4 +- src/util/virprocess.c | 2 +- src/util/virstoragefile.c | 4 +- src/vbox/vbox_common.c | 4 +- tests/Makefile.am | 4 +- ...hitelist.txt => permitted_file_access.txt} | 6 +- .../caps_2.11.0.s390x.xml | 2 +- .../caps_2.11.0.x86_64.xml | 2 +- .../caps_2.12.0.aarch64.xml | 2 +- .../caps_2.12.0.ppc64.xml | 2 +- .../caps_2.12.0.s390x.xml | 2 +- .../caps_2.12.0.x86_64.xml | 2 +- .../qemucapabilitiesdata/caps_3.0.0.ppc64.xml | 2 +- .../caps_3.0.0.riscv32.xml | 2 +- .../caps_3.0.0.riscv64.xml | 2 +- .../qemucapabilitiesdata/caps_3.0.0.s390x.xml | 2 +- .../caps_3.0.0.x86_64.xml | 2 +- .../qemucapabilitiesdata/caps_3.1.0.ppc64.xml | 2 +- .../caps_3.1.0.x86_64.xml | 2 +- .../caps_4.0.0.aarch64.xml | 2 +- .../qemucapabilitiesdata/caps_4.0.0.ppc64.xml | 2 +- .../caps_4.0.0.riscv32.xml | 2 +- .../caps_4.0.0.riscv64.xml | 2 +- .../qemucapabilitiesdata/caps_4.0.0.s390x.xml | 2 +- .../caps_4.0.0.x86_64.xml | 2 +- .../caps_4.1.0.x86_64.xml | 2 +- .../caps_4.2.0.aarch64.xml | 2 +- .../qemucapabilitiesdata/caps_4.2.0.ppc64.xml | 2 +- .../qemucapabilitiesdata/caps_4.2.0.s390x.xml | 2 +- .../caps_4.2.0.x86_64.xml | 2 +- .../caps_5.0.0.aarch64.xml | 2 +- .../qemucapabilitiesdata/caps_5.0.0.ppc64.xml | 2 +- .../caps_5.0.0.riscv64.xml | 2 +- .../caps_5.0.0.x86_64.xml | 2 +- .../caps_5.1.0.x86_64.xml | 2 +- .../qemustatusxml2xmldata/backup-pull-in.xml | 2 +- .../blockjob-blockdev-in.xml | 2 +- tests/qemuxml2argvtest.c | 2 +- tests/virconfdata/libvirtd.conf | 6 +- tests/virconfdata/libvirtd.out | 6 +- tools/virsh-interface.c | 16 ++--- 83 files changed, 301 insertions(+), 281 deletions(-) rename tests/{file_access_whitelist.txt => permitted_file_access.txt} (82%) -- 2.24.1

The term "permitted list" is a better choice for the filtering logic applied. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- scripts/check-aclrules.py | 8 ++++---- scripts/check-file-access.py | 16 ++++++++-------- scripts/mock-noinline.py | 1 - tests/Makefile.am | 4 ++-- ...s_whitelist.txt => permitted_file_access.txt} | 6 +++--- 5 files changed, 17 insertions(+), 18 deletions(-) rename tests/{file_access_whitelist.txt => permitted_file_access.txt} (82%) diff --git a/scripts/check-aclrules.py b/scripts/check-aclrules.py index a1fa473174..2335e8cfdd 100755 --- a/scripts/check-aclrules.py +++ b/scripts/check-aclrules.py @@ -35,7 +35,7 @@ import re import sys -whitelist = { +permitted = { "connectClose": True, "connectIsEncrypted": True, "connectIsSecure": True, @@ -58,7 +58,7 @@ whitelist = { # XXX this vzDomainMigrateConfirm3Params looks # bogus - determine why it doesn't have a valid # ACL check. -implwhitelist = { +implpermitted = { "vzDomainMigrateConfirm3Params": True, } @@ -230,8 +230,8 @@ def process_file(filename): api not in ["no", "name"] and table != "virStateDriver"): if (impl not in acls and - api not in whitelist and - impl not in implwhitelist): + api not in permitted and + impl not in implpermitted): print(("%s:%d Missing ACL check in " + "function '%s' for '%s'") % (filename, lineno, impl, api), diff --git a/scripts/check-file-access.py b/scripts/check-file-access.py index dd39de2d79..aa120cafac 100755 --- a/scripts/check-file-access.py +++ b/scripts/check-file-access.py @@ -25,16 +25,16 @@ import re import sys if len(sys.argv) != 3: - print("syntax: %s ACCESS-FILE ACCESS-WHITELIST") + print("syntax: %s ACCESS-FILE PERMITTED-ACCESS-FILE") sys.exit(1) access_file = sys.argv[1] -whitelist_file = sys.argv[2] +permitted_file = sys.argv[2] known_actions = ["open", "fopen", "access", "stat", "lstat", "connect"] files = [] -whitelist = [] +permitted = [] with open(access_file, "r") as fh: for line in fh: @@ -52,7 +52,7 @@ with open(access_file, "r") as fh: else: raise Exception("Malformed line %s" % line) -with open(whitelist_file, "r") as fh: +with open(permitted_file, "r") as fh: for line in fh: line = line.rstrip("\n") @@ -70,7 +70,7 @@ with open(whitelist_file, "r") as fh: "progname": m.group(4), "testname": m.group(6), } - whitelist.append(rec) + permitted.append(rec) else: m = re.search(r'''^(\S*)(:\s*(\S*)(\s*:\s*(.*))?)?$''', line) if m is not None: @@ -81,18 +81,18 @@ with open(whitelist_file, "r") as fh: "progname": m.group(3), "testname": m.group(5), } - whitelist.append(rec) + permitted.append(rec) else: raise Exception("Malformed line %s" % line) -# Now we should check if %traces is included in $whitelist. For +# Now we should check if %traces is included in $permitted. For # now checking just keys is sufficient err = False for file in files: match = False - for rule in whitelist: + for rule in permitted: if not re.match("^" + rule["path"] + "$", file["path"]): continue diff --git a/scripts/mock-noinline.py b/scripts/mock-noinline.py index 4fc60c0be3..a8b7680c11 100644 --- a/scripts/mock-noinline.py +++ b/scripts/mock-noinline.py @@ -23,7 +23,6 @@ noninlined = {} mocked = {} # Functions in public header don't get the noinline annotation -# so whitelist them here noninlined["virEventAddTimeout"] = True # This one confuses the script as its defined in the mock file # but is actually just a local helper diff --git a/tests/Makefile.am b/tests/Makefile.am index 3505c40f42..65d1ceeefd 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -458,14 +458,14 @@ check-access: file-access-clean VIR_TEST_FILE_ACCESS=1 $(MAKE) $(AM_MAKEFLAGS) check $(RUNUTF8) $(PYTHON) $(top_srcdir)/scripts/check-file-access.py \ $(abs_builddir)/test_file_access.txt \ - $(abs_srcdir)/file_access_whitelist.txt | sort -u + $(abs_srcdir)/permitted_file_access.txt | sort -u file-access-clean: > test_file_access.txt endif WITH_LINUX EXTRA_DIST += \ - file_access_whitelist.txt + permitted_file_access.txt if WITH_TESTS noinst_PROGRAMS = $(test_programs) $(test_helpers) diff --git a/tests/file_access_whitelist.txt b/tests/permitted_file_access.txt similarity index 82% rename from tests/file_access_whitelist.txt rename to tests/permitted_file_access.txt index 5ec7ee63bb..52292d56be 100644 --- a/tests/file_access_whitelist.txt +++ b/tests/permitted_file_access.txt @@ -1,6 +1,6 @@ -# This is a whitelist that allows accesses to files not in our -# build directory nor source directory. The records are in the -# following formats: +# This is a list of files not in our build directory nor source +# directory which are permitted to be accessed by tests. The +# records are in the following formats: # # $path: $progname: $testname # $path: $action: $progname: $testname -- 2.24.1

On Fri, Jun 19, 2020 at 10:32:38 +0100, Daniel Berrange wrote:
The term "permitted list" is a better choice for the filtering logic applied.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- scripts/check-aclrules.py | 8 ++++---- scripts/check-file-access.py | 16 ++++++++-------- scripts/mock-noinline.py | 1 - tests/Makefile.am | 4 ++-- ...s_whitelist.txt => permitted_file_access.txt} | 6 +++--- 5 files changed, 17 insertions(+), 18 deletions(-) rename tests/{file_access_whitelist.txt => permitted_file_access.txt} (82%)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>

The term "access control list" better describes the concept involved. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- docs/internals/rpc.html.in | 9 ++++---- src/remote/libvirtd.conf.in | 6 +++--- src/remote/remote_daemon_dispatch.c | 4 ++-- src/rpc/virnetsaslcontext.c | 10 ++++----- src/rpc/virnetsaslcontext.h | 2 +- src/rpc/virnettlscontext.c | 32 ++++++++++++++--------------- src/rpc/virnettlscontext.h | 4 ++-- tests/virconfdata/libvirtd.conf | 6 +++--- tests/virconfdata/libvirtd.out | 6 +++--- 9 files changed, 40 insertions(+), 39 deletions(-) diff --git a/docs/internals/rpc.html.in b/docs/internals/rpc.html.in index 40d844f31c..129945bf1c 100644 --- a/docs/internals/rpc.html.in +++ b/docs/internals/rpc.html.in @@ -447,7 +447,8 @@ C <-- |32| 8 | 1 | 3 | 1 | 1 | 0 | .o.oOo | <-- S (reply) <dt><code>virNetSASLContextPtr</code> (virnetsaslcontext.h)</dt> <dd>The virNetSASLContext APIs maintain SASL state for a network service (server or client). This is primarily used on the server - to provide a whitelist of allowed SASL usernames for clients. + to provide an access control list of SASL usernames permitted as + clients. </dd> <dt><code>virNetSASLSessionPtr</code> (virnetsaslcontext.h)</dt> @@ -460,7 +461,7 @@ C <-- |32| 8 | 1 | 3 | 1 | 1 | 0 | .o.oOo | <-- S (reply) <dt><code>virNetTLSContextPtr</code> (virnettlscontext.h)</dt> <dd>The virNetTLSContext APIs maintain TLS state for a network service (server or client). This is primarily used on the server - to provide a whitelist of allowed x509 distinguished names, as + to provide an access control list of x509 distinguished names, as well as diffie-hellman keys. It can also do validation of x509 certificates prior to initiating a connection, in order to improve detection of configuration errors. @@ -760,8 +761,8 @@ C <-- |32| 8 | 1 | 3 | 1 | 1 | 0 | .o.oOo | <-- S (reply) next step is to decode the RPC header. The header is validated to ensure the request is sensible, ie the server should not receive a method reply from a client. If the client has not yet authenticated, - a security check is also applied to make sure the procedure is on the - whitelist of those allowed prior to auth. If the packet is a method + an access control list check is also performed to make sure the procedure + is one of those allowed prior to auth. If the packet is a method call, it will be placed on a global processing queue. The event loop thread is now done with the packet for the time being. </p> diff --git a/src/remote/libvirtd.conf.in b/src/remote/libvirtd.conf.in index 34741183cc..2607fbad86 100644 --- a/src/remote/libvirtd.conf.in +++ b/src/remote/libvirtd.conf.in @@ -253,11 +253,11 @@ # will be rejected. # # Default is to always verify. Uncommenting this will disable -# verification - make sure an IP whitelist is set +# verification. #tls_no_verify_certificate = 1 -# A whitelist of allowed x509 Distinguished Names +# An access control list of allowed x509 Distinguished Names # This list may contain wildcards such as # # "C=GB,ST=London,L=London,O=Red Hat,CN=*" @@ -282,7 +282,7 @@ @END@ -# A whitelist of allowed SASL usernames. The format for username +# An access control list of allowed SASL usernames. The format for username # depends on the SASL authentication mechanism. Kerberos usernames # look like username@REALM # diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c index 831e7d165c..67b86cff78 100644 --- a/src/remote/remote_daemon_dispatch.c +++ b/src/remote/remote_daemon_dispatch.c @@ -3861,7 +3861,7 @@ remoteDispatchAuthSaslStart(virNetServerPtr server, if (err == VIR_NET_SASL_CONTINUE) { ret->complete = 0; } else { - /* Check username whitelist ACL */ + /* Check username ACL */ if ((err = remoteSASLFinish(server, client)) < 0) { if (err == -2) goto authdeny; @@ -3957,7 +3957,7 @@ remoteDispatchAuthSaslStep(virNetServerPtr server, if (err == VIR_NET_SASL_CONTINUE) { ret->complete = 0; } else { - /* Check username whitelist ACL */ + /* Check username ACL */ if ((err = remoteSASLFinish(server, client)) < 0) { if (err == -2) goto authdeny; diff --git a/src/rpc/virnetsaslcontext.c b/src/rpc/virnetsaslcontext.c index e7ed8f4390..9253771787 100644 --- a/src/rpc/virnetsaslcontext.c +++ b/src/rpc/virnetsaslcontext.c @@ -36,7 +36,7 @@ VIR_LOG_INIT("rpc.netsaslcontext"); struct _virNetSASLContext { virObjectLockable parent; - const char *const*usernameWhitelist; + const char *const *usernameACL; }; struct _virNetSASLSession { @@ -121,7 +121,7 @@ virNetSASLContextPtr virNetSASLContextNewClient(void) return ctxt; } -virNetSASLContextPtr virNetSASLContextNewServer(const char *const*usernameWhitelist) +virNetSASLContextPtr virNetSASLContextNewServer(const char *const *usernameACL) { virNetSASLContextPtr ctxt; @@ -132,7 +132,7 @@ virNetSASLContextPtr virNetSASLContextNewServer(const char *const*usernameWhitel if (!(ctxt = virObjectLockableNew(virNetSASLContextClass))) return NULL; - ctxt->usernameWhitelist = usernameWhitelist; + ctxt->usernameACL = usernameACL; return ctxt; } @@ -146,7 +146,7 @@ int virNetSASLContextCheckIdentity(virNetSASLContextPtr ctxt, virObjectLock(ctxt); /* If the list is not set, allow any DN. */ - wildcards = ctxt->usernameWhitelist; + wildcards = ctxt->usernameACL; if (!wildcards) { ret = 1; /* No ACL, allow all */ goto cleanup; @@ -162,7 +162,7 @@ int virNetSASLContextCheckIdentity(virNetSASLContextPtr ctxt, } /* Denied */ - VIR_ERROR(_("SASL client identity '%s' not allowed in whitelist"), identity); + VIR_ERROR(_("SASL client identity '%s' not allowed by ACL"), identity); /* This is the most common error: make it informative. */ virReportError(VIR_ERR_SYSTEM_ERROR, "%s", diff --git a/src/rpc/virnetsaslcontext.h b/src/rpc/virnetsaslcontext.h index 4d1845e643..618230f42d 100644 --- a/src/rpc/virnetsaslcontext.h +++ b/src/rpc/virnetsaslcontext.h @@ -38,7 +38,7 @@ enum { }; virNetSASLContextPtr virNetSASLContextNewClient(void); -virNetSASLContextPtr virNetSASLContextNewServer(const char *const*usernameWhitelist); +virNetSASLContextPtr virNetSASLContextNewServer(const char *const *usernameACL); int virNetSASLContextCheckIdentity(virNetSASLContextPtr ctxt, const char *identity); diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c index a8104cf484..168f3010ae 100644 --- a/src/rpc/virnettlscontext.c +++ b/src/rpc/virnettlscontext.c @@ -60,7 +60,7 @@ struct _virNetTLSContext { bool isServer; bool requireValidCert; - const char *const*x509dnWhitelist; + const char *const *x509dnACL; char *priority; }; @@ -356,8 +356,8 @@ static int virNetTLSContextCheckCertKeyPurpose(gnutls_x509_crt_t cert, /* Check DN is on tls_allowed_dn_list. */ static int -virNetTLSContextCheckCertDNWhitelist(const char *dname, - const char *const*wildcards) +virNetTLSContextCheckCertDNACL(const char *dname, + const char *const *wildcards) { while (*wildcards) { if (g_pattern_match_simple(*wildcards, dname)) @@ -367,7 +367,7 @@ virNetTLSContextCheckCertDNWhitelist(const char *dname, } /* Log the client's DN for debugging */ - VIR_DEBUG("Failed whitelist check for client DN '%s'", dname); + VIR_DEBUG("Failed ACL check for client DN '%s'", dname); /* This is the most common error: make it informative. */ virReportError(VIR_ERR_SYSTEM_ERROR, "%s", @@ -385,10 +385,10 @@ virNetTLSContextCheckCertDN(gnutls_x509_crt_t cert, const char *certFile, const char *hostname, const char *dname, - const char *const* whitelist) + const char *const *acl) { - if (whitelist && dname && - virNetTLSContextCheckCertDNWhitelist(dname, whitelist) <= 0) + if (acl && dname && + virNetTLSContextCheckCertDNACL(dname, acl) <= 0) return -1; if (hostname && @@ -675,7 +675,7 @@ static virNetTLSContextPtr virNetTLSContextNew(const char *cacert, const char *cacrl, const char *cert, const char *key, - const char *const*x509dnWhitelist, + const char *const *x509dnACL, const char *priority, bool sanityCheckCert, bool requireValidCert, @@ -740,7 +740,7 @@ static virNetTLSContextPtr virNetTLSContextNew(const char *cacert, } ctxt->requireValidCert = requireValidCert; - ctxt->x509dnWhitelist = x509dnWhitelist; + ctxt->x509dnACL = x509dnACL; ctxt->isServer = isServer; PROBE(RPC_TLS_CONTEXT_NEW, @@ -855,7 +855,7 @@ static int virNetTLSContextLocateCredentials(const char *pkipath, static virNetTLSContextPtr virNetTLSContextNewPath(const char *pkipath, bool tryUserPkiPath, - const char *const*x509dnWhitelist, + const char *const *x509dnACL, const char *priority, bool sanityCheckCert, bool requireValidCert, @@ -869,7 +869,7 @@ static virNetTLSContextPtr virNetTLSContextNewPath(const char *pkipath, return NULL; ctxt = virNetTLSContextNew(cacert, cacrl, cert, key, - x509dnWhitelist, priority, sanityCheckCert, + x509dnACL, priority, sanityCheckCert, requireValidCert, isServer); VIR_FREE(cacert); @@ -882,12 +882,12 @@ static virNetTLSContextPtr virNetTLSContextNewPath(const char *pkipath, virNetTLSContextPtr virNetTLSContextNewServerPath(const char *pkipath, bool tryUserPkiPath, - const char *const*x509dnWhitelist, + const char *const *x509dnACL, const char *priority, bool sanityCheckCert, bool requireValidCert) { - return virNetTLSContextNewPath(pkipath, tryUserPkiPath, x509dnWhitelist, priority, + return virNetTLSContextNewPath(pkipath, tryUserPkiPath, x509dnACL, priority, sanityCheckCert, requireValidCert, true); } @@ -906,12 +906,12 @@ virNetTLSContextPtr virNetTLSContextNewServer(const char *cacert, const char *cacrl, const char *cert, const char *key, - const char *const*x509dnWhitelist, + const char *const *x509dnACL, const char *priority, bool sanityCheckCert, bool requireValidCert) { - return virNetTLSContextNew(cacert, cacrl, cert, key, x509dnWhitelist, priority, + return virNetTLSContextNew(cacert, cacrl, cert, key, x509dnACL, priority, sanityCheckCert, requireValidCert, true); } @@ -1063,7 +1063,7 @@ static int virNetTLSContextValidCertificate(virNetTLSContextPtr ctxt, VIR_DEBUG("Peer DN is %s", dname); if (virNetTLSContextCheckCertDN(cert, "[session]", sess->hostname, dname, - ctxt->x509dnWhitelist) < 0) { + ctxt->x509dnACL) < 0) { gnutls_x509_crt_deinit(cert); goto authdeny; } diff --git a/src/rpc/virnettlscontext.h b/src/rpc/virnettlscontext.h index fe885aed9a..8ac84027b2 100644 --- a/src/rpc/virnettlscontext.h +++ b/src/rpc/virnettlscontext.h @@ -34,7 +34,7 @@ void virNetTLSInit(void); virNetTLSContextPtr virNetTLSContextNewServerPath(const char *pkipath, bool tryUserPkiPath, - const char *const*x509dnWhitelist, + const char *const *x509dnACL, const char *priority, bool sanityCheckCert, bool requireValidCert); @@ -49,7 +49,7 @@ virNetTLSContextPtr virNetTLSContextNewServer(const char *cacert, const char *cacrl, const char *cert, const char *key, - const char *const*x509dnWhitelist, + const char *const *x509dnACL, const char *priority, bool sanityCheckCert, bool requireValidCert); diff --git a/tests/virconfdata/libvirtd.conf b/tests/virconfdata/libvirtd.conf index 791d6c972b..6d1fd33dcd 100644 --- a/tests/virconfdata/libvirtd.conf +++ b/tests/virconfdata/libvirtd.conf @@ -174,11 +174,11 @@ crl_file = "/etc/pki/CA/crl.pem" # will be rejected. # # Default is to always verify. Uncommenting this will disable -# verification - make sure an IP whitelist is set +# verification. tls_no_verify_certificate = 1 -# A whitelist of allowed x509 Distinguished Names +# An access control list of allowed x509 Distinguished Names # This list may contain wildcards such as # # "C=GB,ST=London,L=London,O=Red Hat,CN=*" @@ -194,7 +194,7 @@ tls_no_verify_certificate = 1 tls_allowed_dn_list = ["DN1", "DN2"] -# A whitelist of allowed SASL usernames. The format for usernames +# An access control list of allowed SASL usernames. The format for usernames # depends on the SASL authentication mechanism. Kerberos usernames # look like username@REALM # diff --git a/tests/virconfdata/libvirtd.out b/tests/virconfdata/libvirtd.out index cfdd23fd21..ce50480b8c 100644 --- a/tests/virconfdata/libvirtd.out +++ b/tests/virconfdata/libvirtd.out @@ -140,9 +140,9 @@ crl_file = "/etc/pki/CA/crl.pem" # will be rejected. # # Default is to always verify. Uncommenting this will disable -# verification - make sure an IP whitelist is set +# verification. tls_no_verify_certificate = 1 -# A whitelist of allowed x509 Distinguished Names +# An access control list of allowed x509 Distinguished Names # This list may contain wildcards such as # # "C=GB,ST=London,L=London,O=Red Hat,CN=*" @@ -156,7 +156,7 @@ tls_no_verify_certificate = 1 # # By default, no DN's are checked tls_allowed_dn_list = [ "DN1", "DN2" ] -# A whitelist of allowed SASL usernames. The format for usernames +# An access control list of allowed SASL usernames. The format for usernames # depends on the SASL authentication mechanism. Kerberos usernames # look like username@REALM # -- 2.24.1

On Fri, Jun 19, 2020 at 10:32:39 +0100, Daniel Berrange wrote:
The term "access control list" better describes the concept involved.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- docs/internals/rpc.html.in | 9 ++++---- src/remote/libvirtd.conf.in | 6 +++--- src/remote/remote_daemon_dispatch.c | 4 ++-- src/rpc/virnetsaslcontext.c | 10 ++++----- src/rpc/virnetsaslcontext.h | 2 +- src/rpc/virnettlscontext.c | 32 ++++++++++++++--------------- src/rpc/virnettlscontext.h | 4 ++-- tests/virconfdata/libvirtd.conf | 6 +++--- tests/virconfdata/libvirtd.out | 6 +++--- 9 files changed, 40 insertions(+), 39 deletions(-)
IMO this commit is changing too many places at the same time, but given that RPC internals aren't really touched that often downstream I'm okay with it in this instance. Reviewed-by: Peter Krempa <pkrempa@redhat.com>

The term "access control list" better describes the concept involved. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- docs/drvqemu.html.in | 12 ++++++------ docs/kbase/qemu-passthrough-security.rst | 3 ++- src/lxc/lxc_cgroup.c | 2 +- src/qemu/qemu.conf | 2 +- src/qemu/qemu_cgroup.c | 2 +- src/util/vircgroup.c | 2 +- 6 files changed, 12 insertions(+), 11 deletions(-) diff --git a/docs/drvqemu.html.in b/docs/drvqemu.html.in index b6d731bb59..31d3fee213 100644 --- a/docs/drvqemu.html.in +++ b/docs/drvqemu.html.in @@ -468,12 +468,12 @@ chmod o+x /path/to/directory for resource management. It is implemented via a number of "controllers", each controller covering a specific task/functional area. One of the available controllers is the "devices" controller, which is able to - setup whitelists of block/character devices that a cgroup should be - allowed to access. If the "devices" controller is mounted on a host, - then libvirt will automatically create a dedicated cgroup for each - QEMU virtual machine and setup the device whitelist so that the QEMU - process can only access shared devices, and explicitly disks images - backed by block devices. + setup access control lists of block/character devices that a cgroup + should be allowed to access. If the "devices" controller is mounted on a + host, then libvirt will automatically create a dedicated cgroup for each + QEMU virtual machine and setup the device access control list so that the + QEMU process can only access shared devices, and explicitly assigned disks + images backed by block devices. </p> <p> diff --git a/docs/kbase/qemu-passthrough-security.rst b/docs/kbase/qemu-passthrough-security.rst index 5f761cbfcb..4381d9f3a6 100644 --- a/docs/kbase/qemu-passthrough-security.rst +++ b/docs/kbase/qemu-passthrough-security.rst @@ -110,7 +110,8 @@ Granting access per VM policy on a per VM basis. * Cgroups - a custom cgroup is created per VM and this will either use the - ``devices`` controller or an ``BPF`` rule to whitelist a set of device nodes. + ``devices`` controller or an ``BPF`` rule to define an access control list + for the set of device nodes. There is no way to change this policy on a per VM basis. Disabling security protection per VM diff --git a/src/lxc/lxc_cgroup.c b/src/lxc/lxc_cgroup.c index e71f37d2b1..d13f2adde5 100644 --- a/src/lxc/lxc_cgroup.c +++ b/src/lxc/lxc_cgroup.c @@ -374,7 +374,7 @@ static int virLXCCgroupSetupDeviceACL(virDomainDefPtr def, return -1; } - VIR_DEBUG("Device whitelist complete"); + VIR_DEBUG("Device ACL setup complete"); return 0; } diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index 404961c53e..f89dbd2c3a 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -464,7 +464,7 @@ # What cgroup controllers to make use of with QEMU guests # # - 'cpu' - use for scheduler tunables -# - 'devices' - use for device whitelisting +# - 'devices' - use for device access control # - 'memory' - use for memory tunables # - 'blkio' - use for block devices I/O tunables # - 'cpuset' - use for CPUs and memory nodes diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index d92202f847..57c5b6e69b 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -745,7 +745,7 @@ qemuSetupDevicesCgroup(virDomainObjPtr vm) if (rv < 0) { if (virLastErrorIsSystemErrno(EPERM)) { virResetLastError(); - VIR_WARN("Group devices ACL is not accessible, disabling whitelisting"); + VIR_WARN("Group devices ACL is not accessible, disabling filtering"); return 0; } diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index bb535df4f2..e20cc71c78 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -773,7 +773,7 @@ virCgroupSetPartitionSuffix(const char *path, char **res) return ret; for (i = 0; tokens[i] != NULL; i++) { - /* Whitelist the 3 top level fixed dirs + /* Special case the 3 top level fixed dirs * NB i == 0 is "", since we have leading '/' */ if (i == 1 && -- 2.24.1

On Fri, Jun 19, 2020 at 10:32:40 +0100, Daniel Berrange wrote:
The term "access control list" better describes the concept involved.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- docs/drvqemu.html.in | 12 ++++++------ docs/kbase/qemu-passthrough-security.rst | 3 ++- src/lxc/lxc_cgroup.c | 2 +- src/qemu/qemu.conf | 2 +- src/qemu/qemu_cgroup.c | 2 +- src/util/vircgroup.c | 2 +- 6 files changed, 12 insertions(+), 11 deletions(-)
Preferably at least docs changes are kept separate. Reviewed-by: Peter Krempa <pkrempa@redhat.com>

When listing CPU models, we need to filter the data based on sets of permitted and forbidden CPU models. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/qemu/qemu_capabilities.c | 18 +++++++++--------- src/qemu/qemu_capabilities.h | 4 ++-- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 484fff99e5..68fcbd3c4f 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -2256,8 +2256,8 @@ virQEMUCapsAddCPUDefinitions(virQEMUCapsPtr qemuCaps, static virDomainCapsCPUModelsPtr virQEMUCapsCPUDefsToModels(qemuMonitorCPUDefsPtr defs, - const char **modelWhitelist, - const char **modelBlacklist) + const char **modelAllowed, + const char **modelForbidden) { g_autoptr(virDomainCapsCPUModels) cpuModels = NULL; size_t i; @@ -2268,10 +2268,10 @@ virQEMUCapsCPUDefsToModels(qemuMonitorCPUDefsPtr defs, for (i = 0; i < defs->ncpus; i++) { qemuMonitorCPUDefInfoPtr cpu = defs->cpus + i; - if (modelWhitelist && !virStringListHasString(modelWhitelist, cpu->name)) + if (modelAllowed && !virStringListHasString(modelAllowed, cpu->name)) continue; - if (modelBlacklist && virStringListHasString(modelBlacklist, cpu->name)) + if (modelForbidden && virStringListHasString(modelForbidden, cpu->name)) continue; if (virDomainCapsCPUModelsAdd(cpuModels, cpu->name, cpu->usable, @@ -2286,15 +2286,15 @@ virQEMUCapsCPUDefsToModels(qemuMonitorCPUDefsPtr defs, virDomainCapsCPUModelsPtr virQEMUCapsGetCPUModels(virQEMUCapsPtr qemuCaps, virDomainVirtType type, - const char **modelWhitelist, - const char **modelBlacklist) + const char **modelAllowed, + const char **modelForbidden) { qemuMonitorCPUDefsPtr defs; if (!(defs = virQEMUCapsGetAccel(qemuCaps, type)->cpuModels)) return NULL; - return virQEMUCapsCPUDefsToModels(defs, modelWhitelist, modelBlacklist); + return virQEMUCapsCPUDefsToModels(defs, modelAllowed, modelForbidden); } @@ -5976,14 +5976,14 @@ virQEMUCapsFillDomainCPUCaps(virQEMUCapsPtr qemuCaps, if (virQEMUCapsIsCPUModeSupported(qemuCaps, hostarch, domCaps->virttype, VIR_CPU_MODE_CUSTOM, domCaps->machine)) { - const char *blacklist[] = { "host", NULL }; + const char *forbidden[] = { "host", NULL }; VIR_AUTOSTRINGLIST models = NULL; if (virCPUGetModels(domCaps->arch, &models) >= 0) { domCaps->cpu.custom = virQEMUCapsGetCPUModels(qemuCaps, domCaps->virttype, (const char **)models, - blacklist); + forbidden); } else { domCaps->cpu.custom = NULL; } diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 92d42ed80b..ad93816d41 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -629,8 +629,8 @@ int virQEMUCapsAddCPUDefinitions(virQEMUCapsPtr qemuCaps, virDomainCapsCPUUsable usable); virDomainCapsCPUModelsPtr virQEMUCapsGetCPUModels(virQEMUCapsPtr qemuCaps, virDomainVirtType type, - const char **modelWhitelist, - const char **modelBlacklist); + const char **modelAllowed, + const char **modelForbidden); int virQEMUCapsFetchCPUModels(qemuMonitorPtr mon, virArch arch, virDomainCapsCPUModelsPtr *cpuModels); -- 2.24.1

On Fri, Jun 19, 2020 at 10:32:41 +0100, Daniel Berrange wrote:
When listing CPU models, we need to filter the data based on sets of permitted and forbidden CPU models.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/qemu/qemu_capabilities.c | 18 +++++++++--------- src/qemu/qemu_capabilities.h | 4 ++-- 2 files changed, 11 insertions(+), 11 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>

The concept we're really testing for is whether QEMU supports the seccomp syscall filter groups. We need to keep one place using the old term to deal with upgrades from existing hosts with running VMs. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/qemu/qemu.conf | 2 +- src/qemu/qemu_capabilities.c | 4 ++-- src/qemu/qemu_capabilities.h | 2 +- src/qemu/qemu_command.c | 4 ++-- src/qemu/qemu_domain.c | 10 +++++++--- tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml | 2 +- tests/qemucapabilitiesdata/caps_2.11.0.x86_64.xml | 2 +- tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml | 2 +- tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml | 2 +- tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml | 2 +- tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml | 2 +- tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml | 2 +- tests/qemucapabilitiesdata/caps_3.0.0.riscv32.xml | 2 +- tests/qemucapabilitiesdata/caps_3.0.0.riscv64.xml | 2 +- tests/qemucapabilitiesdata/caps_3.0.0.s390x.xml | 2 +- tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml | 2 +- tests/qemucapabilitiesdata/caps_3.1.0.ppc64.xml | 2 +- tests/qemucapabilitiesdata/caps_3.1.0.x86_64.xml | 2 +- tests/qemucapabilitiesdata/caps_4.0.0.aarch64.xml | 2 +- tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml | 2 +- tests/qemucapabilitiesdata/caps_4.0.0.riscv32.xml | 2 +- tests/qemucapabilitiesdata/caps_4.0.0.riscv64.xml | 2 +- tests/qemucapabilitiesdata/caps_4.0.0.s390x.xml | 2 +- tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml | 2 +- tests/qemucapabilitiesdata/caps_4.1.0.x86_64.xml | 2 +- tests/qemucapabilitiesdata/caps_4.2.0.aarch64.xml | 2 +- tests/qemucapabilitiesdata/caps_4.2.0.ppc64.xml | 2 +- tests/qemucapabilitiesdata/caps_4.2.0.s390x.xml | 2 +- tests/qemucapabilitiesdata/caps_4.2.0.x86_64.xml | 2 +- tests/qemucapabilitiesdata/caps_5.0.0.aarch64.xml | 2 +- tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml | 2 +- tests/qemucapabilitiesdata/caps_5.0.0.riscv64.xml | 2 +- tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml | 2 +- tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml | 2 +- tests/qemustatusxml2xmldata/backup-pull-in.xml | 2 +- tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml | 2 +- tests/qemuxml2argvtest.c | 2 +- 37 files changed, 45 insertions(+), 41 deletions(-) diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index f89dbd2c3a..99b9ce53e5 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -704,7 +704,7 @@ # If it is unset (or -1), then seccomp will be enabled # only if QEMU >= 2.11.0 is detected, otherwise it is # left disabled. This ensures the default config gets -# protection for new QEMU using the blacklist approach. +# protection for new QEMU with filter groups. # #seccomp_sandbox = 1 diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 68fcbd3c4f..310be800e2 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -468,7 +468,7 @@ VIR_ENUM_IMPL(virQEMUCaps, /* 285 */ "qcow2-luks", "pcie-pci-bridge", - "seccomp-blacklist", + "seccomp-filter-groups", "query-cpus-fast", "disk-write-cache", @@ -3292,7 +3292,7 @@ static struct virQEMUCapsCommandLineProps virQEMUCapsCommandLine[] = { { "vnc", "vnc", QEMU_CAPS_VNC_MULTI_SERVERS }, { "chardev", "reconnect", QEMU_CAPS_CHARDEV_RECONNECT }, { "sandbox", "enable", QEMU_CAPS_SECCOMP_SANDBOX }, - { "sandbox", "elevateprivileges", QEMU_CAPS_SECCOMP_BLACKLIST }, + { "sandbox", "elevateprivileges", QEMU_CAPS_SECCOMP_FILTER_GROUPS }, { "chardev", "fd", QEMU_CAPS_CHARDEV_FD_PASS }, { "overcommit", NULL, QEMU_CAPS_OVERCOMMIT }, { "smp-opts", "dies", QEMU_CAPS_SMP_DIES }, diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index ad93816d41..0ee3e357cb 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -448,7 +448,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */ /* 285 */ QEMU_CAPS_QCOW2_LUKS, /* qcow2 format support LUKS encryption */ QEMU_CAPS_DEVICE_PCIE_PCI_BRIDGE, /* -device pcie-pci-bridge */ - QEMU_CAPS_SECCOMP_BLACKLIST, /* -sandbox.elevateprivileges */ + QEMU_CAPS_SECCOMP_FILTER_GROUPS, /* -sandbox.elevateprivileges */ QEMU_CAPS_QUERY_CPUS_FAST, /* query-cpus-fast command */ QEMU_CAPS_DISK_WRITE_CACHE, /* qemu block frontends support write-cache param */ diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index f27246b4c6..37113a433a 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -9517,8 +9517,8 @@ qemuBuildSeccompSandboxCommandLine(virCommandPtr cmd, return 0; } - /* Use blacklist by default if supported */ - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SECCOMP_BLACKLIST)) { + /* Block undesirable syscall groups by default if supported */ + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SECCOMP_FILTER_GROUPS)) { virCommandAddArgList(cmd, "-sandbox", "on,obsolete=deny,elevateprivileges=deny," "spawn=deny,resourcecontrol=deny", diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 72874ee4fd..56ec5c0352 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3851,9 +3851,13 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, if (str) { int flag = virQEMUCapsTypeFromString(str); if (flag < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unknown qemu capabilities flag %s"), str); - goto error; + if (g_str_equal(str, "seccomp-blacklist")) { + flag = QEMU_CAPS_SECCOMP_FILTER_GROUPS; + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unknown qemu capabilities flag %s"), str); + goto error; + } } virQEMUCapsSet(qemuCaps, flag); } diff --git a/tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml b/tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml index 0391f4b81e..9822f50827 100644 --- a/tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml @@ -99,7 +99,7 @@ <flag name='virtio-mouse-ccw'/> <flag name='virtio-tablet-ccw'/> <flag name='qcow2-luks'/> - <flag name='seccomp-blacklist'/> + <flag name='seccomp-filter-groups'/> <flag name='disk-write-cache'/> <flag name='nbd-tls'/> <flag name='pr-manager-helper'/> diff --git a/tests/qemucapabilitiesdata/caps_2.11.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.11.0.x86_64.xml index 9eaafb4ba6..3e5e3b4ad3 100644 --- a/tests/qemucapabilitiesdata/caps_2.11.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.11.0.x86_64.xml @@ -173,7 +173,7 @@ <flag name='dump-completed'/> <flag name='qcow2-luks'/> <flag name='pcie-pci-bridge'/> - <flag name='seccomp-blacklist'/> + <flag name='seccomp-filter-groups'/> <flag name='disk-write-cache'/> <flag name='nbd-tls'/> <flag name='pr-manager-helper'/> diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml b/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml index a5d6dc3bef..3c5f8235fe 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml @@ -134,7 +134,7 @@ <flag name='dump-completed'/> <flag name='qcow2-luks'/> <flag name='pcie-pci-bridge'/> - <flag name='seccomp-blacklist'/> + <flag name='seccomp-filter-groups'/> <flag name='query-cpus-fast'/> <flag name='disk-write-cache'/> <flag name='nbd-tls'/> diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml b/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml index d1ed9f6e28..e5a02c382e 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml @@ -131,7 +131,7 @@ <flag name='machine.pseries.max-cpu-compat'/> <flag name='dump-completed'/> <flag name='qcow2-luks'/> - <flag name='seccomp-blacklist'/> + <flag name='seccomp-filter-groups'/> <flag name='query-cpus-fast'/> <flag name='disk-write-cache'/> <flag name='nbd-tls'/> diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml b/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml index cef6ebb9ad..238b05240c 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml @@ -99,7 +99,7 @@ <flag name='virtio-mouse-ccw'/> <flag name='virtio-tablet-ccw'/> <flag name='qcow2-luks'/> - <flag name='seccomp-blacklist'/> + <flag name='seccomp-filter-groups'/> <flag name='query-cpus-fast'/> <flag name='disk-write-cache'/> <flag name='nbd-tls'/> diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml index 6d48699e3e..6011f2f4a2 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml @@ -170,7 +170,7 @@ <flag name='dump-completed'/> <flag name='qcow2-luks'/> <flag name='pcie-pci-bridge'/> - <flag name='seccomp-blacklist'/> + <flag name='seccomp-filter-groups'/> <flag name='query-cpus-fast'/> <flag name='disk-write-cache'/> <flag name='nbd-tls'/> diff --git a/tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml b/tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml index e4a560bac5..a1643260ab 100644 --- a/tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml +++ b/tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml @@ -130,7 +130,7 @@ <flag name='machine.pseries.max-cpu-compat'/> <flag name='dump-completed'/> <flag name='qcow2-luks'/> - <flag name='seccomp-blacklist'/> + <flag name='seccomp-filter-groups'/> <flag name='query-cpus-fast'/> <flag name='disk-write-cache'/> <flag name='nbd-tls'/> diff --git a/tests/qemucapabilitiesdata/caps_3.0.0.riscv32.xml b/tests/qemucapabilitiesdata/caps_3.0.0.riscv32.xml index 71f9b0c37f..6d1e3d8cd5 100644 --- a/tests/qemucapabilitiesdata/caps_3.0.0.riscv32.xml +++ b/tests/qemucapabilitiesdata/caps_3.0.0.riscv32.xml @@ -75,7 +75,7 @@ <flag name='iscsi.password-secret'/> <flag name='dump-completed'/> <flag name='qcow2-luks'/> - <flag name='seccomp-blacklist'/> + <flag name='seccomp-filter-groups'/> <flag name='query-cpus-fast'/> <flag name='disk-write-cache'/> <flag name='nbd-tls'/> diff --git a/tests/qemucapabilitiesdata/caps_3.0.0.riscv64.xml b/tests/qemucapabilitiesdata/caps_3.0.0.riscv64.xml index 279078d541..a6994acac3 100644 --- a/tests/qemucapabilitiesdata/caps_3.0.0.riscv64.xml +++ b/tests/qemucapabilitiesdata/caps_3.0.0.riscv64.xml @@ -75,7 +75,7 @@ <flag name='iscsi.password-secret'/> <flag name='dump-completed'/> <flag name='qcow2-luks'/> - <flag name='seccomp-blacklist'/> + <flag name='seccomp-filter-groups'/> <flag name='query-cpus-fast'/> <flag name='disk-write-cache'/> <flag name='nbd-tls'/> diff --git a/tests/qemucapabilitiesdata/caps_3.0.0.s390x.xml b/tests/qemucapabilitiesdata/caps_3.0.0.s390x.xml index f1ed34c612..4d80f9c6ba 100644 --- a/tests/qemucapabilitiesdata/caps_3.0.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_3.0.0.s390x.xml @@ -101,7 +101,7 @@ <flag name='virtio-mouse-ccw'/> <flag name='virtio-tablet-ccw'/> <flag name='qcow2-luks'/> - <flag name='seccomp-blacklist'/> + <flag name='seccomp-filter-groups'/> <flag name='query-cpus-fast'/> <flag name='disk-write-cache'/> <flag name='nbd-tls'/> diff --git a/tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml index ae1836b28f..e31cb7c345 100644 --- a/tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml @@ -172,7 +172,7 @@ <flag name='dump-completed'/> <flag name='qcow2-luks'/> <flag name='pcie-pci-bridge'/> - <flag name='seccomp-blacklist'/> + <flag name='seccomp-filter-groups'/> <flag name='query-cpus-fast'/> <flag name='disk-write-cache'/> <flag name='nbd-tls'/> diff --git a/tests/qemucapabilitiesdata/caps_3.1.0.ppc64.xml b/tests/qemucapabilitiesdata/caps_3.1.0.ppc64.xml index 0dc0393c22..d01de900c9 100644 --- a/tests/qemucapabilitiesdata/caps_3.1.0.ppc64.xml +++ b/tests/qemucapabilitiesdata/caps_3.1.0.ppc64.xml @@ -131,7 +131,7 @@ <flag name='machine.pseries.max-cpu-compat'/> <flag name='dump-completed'/> <flag name='qcow2-luks'/> - <flag name='seccomp-blacklist'/> + <flag name='seccomp-filter-groups'/> <flag name='query-cpus-fast'/> <flag name='disk-write-cache'/> <flag name='nbd-tls'/> diff --git a/tests/qemucapabilitiesdata/caps_3.1.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_3.1.0.x86_64.xml index d4ff21fdac..177dedbfb5 100644 --- a/tests/qemucapabilitiesdata/caps_3.1.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_3.1.0.x86_64.xml @@ -172,7 +172,7 @@ <flag name='dump-completed'/> <flag name='qcow2-luks'/> <flag name='pcie-pci-bridge'/> - <flag name='seccomp-blacklist'/> + <flag name='seccomp-filter-groups'/> <flag name='query-cpus-fast'/> <flag name='disk-write-cache'/> <flag name='nbd-tls'/> diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.aarch64.xml b/tests/qemucapabilitiesdata/caps_4.0.0.aarch64.xml index 404a39af03..7afec03c2f 100644 --- a/tests/qemucapabilitiesdata/caps_4.0.0.aarch64.xml +++ b/tests/qemucapabilitiesdata/caps_4.0.0.aarch64.xml @@ -135,7 +135,7 @@ <flag name='dump-completed'/> <flag name='qcow2-luks'/> <flag name='pcie-pci-bridge'/> - <flag name='seccomp-blacklist'/> + <flag name='seccomp-filter-groups'/> <flag name='query-cpus-fast'/> <flag name='disk-write-cache'/> <flag name='nbd-tls'/> diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml b/tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml index cb0232173c..81ed3b58de 100644 --- a/tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml +++ b/tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml @@ -138,7 +138,7 @@ <flag name='dump-completed'/> <flag name='qcow2-luks'/> <flag name='pcie-pci-bridge'/> - <flag name='seccomp-blacklist'/> + <flag name='seccomp-filter-groups'/> <flag name='query-cpus-fast'/> <flag name='disk-write-cache'/> <flag name='nbd-tls'/> diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.riscv32.xml b/tests/qemucapabilitiesdata/caps_4.0.0.riscv32.xml index 11475306f9..bfb38b6eae 100644 --- a/tests/qemucapabilitiesdata/caps_4.0.0.riscv32.xml +++ b/tests/qemucapabilitiesdata/caps_4.0.0.riscv32.xml @@ -139,7 +139,7 @@ <flag name='dump-completed'/> <flag name='qcow2-luks'/> <flag name='pcie-pci-bridge'/> - <flag name='seccomp-blacklist'/> + <flag name='seccomp-filter-groups'/> <flag name='query-cpus-fast'/> <flag name='disk-write-cache'/> <flag name='nbd-tls'/> diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.riscv64.xml b/tests/qemucapabilitiesdata/caps_4.0.0.riscv64.xml index 608590a35b..801a7c368e 100644 --- a/tests/qemucapabilitiesdata/caps_4.0.0.riscv64.xml +++ b/tests/qemucapabilitiesdata/caps_4.0.0.riscv64.xml @@ -139,7 +139,7 @@ <flag name='dump-completed'/> <flag name='qcow2-luks'/> <flag name='pcie-pci-bridge'/> - <flag name='seccomp-blacklist'/> + <flag name='seccomp-filter-groups'/> <flag name='query-cpus-fast'/> <flag name='disk-write-cache'/> <flag name='nbd-tls'/> diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.s390x.xml b/tests/qemucapabilitiesdata/caps_4.0.0.s390x.xml index f4d20169e0..0be526ce7f 100644 --- a/tests/qemucapabilitiesdata/caps_4.0.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_4.0.0.s390x.xml @@ -101,7 +101,7 @@ <flag name='virtio-mouse-ccw'/> <flag name='virtio-tablet-ccw'/> <flag name='qcow2-luks'/> - <flag name='seccomp-blacklist'/> + <flag name='seccomp-filter-groups'/> <flag name='query-cpus-fast'/> <flag name='disk-write-cache'/> <flag name='nbd-tls'/> diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml index 0e66a4c847..930f508048 100644 --- a/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml @@ -171,7 +171,7 @@ <flag name='dump-completed'/> <flag name='qcow2-luks'/> <flag name='pcie-pci-bridge'/> - <flag name='seccomp-blacklist'/> + <flag name='seccomp-filter-groups'/> <flag name='query-cpus-fast'/> <flag name='disk-write-cache'/> <flag name='nbd-tls'/> diff --git a/tests/qemucapabilitiesdata/caps_4.1.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_4.1.0.x86_64.xml index f2d3902e6c..e1481979e8 100644 --- a/tests/qemucapabilitiesdata/caps_4.1.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_4.1.0.x86_64.xml @@ -171,7 +171,7 @@ <flag name='dump-completed'/> <flag name='qcow2-luks'/> <flag name='pcie-pci-bridge'/> - <flag name='seccomp-blacklist'/> + <flag name='seccomp-filter-groups'/> <flag name='query-cpus-fast'/> <flag name='disk-write-cache'/> <flag name='nbd-tls'/> diff --git a/tests/qemucapabilitiesdata/caps_4.2.0.aarch64.xml b/tests/qemucapabilitiesdata/caps_4.2.0.aarch64.xml index 98cee36669..bc643545ac 100644 --- a/tests/qemucapabilitiesdata/caps_4.2.0.aarch64.xml +++ b/tests/qemucapabilitiesdata/caps_4.2.0.aarch64.xml @@ -137,7 +137,7 @@ <flag name='dump-completed'/> <flag name='qcow2-luks'/> <flag name='pcie-pci-bridge'/> - <flag name='seccomp-blacklist'/> + <flag name='seccomp-filter-groups'/> <flag name='query-cpus-fast'/> <flag name='disk-write-cache'/> <flag name='nbd-tls'/> diff --git a/tests/qemucapabilitiesdata/caps_4.2.0.ppc64.xml b/tests/qemucapabilitiesdata/caps_4.2.0.ppc64.xml index 70c826e0cf..ed3c865747 100644 --- a/tests/qemucapabilitiesdata/caps_4.2.0.ppc64.xml +++ b/tests/qemucapabilitiesdata/caps_4.2.0.ppc64.xml @@ -138,7 +138,7 @@ <flag name='dump-completed'/> <flag name='qcow2-luks'/> <flag name='pcie-pci-bridge'/> - <flag name='seccomp-blacklist'/> + <flag name='seccomp-filter-groups'/> <flag name='query-cpus-fast'/> <flag name='disk-write-cache'/> <flag name='nbd-tls'/> diff --git a/tests/qemucapabilitiesdata/caps_4.2.0.s390x.xml b/tests/qemucapabilitiesdata/caps_4.2.0.s390x.xml index 0b174ffeec..335a06d897 100644 --- a/tests/qemucapabilitiesdata/caps_4.2.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_4.2.0.s390x.xml @@ -101,7 +101,7 @@ <flag name='virtio-mouse-ccw'/> <flag name='virtio-tablet-ccw'/> <flag name='qcow2-luks'/> - <flag name='seccomp-blacklist'/> + <flag name='seccomp-filter-groups'/> <flag name='query-cpus-fast'/> <flag name='disk-write-cache'/> <flag name='nbd-tls'/> diff --git a/tests/qemucapabilitiesdata/caps_4.2.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_4.2.0.x86_64.xml index eaf71eb469..009536f0b4 100644 --- a/tests/qemucapabilitiesdata/caps_4.2.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_4.2.0.x86_64.xml @@ -172,7 +172,7 @@ <flag name='dump-completed'/> <flag name='qcow2-luks'/> <flag name='pcie-pci-bridge'/> - <flag name='seccomp-blacklist'/> + <flag name='seccomp-filter-groups'/> <flag name='query-cpus-fast'/> <flag name='disk-write-cache'/> <flag name='nbd-tls'/> diff --git a/tests/qemucapabilitiesdata/caps_5.0.0.aarch64.xml b/tests/qemucapabilitiesdata/caps_5.0.0.aarch64.xml index f2d691734f..b2f6e0ed30 100644 --- a/tests/qemucapabilitiesdata/caps_5.0.0.aarch64.xml +++ b/tests/qemucapabilitiesdata/caps_5.0.0.aarch64.xml @@ -139,7 +139,7 @@ <flag name='dump-completed'/> <flag name='qcow2-luks'/> <flag name='pcie-pci-bridge'/> - <flag name='seccomp-blacklist'/> + <flag name='seccomp-filter-groups'/> <flag name='query-cpus-fast'/> <flag name='disk-write-cache'/> <flag name='nbd-tls'/> diff --git a/tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml b/tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml index b3f673b0f6..c9cb2c0639 100644 --- a/tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml +++ b/tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml @@ -140,7 +140,7 @@ <flag name='dump-completed'/> <flag name='qcow2-luks'/> <flag name='pcie-pci-bridge'/> - <flag name='seccomp-blacklist'/> + <flag name='seccomp-filter-groups'/> <flag name='query-cpus-fast'/> <flag name='disk-write-cache'/> <flag name='nbd-tls'/> diff --git a/tests/qemucapabilitiesdata/caps_5.0.0.riscv64.xml b/tests/qemucapabilitiesdata/caps_5.0.0.riscv64.xml index 3119f6deb7..75c2fbfd54 100644 --- a/tests/qemucapabilitiesdata/caps_5.0.0.riscv64.xml +++ b/tests/qemucapabilitiesdata/caps_5.0.0.riscv64.xml @@ -139,7 +139,7 @@ <flag name='dump-completed'/> <flag name='qcow2-luks'/> <flag name='pcie-pci-bridge'/> - <flag name='seccomp-blacklist'/> + <flag name='seccomp-filter-groups'/> <flag name='query-cpus-fast'/> <flag name='disk-write-cache'/> <flag name='nbd-tls'/> diff --git a/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml index 6d1c779272..a01395cf53 100644 --- a/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml @@ -172,7 +172,7 @@ <flag name='dump-completed'/> <flag name='qcow2-luks'/> <flag name='pcie-pci-bridge'/> - <flag name='seccomp-blacklist'/> + <flag name='seccomp-filter-groups'/> <flag name='query-cpus-fast'/> <flag name='disk-write-cache'/> <flag name='nbd-tls'/> diff --git a/tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml index 26a7985add..7c36716d88 100644 --- a/tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml @@ -172,7 +172,7 @@ <flag name='dump-completed'/> <flag name='qcow2-luks'/> <flag name='pcie-pci-bridge'/> - <flag name='seccomp-blacklist'/> + <flag name='seccomp-filter-groups'/> <flag name='query-cpus-fast'/> <flag name='disk-write-cache'/> <flag name='nbd-tls'/> diff --git a/tests/qemustatusxml2xmldata/backup-pull-in.xml b/tests/qemustatusxml2xmldata/backup-pull-in.xml index 1db978a3ac..76d723cf76 100644 --- a/tests/qemustatusxml2xmldata/backup-pull-in.xml +++ b/tests/qemustatusxml2xmldata/backup-pull-in.xml @@ -189,7 +189,7 @@ <flag name='dump-completed'/> <flag name='qcow2-luks'/> <flag name='pcie-pci-bridge'/> - <flag name='seccomp-blacklist'/> + <flag name='seccomp-filter-groups'/> <flag name='query-cpus-fast'/> <flag name='disk-write-cache'/> <flag name='nbd-tls'/> diff --git a/tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml b/tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml index cc17a17ff4..a4d9d57666 100644 --- a/tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml +++ b/tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml @@ -189,7 +189,7 @@ <flag name='dump-completed'/> <flag name='qcow2-luks'/> <flag name='pcie-pci-bridge'/> - <flag name='seccomp-blacklist'/> + <flag name='seccomp-filter-groups'/> <flag name='query-cpus-fast'/> <flag name='disk-write-cache'/> <flag name='nbd-tls'/> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index d5b2a21b5a..0989abd042 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -921,7 +921,7 @@ mymain(void) DO_TEST("minimal", NONE); DO_TEST("minimal-sandbox", - QEMU_CAPS_SECCOMP_BLACKLIST); + QEMU_CAPS_SECCOMP_FILTER_GROUPS); DO_TEST_PARSE_ERROR("minimal-no-memory", NONE); DO_TEST("minimal-msg-timestamp", QEMU_CAPS_MSG_TIMESTAMP); -- 2.24.1

On Fri, Jun 19, 2020 at 10:32:42 +0100, Daniel Berrange wrote:
The concept we're really testing for is whether QEMU supports the seccomp syscall filter groups. We need to keep one place using the old term to deal with upgrades from existing hosts with running VMs.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 72874ee4fd..56ec5c0352 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3851,9 +3851,13 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, if (str) { int flag = virQEMUCapsTypeFromString(str); if (flag < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unknown qemu capabilities flag %s"), str); - goto error; + if (g_str_equal(str, "seccomp-blacklist")) { + flag = QEMU_CAPS_SECCOMP_FILTER_GROUPS; + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unknown qemu capabilities flag %s"), str);
I think this should become an array so that we can extend it arbitrarily later. This concept may come in useful. Additionally making it with a proper explanation will prevent us from having a magic constant in a random place in the code without proper explanation.

On a Friday in 2020, Daniel P. Berrangé wrote:
The concept we're really testing for is whether QEMU supports the seccomp syscall filter groups. We need to keep one place using the old term to deal with upgrades from existing hosts with running VMs.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/qemu/qemu.conf | 2 +- src/qemu/qemu_capabilities.c | 4 ++-- src/qemu/qemu_capabilities.h | 2 +- src/qemu/qemu_command.c | 4 ++-- src/qemu/qemu_domain.c | 10 +++++++--- tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml | 2 +- tests/qemucapabilitiesdata/caps_2.11.0.x86_64.xml | 2 +- tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml | 2 +- tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml | 2 +- tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml | 2 +- tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml | 2 +- tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml | 2 +- tests/qemucapabilitiesdata/caps_3.0.0.riscv32.xml | 2 +- tests/qemucapabilitiesdata/caps_3.0.0.riscv64.xml | 2 +- tests/qemucapabilitiesdata/caps_3.0.0.s390x.xml | 2 +- tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml | 2 +- tests/qemucapabilitiesdata/caps_3.1.0.ppc64.xml | 2 +- tests/qemucapabilitiesdata/caps_3.1.0.x86_64.xml | 2 +- tests/qemucapabilitiesdata/caps_4.0.0.aarch64.xml | 2 +- tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml | 2 +- tests/qemucapabilitiesdata/caps_4.0.0.riscv32.xml | 2 +- tests/qemucapabilitiesdata/caps_4.0.0.riscv64.xml | 2 +- tests/qemucapabilitiesdata/caps_4.0.0.s390x.xml | 2 +- tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml | 2 +- tests/qemucapabilitiesdata/caps_4.1.0.x86_64.xml | 2 +- tests/qemucapabilitiesdata/caps_4.2.0.aarch64.xml | 2 +- tests/qemucapabilitiesdata/caps_4.2.0.ppc64.xml | 2 +- tests/qemucapabilitiesdata/caps_4.2.0.s390x.xml | 2 +- tests/qemucapabilitiesdata/caps_4.2.0.x86_64.xml | 2 +- tests/qemucapabilitiesdata/caps_5.0.0.aarch64.xml | 2 +- tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml | 2 +- tests/qemucapabilitiesdata/caps_5.0.0.riscv64.xml | 2 +- tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml | 2 +- tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml | 2 +- tests/qemustatusxml2xmldata/backup-pull-in.xml | 2 +- tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml | 2 +- tests/qemuxml2argvtest.c | 2 +- 37 files changed, 45 insertions(+), 41 deletions(-)
diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index f89dbd2c3a..99b9ce53e5 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -704,7 +704,7 @@ # If it is unset (or -1), then seccomp will be enabled # only if QEMU >= 2.11.0 is detected, otherwise it is # left disabled. This ensures the default config gets -# protection for new QEMU using the blacklist approach. +# protection for new QEMU with filter groups. # #seccomp_sandbox = 1
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 68fcbd3c4f..310be800e2 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -468,7 +468,7 @@ VIR_ENUM_IMPL(virQEMUCaps, /* 285 */ "qcow2-luks", "pcie-pci-bridge", - "seccomp-blacklist", + "seccomp-filter-groups", "query-cpus-fast", "disk-write-cache",
@@ -3292,7 +3292,7 @@ static struct virQEMUCapsCommandLineProps virQEMUCapsCommandLine[] = { { "vnc", "vnc", QEMU_CAPS_VNC_MULTI_SERVERS }, { "chardev", "reconnect", QEMU_CAPS_CHARDEV_RECONNECT }, { "sandbox", "enable", QEMU_CAPS_SECCOMP_SANDBOX }, - { "sandbox", "elevateprivileges", QEMU_CAPS_SECCOMP_BLACKLIST }, + { "sandbox", "elevateprivileges", QEMU_CAPS_SECCOMP_FILTER_GROUPS }, { "chardev", "fd", QEMU_CAPS_CHARDEV_FD_PASS }, { "overcommit", NULL, QEMU_CAPS_OVERCOMMIT }, { "smp-opts", "dies", QEMU_CAPS_SMP_DIES }, diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index ad93816d41..0ee3e357cb 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -448,7 +448,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */ /* 285 */ QEMU_CAPS_QCOW2_LUKS, /* qcow2 format support LUKS encryption */ QEMU_CAPS_DEVICE_PCIE_PCI_BRIDGE, /* -device pcie-pci-bridge */ - QEMU_CAPS_SECCOMP_BLACKLIST, /* -sandbox.elevateprivileges */ + QEMU_CAPS_SECCOMP_FILTER_GROUPS, /* -sandbox.elevateprivileges */ QEMU_CAPS_QUERY_CPUS_FAST, /* query-cpus-fast command */ QEMU_CAPS_DISK_WRITE_CACHE, /* qemu block frontends support write-cache param */
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index f27246b4c6..37113a433a 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -9517,8 +9517,8 @@ qemuBuildSeccompSandboxCommandLine(virCommandPtr cmd, return 0; }
- /* Use blacklist by default if supported */ - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SECCOMP_BLACKLIST)) { + /* Block undesirable syscall groups by default if supported */ + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SECCOMP_FILTER_GROUPS)) {
While 'filter groups' describes the underlying QEMU functionality better, we only use it to deny syscalls. So using 'blocklist' as proposed in the RFC you linked would better show the contrast between this and the old approach.
virCommandAddArgList(cmd, "-sandbox", "on,obsolete=deny,elevateprivileges=deny," "spawn=deny,resourcecontrol=deny", diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 72874ee4fd..56ec5c0352 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3851,9 +3851,13 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, if (str) { int flag = virQEMUCapsTypeFromString(str); if (flag < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unknown qemu capabilities flag %s"), str); - goto error; + if (g_str_equal(str, "seccomp-blacklist")) { + flag = QEMU_CAPS_SECCOMP_FILTER_GROUPS;
I'd just leave the XML as-is, to avoid introducing this special-casing. Jano
+ } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unknown qemu capabilities flag %s"), str); + goto error; + } } virQEMUCapsSet(qemuCaps, flag); }

On Fri, Jun 19, 2020 at 01:56:55PM +0200, Ján Tomko wrote:
On a Friday in 2020, Daniel P. Berrangé wrote:
The concept we're really testing for is whether QEMU supports the seccomp syscall filter groups. We need to keep one place using the old term to deal with upgrades from existing hosts with running VMs.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/qemu/qemu.conf | 2 +- src/qemu/qemu_capabilities.c | 4 ++-- src/qemu/qemu_capabilities.h | 2 +- src/qemu/qemu_command.c | 4 ++-- src/qemu/qemu_domain.c | 10 +++++++--- tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml | 2 +- tests/qemucapabilitiesdata/caps_2.11.0.x86_64.xml | 2 +- tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml | 2 +- tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml | 2 +- tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml | 2 +- tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml | 2 +- tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml | 2 +- tests/qemucapabilitiesdata/caps_3.0.0.riscv32.xml | 2 +- tests/qemucapabilitiesdata/caps_3.0.0.riscv64.xml | 2 +- tests/qemucapabilitiesdata/caps_3.0.0.s390x.xml | 2 +- tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml | 2 +- tests/qemucapabilitiesdata/caps_3.1.0.ppc64.xml | 2 +- tests/qemucapabilitiesdata/caps_3.1.0.x86_64.xml | 2 +- tests/qemucapabilitiesdata/caps_4.0.0.aarch64.xml | 2 +- tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml | 2 +- tests/qemucapabilitiesdata/caps_4.0.0.riscv32.xml | 2 +- tests/qemucapabilitiesdata/caps_4.0.0.riscv64.xml | 2 +- tests/qemucapabilitiesdata/caps_4.0.0.s390x.xml | 2 +- tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml | 2 +- tests/qemucapabilitiesdata/caps_4.1.0.x86_64.xml | 2 +- tests/qemucapabilitiesdata/caps_4.2.0.aarch64.xml | 2 +- tests/qemucapabilitiesdata/caps_4.2.0.ppc64.xml | 2 +- tests/qemucapabilitiesdata/caps_4.2.0.s390x.xml | 2 +- tests/qemucapabilitiesdata/caps_4.2.0.x86_64.xml | 2 +- tests/qemucapabilitiesdata/caps_5.0.0.aarch64.xml | 2 +- tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml | 2 +- tests/qemucapabilitiesdata/caps_5.0.0.riscv64.xml | 2 +- tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml | 2 +- tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml | 2 +- tests/qemustatusxml2xmldata/backup-pull-in.xml | 2 +- tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml | 2 +- tests/qemuxml2argvtest.c | 2 +- 37 files changed, 45 insertions(+), 41 deletions(-)
diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index f89dbd2c3a..99b9ce53e5 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -704,7 +704,7 @@ # If it is unset (or -1), then seccomp will be enabled # only if QEMU >= 2.11.0 is detected, otherwise it is # left disabled. This ensures the default config gets -# protection for new QEMU using the blacklist approach. +# protection for new QEMU with filter groups. # #seccomp_sandbox = 1
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 68fcbd3c4f..310be800e2 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -468,7 +468,7 @@ VIR_ENUM_IMPL(virQEMUCaps, /* 285 */ "qcow2-luks", "pcie-pci-bridge", - "seccomp-blacklist", + "seccomp-filter-groups", "query-cpus-fast", "disk-write-cache",
@@ -3292,7 +3292,7 @@ static struct virQEMUCapsCommandLineProps virQEMUCapsCommandLine[] = { { "vnc", "vnc", QEMU_CAPS_VNC_MULTI_SERVERS }, { "chardev", "reconnect", QEMU_CAPS_CHARDEV_RECONNECT }, { "sandbox", "enable", QEMU_CAPS_SECCOMP_SANDBOX }, - { "sandbox", "elevateprivileges", QEMU_CAPS_SECCOMP_BLACKLIST }, + { "sandbox", "elevateprivileges", QEMU_CAPS_SECCOMP_FILTER_GROUPS }, { "chardev", "fd", QEMU_CAPS_CHARDEV_FD_PASS }, { "overcommit", NULL, QEMU_CAPS_OVERCOMMIT }, { "smp-opts", "dies", QEMU_CAPS_SMP_DIES }, diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index ad93816d41..0ee3e357cb 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -448,7 +448,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */ /* 285 */ QEMU_CAPS_QCOW2_LUKS, /* qcow2 format support LUKS encryption */ QEMU_CAPS_DEVICE_PCIE_PCI_BRIDGE, /* -device pcie-pci-bridge */ - QEMU_CAPS_SECCOMP_BLACKLIST, /* -sandbox.elevateprivileges */ + QEMU_CAPS_SECCOMP_FILTER_GROUPS, /* -sandbox.elevateprivileges */ QEMU_CAPS_QUERY_CPUS_FAST, /* query-cpus-fast command */ QEMU_CAPS_DISK_WRITE_CACHE, /* qemu block frontends support write-cache param */
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index f27246b4c6..37113a433a 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -9517,8 +9517,8 @@ qemuBuildSeccompSandboxCommandLine(virCommandPtr cmd, return 0; }
- /* Use blacklist by default if supported */ - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SECCOMP_BLACKLIST)) { + /* Block undesirable syscall groups by default if supported */ + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SECCOMP_FILTER_GROUPS)) {
While 'filter groups' describes the underlying QEMU functionality better, we only use it to deny syscalls. So using 'blocklist' as proposed in the RFC you linked would better show the contrast between this and the old approach.
I don't want to name it based on libvirt's /current/ usage, as we could alter that usage in future, hence naming it based on the QEMU conceptual feature.
virCommandAddArgList(cmd, "-sandbox", "on,obsolete=deny,elevateprivileges=deny," "spawn=deny,resourcecontrol=deny", diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 72874ee4fd..56ec5c0352 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3851,9 +3851,13 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, if (str) { int flag = virQEMUCapsTypeFromString(str); if (flag < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unknown qemu capabilities flag %s"), str); - goto error; + if (g_str_equal(str, "seccomp-blacklist")) { + flag = QEMU_CAPS_SECCOMP_FILTER_GROUPS;
I'd just leave the XML as-is, to avoid introducing this special-casing.
Renaming the capability lets us eliminate this from all the capabilities test data files we have (and the ones we cointinue to add in future), so I think it is a net win to just have this 2 line special case. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

On a Friday in 2020, Daniel P. Berrangé wrote:
On Fri, Jun 19, 2020 at 01:56:55PM +0200, Ján Tomko wrote:
On a Friday in 2020, Daniel P. Berrangé wrote:
The concept we're really testing for is whether QEMU supports the seccomp syscall filter groups. We need to keep one place using the old term to deal with upgrades from existing hosts with running VMs.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/qemu/qemu.conf | 2 +- src/qemu/qemu_capabilities.c | 4 ++-- src/qemu/qemu_capabilities.h | 2 +- src/qemu/qemu_command.c | 4 ++-- src/qemu/qemu_domain.c | 10 +++++++--- tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml | 2 +- tests/qemucapabilitiesdata/caps_2.11.0.x86_64.xml | 2 +- tests/qemucapabilitiesdata/caps_2.12.0.aarch64.xml | 2 +- tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml | 2 +- tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml | 2 +- tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml | 2 +- tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml | 2 +- tests/qemucapabilitiesdata/caps_3.0.0.riscv32.xml | 2 +- tests/qemucapabilitiesdata/caps_3.0.0.riscv64.xml | 2 +- tests/qemucapabilitiesdata/caps_3.0.0.s390x.xml | 2 +- tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml | 2 +- tests/qemucapabilitiesdata/caps_3.1.0.ppc64.xml | 2 +- tests/qemucapabilitiesdata/caps_3.1.0.x86_64.xml | 2 +- tests/qemucapabilitiesdata/caps_4.0.0.aarch64.xml | 2 +- tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml | 2 +- tests/qemucapabilitiesdata/caps_4.0.0.riscv32.xml | 2 +- tests/qemucapabilitiesdata/caps_4.0.0.riscv64.xml | 2 +- tests/qemucapabilitiesdata/caps_4.0.0.s390x.xml | 2 +- tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml | 2 +- tests/qemucapabilitiesdata/caps_4.1.0.x86_64.xml | 2 +- tests/qemucapabilitiesdata/caps_4.2.0.aarch64.xml | 2 +- tests/qemucapabilitiesdata/caps_4.2.0.ppc64.xml | 2 +- tests/qemucapabilitiesdata/caps_4.2.0.s390x.xml | 2 +- tests/qemucapabilitiesdata/caps_4.2.0.x86_64.xml | 2 +- tests/qemucapabilitiesdata/caps_5.0.0.aarch64.xml | 2 +- tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml | 2 +- tests/qemucapabilitiesdata/caps_5.0.0.riscv64.xml | 2 +- tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml | 2 +- tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml | 2 +- tests/qemustatusxml2xmldata/backup-pull-in.xml | 2 +- tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml | 2 +- tests/qemuxml2argvtest.c | 2 +- 37 files changed, 45 insertions(+), 41 deletions(-)
diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index f89dbd2c3a..99b9ce53e5 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -704,7 +704,7 @@ # If it is unset (or -1), then seccomp will be enabled # only if QEMU >= 2.11.0 is detected, otherwise it is # left disabled. This ensures the default config gets -# protection for new QEMU using the blacklist approach. +# protection for new QEMU with filter groups. # #seccomp_sandbox = 1
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 68fcbd3c4f..310be800e2 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -468,7 +468,7 @@ VIR_ENUM_IMPL(virQEMUCaps, /* 285 */ "qcow2-luks", "pcie-pci-bridge", - "seccomp-blacklist", + "seccomp-filter-groups", "query-cpus-fast", "disk-write-cache",
@@ -3292,7 +3292,7 @@ static struct virQEMUCapsCommandLineProps virQEMUCapsCommandLine[] = { { "vnc", "vnc", QEMU_CAPS_VNC_MULTI_SERVERS }, { "chardev", "reconnect", QEMU_CAPS_CHARDEV_RECONNECT }, { "sandbox", "enable", QEMU_CAPS_SECCOMP_SANDBOX }, - { "sandbox", "elevateprivileges", QEMU_CAPS_SECCOMP_BLACKLIST }, + { "sandbox", "elevateprivileges", QEMU_CAPS_SECCOMP_FILTER_GROUPS }, { "chardev", "fd", QEMU_CAPS_CHARDEV_FD_PASS }, { "overcommit", NULL, QEMU_CAPS_OVERCOMMIT }, { "smp-opts", "dies", QEMU_CAPS_SMP_DIES }, diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index ad93816d41..0ee3e357cb 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -448,7 +448,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */ /* 285 */ QEMU_CAPS_QCOW2_LUKS, /* qcow2 format support LUKS encryption */ QEMU_CAPS_DEVICE_PCIE_PCI_BRIDGE, /* -device pcie-pci-bridge */ - QEMU_CAPS_SECCOMP_BLACKLIST, /* -sandbox.elevateprivileges */ + QEMU_CAPS_SECCOMP_FILTER_GROUPS, /* -sandbox.elevateprivileges */ QEMU_CAPS_QUERY_CPUS_FAST, /* query-cpus-fast command */ QEMU_CAPS_DISK_WRITE_CACHE, /* qemu block frontends support write-cache param */
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index f27246b4c6..37113a433a 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -9517,8 +9517,8 @@ qemuBuildSeccompSandboxCommandLine(virCommandPtr cmd, return 0; }
- /* Use blacklist by default if supported */ - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SECCOMP_BLACKLIST)) { + /* Block undesirable syscall groups by default if supported */ + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SECCOMP_FILTER_GROUPS)) {
While 'filter groups' describes the underlying QEMU functionality better, we only use it to deny syscalls. So using 'blocklist' as proposed in the RFC you linked would better show the contrast between this and the old approach.
I don't want to name it based on libvirt's /current/ usage, as we could alter that usage in future, hence naming it based on the QEMU conceptual feature.
virCommandAddArgList(cmd, "-sandbox", "on,obsolete=deny,elevateprivileges=deny," "spawn=deny,resourcecontrol=deny", diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 72874ee4fd..56ec5c0352 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3851,9 +3851,13 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, if (str) { int flag = virQEMUCapsTypeFromString(str); if (flag < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unknown qemu capabilities flag %s"), str); - goto error; + if (g_str_equal(str, "seccomp-blacklist")) { + flag = QEMU_CAPS_SECCOMP_FILTER_GROUPS;
I'd just leave the XML as-is, to avoid introducing this special-casing.
Renaming the capability lets us eliminate this from all the capabilities test data files we have (and the ones we cointinue to add in future), so I think it is a net win to just have this 2 line special case.
Sigh, Jano

On Fri, Jun 19, 2020 at 13:23:24 +0100, Daniel Berrange wrote:
On Fri, Jun 19, 2020 at 01:56:55PM +0200, Ján Tomko wrote:
On a Friday in 2020, Daniel P. Berrangé wrote:
[...]
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 72874ee4fd..56ec5c0352 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3851,9 +3851,13 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, if (str) { int flag = virQEMUCapsTypeFromString(str); if (flag < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unknown qemu capabilities flag %s"), str); - goto error; + if (g_str_equal(str, "seccomp-blacklist")) { + flag = QEMU_CAPS_SECCOMP_FILTER_GROUPS;
I'd just leave the XML as-is, to avoid introducing this special-casing.
Renaming the capability lets us eliminate this from all the capabilities test data files we have (and the ones we cointinue to add in future), so I think it is a net win to just have this 2 line special case.
I will consider this being a win/improvement of the code if you make it a generic concept for renaming capabilities as I've mentioned in my review. Having a one-off hack seems to be too forced without any possible improvement in the code. I will happily ack it it's made a generic concept.

On Fri, Jun 19, 2020 at 03:05:40PM +0200, Peter Krempa wrote:
On Fri, Jun 19, 2020 at 13:23:24 +0100, Daniel Berrange wrote:
On Fri, Jun 19, 2020 at 01:56:55PM +0200, Ján Tomko wrote:
On a Friday in 2020, Daniel P. Berrangé wrote:
[...]
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 72874ee4fd..56ec5c0352 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3851,9 +3851,13 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, if (str) { int flag = virQEMUCapsTypeFromString(str); if (flag < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unknown qemu capabilities flag %s"), str); - goto error; + if (g_str_equal(str, "seccomp-blacklist")) { + flag = QEMU_CAPS_SECCOMP_FILTER_GROUPS;
I'd just leave the XML as-is, to avoid introducing this special-casing.
Renaming the capability lets us eliminate this from all the capabilities test data files we have (and the ones we cointinue to add in future), so I think it is a net win to just have this 2 line special case.
I will consider this being a win/improvement of the code if you make it a generic concept for renaming capabilities as I've mentioned in my review. Having a one-off hack seems to be too forced without any possible improvement in the code.
I will happily ack it it's made a generic concept.
Yes, that's what I'm already working on doing. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

We can't change the term used by scsi_id for its CLI arg, but we can avoid it by picking the short form instead. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/virstoragefile.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index a6357abb08..b4337c2736 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1365,7 +1365,7 @@ virStorageFileGetSCSIKey(const char *path, cmd = virCommandNewArgList("/lib/udev/scsi_id", "--replace-whitespace", - "--whitelisted", + "-g", "--device", path, NULL ); @@ -1433,7 +1433,7 @@ virStorageFileGetNPIVKey(const char *path, cmd = virCommandNewArgList("/lib/udev/scsi_id", "--replace-whitespace", - "--whitelisted", + "-g", "--export", "--device", path, NULL -- 2.24.1

On Fri, Jun 19, 2020 at 10:32:43 +0100, Daniel Berrange wrote:
We can't change the term used by scsi_id for its CLI arg, but we can avoid it by picking the short form instead.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/virstoragefile.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index a6357abb08..b4337c2736 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1365,7 +1365,7 @@ virStorageFileGetSCSIKey(const char *path,
cmd = virCommandNewArgList("/lib/udev/scsi_id", "--replace-whitespace", - "--whitelisted", + "-g",
IMO this decreases the readability of the code.

On Fri, Jun 19, 2020 at 11:56:07AM +0200, Peter Krempa wrote:
On Fri, Jun 19, 2020 at 10:32:43 +0100, Daniel Berrange wrote:
We can't change the term used by scsi_id for its CLI arg, but we can avoid it by picking the short form instead.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/virstoragefile.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index a6357abb08..b4337c2736 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1365,7 +1365,7 @@ virStorageFileGetSCSIKey(const char *path,
cmd = virCommandNewArgList("/lib/udev/scsi_id", "--replace-whitespace", - "--whitelisted", + "-g",
IMO this decreases the readability of the code.
I agree, but I think that doesn't really matter in the big picture as this is not code that is a serious maint burden in libvirt. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

On Fri, Jun 19, 2020 at 11:17:49 +0100, Daniel Berrange wrote:
On Fri, Jun 19, 2020 at 11:56:07AM +0200, Peter Krempa wrote:
On Fri, Jun 19, 2020 at 10:32:43 +0100, Daniel Berrange wrote:
We can't change the term used by scsi_id for its CLI arg, but we can avoid it by picking the short form instead.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/virstoragefile.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index a6357abb08..b4337c2736 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1365,7 +1365,7 @@ virStorageFileGetSCSIKey(const char *path,
cmd = virCommandNewArgList("/lib/udev/scsi_id", "--replace-whitespace", - "--whitelisted", + "-g",
IMO this decreases the readability of the code.
I agree, but I think that doesn't really matter in the big picture as this is not code that is a serious maint burden in libvirt.
It still a parameter of a tool we use rather than our own. Since there is no technical reason for it and IMO makes the code worse I will not provide the R-B for this patch.

On Fri, Jun 19, 2020 at 12:31:22PM +0200, Peter Krempa wrote:
On Fri, Jun 19, 2020 at 11:17:49 +0100, Daniel Berrange wrote:
On Fri, Jun 19, 2020 at 11:56:07AM +0200, Peter Krempa wrote:
On Fri, Jun 19, 2020 at 10:32:43 +0100, Daniel Berrange wrote:
We can't change the term used by scsi_id for its CLI arg, but we can avoid it by picking the short form instead.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/virstoragefile.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index a6357abb08..b4337c2736 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1365,7 +1365,7 @@ virStorageFileGetSCSIKey(const char *path,
cmd = virCommandNewArgList("/lib/udev/scsi_id", "--replace-whitespace", - "--whitelisted", + "-g",
IMO this decreases the readability of the code.
I agree, but I think that doesn't really matter in the big picture as this is not code that is a serious maint burden in libvirt.
It still a parameter of a tool we use rather than our own. Since there is no technical reason for it and IMO makes the code worse I will not provide the R-B for this patch.
No matter what the spelling used is, I've no idea what the option actually does, or why we need it, not helped by the lack of a manpage for scsi_id on Fedora. So in terms of understanding the code, both the before and after state are bad. What I'll do is add a comment to describe the purpose of adding the option....once I find out what that purpose actually is.... Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

On Fri, Jun 19, 2020 at 12:09:07 +0100, Daniel Berrange wrote:
On Fri, Jun 19, 2020 at 12:31:22PM +0200, Peter Krempa wrote:
On Fri, Jun 19, 2020 at 11:17:49 +0100, Daniel Berrange wrote:
On Fri, Jun 19, 2020 at 11:56:07AM +0200, Peter Krempa wrote:
On Fri, Jun 19, 2020 at 10:32:43 +0100, Daniel Berrange wrote:
We can't change the term used by scsi_id for its CLI arg, but we can avoid it by picking the short form instead.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/virstoragefile.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index a6357abb08..b4337c2736 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1365,7 +1365,7 @@ virStorageFileGetSCSIKey(const char *path,
cmd = virCommandNewArgList("/lib/udev/scsi_id", "--replace-whitespace", - "--whitelisted", + "-g",
IMO this decreases the readability of the code.
I agree, but I think that doesn't really matter in the big picture as this is not code that is a serious maint burden in libvirt.
It still a parameter of a tool we use rather than our own. Since there is no technical reason for it and IMO makes the code worse I will not provide the R-B for this patch.
No matter what the spelling used is, I've no idea what the option actually does, or why we need it, not helped by the lack of a manpage for scsi_id on Fedora. So in terms of understanding the code, both the before and after state are bad. What I'll do is add a comment to describe the purpose of adding the option....once I find out what that purpose actually is....
I'll consider a net win if you describe/document the options used for the command regardless of which spelling is used.

The term is redundant in the context used. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- docs/firewall.html.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/firewall.html.in b/docs/firewall.html.in index e86ab0d974..bfb5a47b1c 100644 --- a/docs/firewall.html.in +++ b/docs/firewall.html.in @@ -311,7 +311,7 @@ f5c78134-9da4-0c60-a9f0-fb37bc21ac1f no-other-rarp-traffic <p>To associate the clean-traffic filter with a guest, edit the guest XML config and change the <code><interface></code> element to include a <code><filterref></code> and also specify the - whitelisted <code><ip address/></code> the guest is allowed to + <code><ip address/></code> that the guest is allowed to use: </p> <pre> -- 2.24.1

On Fri, Jun 19, 2020 at 10:32:44 +0100, Daniel Berrange wrote:
The term is redundant in the context used.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- docs/firewall.html.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>

This naming better matches the semantics usage of the test. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/libvirt_private.syms | 2 +- src/util/virfile.c | 2 +- src/util/virkmod.c | 24 ++++++++++++------------ src/util/virkmod.h | 2 +- src/util/virpci.c | 4 ++-- 5 files changed, 17 insertions(+), 17 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 42f8d7c222..c3ea3840d8 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2403,7 +2403,7 @@ virKeycodeValueTranslate; # util/virkmod.h virKModConfig; -virKModIsBlacklisted; +virKModIsProhibited; virKModLoad; virKModUnload; diff --git a/src/util/virfile.c b/src/util/virfile.c index 20260a2e58..58dfd29304 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -897,7 +897,7 @@ virFileNBDDeviceFindUnused(void) static bool virFileNBDLoadDriver(void) { - if (virKModIsBlacklisted(NBD_DRIVER)) { + if (virKModIsProhibited(NBD_DRIVER)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Failed to load nbd module: " "administratively prohibited")); diff --git a/src/util/virkmod.c b/src/util/virkmod.c index 12cb5920e8..9454581fa8 100644 --- a/src/util/virkmod.c +++ b/src/util/virkmod.c @@ -134,32 +134,32 @@ virKModUnload(const char *module) /** - * virKModIsBlacklisted: - * @module: Name of the module to check for on the blacklist + * virKModIsProhibited: + * @module: Name of the module to check * - * Search the output of the configuration data for the module being - * blacklisted. + * Determine if loading of @module is prohibited by admin + * configuration. * - * returns true when found blacklisted, false otherwise. + * returns true when found prohibited, false otherwise. */ bool -virKModIsBlacklisted(const char *module) +virKModIsProhibited(const char *module) { size_t i; - g_autofree char *drvblklst = NULL; + g_autofree char *drvmatch = NULL; g_autofree char *outbuf = NULL; - drvblklst = g_strdup_printf("blacklist %s\n", module); + drvmatch = g_strdup_printf("blacklist %s\n", module); /* modprobe will convert all '-' into '_', so we need to as well */ - for (i = 0; i < drvblklst[i]; i++) - if (drvblklst[i] == '-') - drvblklst[i] = '_'; + for (i = 0; i < drvmatch[i]; i++) + if (drvmatch[i] == '-') + drvmatch[i] = '_'; if (doModprobe("-c", NULL, &outbuf, NULL) < 0) return false; - if (strstr(outbuf, drvblklst)) + if (strstr(outbuf, drvmatch)) return true; return false; diff --git a/src/util/virkmod.h b/src/util/virkmod.h index f05cf83cf6..bd2fa1a1c7 100644 --- a/src/util/virkmod.h +++ b/src/util/virkmod.h @@ -28,5 +28,5 @@ char *virKModLoad(const char *, bool) ATTRIBUTE_NONNULL(1); char *virKModUnload(const char *) ATTRIBUTE_NONNULL(1); -bool virKModIsBlacklisted(const char *) +bool virKModIsProhibited(const char *) ATTRIBUTE_NONNULL(1); diff --git a/src/util/virpci.c b/src/util/virpci.c index 786b1393e6..e367d5d1e9 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -1023,10 +1023,10 @@ virPCIProbeStubDriver(virPCIStubDriver driver) } cleanup: - /* If we know failure was because of blacklist, let's report that; + /* If we know failure was because of admin config, let's report that; * otherwise, report a more generic failure message */ - if (virKModIsBlacklisted(drvname)) { + if (virKModIsProhibited(drvname)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to load PCI stub module %s: " "administratively prohibited"), -- 2.24.1

On Fri, Jun 19, 2020 at 10:32:45 +0100, Daniel Berrange wrote:
This naming better matches the semantics usage of the test.
I this instance I disagree that it's better as it's diverging from the terminology used by modprobe and doesn't really describe it any better than it did before, so I'd prefer if it's not sold as being better.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/libvirt_private.syms | 2 +- src/util/virfile.c | 2 +- src/util/virkmod.c | 24 ++++++++++++------------ src/util/virkmod.h | 2 +- src/util/virpci.c | 4 ++-- 5 files changed, 17 insertions(+), 17 deletions(-)
I don't have problem with the change per-se, just the justification for it. Reviewed-by: Peter Krempa <pkrempa@redhat.com>

The term "ignored" is a better choice for the filtering performed on devices from udev. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/node_device/node_device_udev.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index bdf0b03add..cec99cb898 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1397,7 +1397,7 @@ udevProcessDeviceListEntry(struct udev *udev, * Do not bother enumerating over subsystems that do not * contain interesting devices. */ -const char *subsystem_blacklist[] = { +const char *subsystem_ignored[] = { "acpi", "tty", "vc", "i2c", }; @@ -1406,8 +1406,8 @@ udevEnumerateAddMatches(struct udev_enumerate *udev_enumerate) { size_t i; - for (i = 0; i < G_N_ELEMENTS(subsystem_blacklist); i++) { - const char *s = subsystem_blacklist[i]; + for (i = 0; i < G_N_ELEMENTS(subsystem_ignored); i++) { + const char *s = subsystem_ignored[i]; if (udev_enumerate_add_nomatch_subsystem(udev_enumerate, s) < 0) { virReportSystemError(errno, "%s", _("failed to add susbsystem filter")); return -1; -- 2.24.1

On Fri, Jun 19, 2020 at 10:32:46 +0100, Daniel Berrange wrote:
The term "ignored" is a better choice for the filtering performed on devices from udev.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/node_device/node_device_udev.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>

The term is redundant or easily substituted with 'prohibit'. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- build-aux/syntax-check.mk | 2 +- m4/virt-compile-warnings.m4 | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk index d47a92b530..2e49b5172e 100644 --- a/build-aux/syntax-check.mk +++ b/build-aux/syntax-check.mk @@ -1155,7 +1155,7 @@ sc_prohibit_class: # exists some filesystems will only ever return DT_UNKNOWN. # This field should only be used by code which is exclusively # run platforms supporting "d_type" and must expect DT_UNKNOWN. -# We blacklist it to discourage accidental usage which has +# We prohibit it to discourage accidental usage which has # happened many times. Add an exclude rule if it is genuinely # needed and the above restrictions are acceptable. sc_prohibit_dirent_d_type: diff --git a/m4/virt-compile-warnings.m4 b/m4/virt-compile-warnings.m4 index d3538d59f8..4ebc25302c 100644 --- a/m4/virt-compile-warnings.m4 +++ b/m4/virt-compile-warnings.m4 @@ -123,7 +123,7 @@ AC_DEFUN([LIBVIRT_COMPILE_WARNINGS],[ # Get all possible GCC warnings gl_MANYWARN_ALL_GCC([maybewarn]) - # Remove the ones we don't want, blacklisted earlier + # Remove the ones we don't want gl_MANYWARN_COMPLEMENT([wantwarn], [$maybewarn], [$dontwarn]) # -Wunused-functin is implied by -Wall we must turn it -- 2.24.1

On Fri, Jun 19, 2020 at 10:32:47 +0100, Daniel Berrange wrote:
The term is redundant or easily substituted with 'prohibit'.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- build-aux/syntax-check.mk | 2 +- m4/virt-compile-warnings.m4 | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk index d47a92b530..2e49b5172e 100644 --- a/build-aux/syntax-check.mk +++ b/build-aux/syntax-check.mk @@ -1155,7 +1155,7 @@ sc_prohibit_class: # exists some filesystems will only ever return DT_UNKNOWN. # This field should only be used by code which is exclusively # run platforms supporting "d_type" and must expect DT_UNKNOWN. -# We blacklist it to discourage accidental usage which has +# We prohibit it to discourage accidental usage which has
This may or may not make sence depending on how syntax check is ported to meson. You can use my R-b for this hunk if this survives the rewrite.
# happened many times. Add an exclude rule if it is genuinely # needed and the above restrictions are acceptable. sc_prohibit_dirent_d_type: diff --git a/m4/virt-compile-warnings.m4 b/m4/virt-compile-warnings.m4 index d3538d59f8..4ebc25302c 100644 --- a/m4/virt-compile-warnings.m4 +++ b/m4/virt-compile-warnings.m4 @@ -123,7 +123,7 @@ AC_DEFUN([LIBVIRT_COMPILE_WARNINGS],[ # Get all possible GCC warnings gl_MANYWARN_ALL_GCC([maybewarn])
- # Remove the ones we don't want, blacklisted earlier + # Remove the ones we don't want gl_MANYWARN_COMPLEMENT([wantwarn], [$maybewarn], [$dontwarn])
IMO this change doesn't make sense in the light of the meson rewrite.
# -Wunused-functin is implied by -Wall we must turn it -- 2.24.1

On Fri, Jun 19, 2020 at 12:18:23PM +0200, Peter Krempa wrote:
On Fri, Jun 19, 2020 at 10:32:47 +0100, Daniel Berrange wrote:
The term is redundant or easily substituted with 'prohibit'.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- build-aux/syntax-check.mk | 2 +- m4/virt-compile-warnings.m4 | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk index d47a92b530..2e49b5172e 100644 --- a/build-aux/syntax-check.mk +++ b/build-aux/syntax-check.mk @@ -1155,7 +1155,7 @@ sc_prohibit_class: # exists some filesystems will only ever return DT_UNKNOWN. # This field should only be used by code which is exclusively # run platforms supporting "d_type" and must expect DT_UNKNOWN. -# We blacklist it to discourage accidental usage which has +# We prohibit it to discourage accidental usage which has
This may or may not make sence depending on how syntax check is ported to meson. You can use my R-b for this hunk if this survives the rewrite.
I'd expect this file to be unchanged with meson. This is why we changed it to be independant of the current makefile infra when getting rid of gnulib.
# happened many times. Add an exclude rule if it is genuinely # needed and the above restrictions are acceptable. sc_prohibit_dirent_d_type: diff --git a/m4/virt-compile-warnings.m4 b/m4/virt-compile-warnings.m4 index d3538d59f8..4ebc25302c 100644 --- a/m4/virt-compile-warnings.m4 +++ b/m4/virt-compile-warnings.m4 @@ -123,7 +123,7 @@ AC_DEFUN([LIBVIRT_COMPILE_WARNINGS],[ # Get all possible GCC warnings gl_MANYWARN_ALL_GCC([maybewarn])
- # Remove the ones we don't want, blacklisted earlier + # Remove the ones we don't want gl_MANYWARN_COMPLEMENT([wantwarn], [$maybewarn], [$dontwarn])
IMO this change doesn't make sense in the light of the meson rewrite.
Sure, this will go away when meson switch arrives, but we've still not got a clear ETA for that, so we shouldn't reject changes to the existing build system in the meantime. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

On Mon, Jun 22, 2020 at 12:14:56 +0100, Daniel Berrange wrote:
On Fri, Jun 19, 2020 at 12:18:23PM +0200, Peter Krempa wrote:
On Fri, Jun 19, 2020 at 10:32:47 +0100, Daniel Berrange wrote:
The term is redundant or easily substituted with 'prohibit'.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> ---
[...]
diff --git a/m4/virt-compile-warnings.m4 b/m4/virt-compile-warnings.m4 index d3538d59f8..4ebc25302c 100644 --- a/m4/virt-compile-warnings.m4 +++ b/m4/virt-compile-warnings.m4 @@ -123,7 +123,7 @@ AC_DEFUN([LIBVIRT_COMPILE_WARNINGS],[ # Get all possible GCC warnings gl_MANYWARN_ALL_GCC([maybewarn])
- # Remove the ones we don't want, blacklisted earlier + # Remove the ones we don't want gl_MANYWARN_COMPLEMENT([wantwarn], [$maybewarn], [$dontwarn])
IMO this change doesn't make sense in the light of the meson rewrite.
Sure, this will go away when meson switch arrives, but we've still not got a clear ETA for that, so we shouldn't reject changes to the existing build system in the meantime.
Main reason is to minimize conflicts with Pavel's effort. This hunk is here since 2012, so waiting even for amonth doesn't seem to be that much of a problem.

The terms can be avoided with simple tweaks. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/libvirt.c | 2 +- src/qemu/qemu_command.c | 2 +- src/qemu/qemu_conf.c | 4 ++-- src/rpc/gendispatch.pl | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/libvirt.c b/src/libvirt.c index b2d0ba3d23..a0a21fd5d2 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -1025,7 +1025,7 @@ virConnectOpenInternal(const char *name, bool matchScheme = false; size_t s; if (!ret->uri) { - VIR_DEBUG("No URI, skipping driver with URI whitelist"); + VIR_DEBUG("No URI, skipping driver with URI scheme filtering"); continue; } if (embed && !virConnectDriverTab[i]->embeddable) { diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 37113a433a..ff539b1556 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -9526,7 +9526,7 @@ qemuBuildSeccompSandboxCommandLine(virCommandPtr cmd, return 0; } - /* Seccomp whitelist is opt-in */ + /* Seccomp sandbox is opt-in */ if (cfg->seccompSandbox > 0) virCommandAddArgList(cmd, "-sandbox", "on", NULL); diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 33b3989268..c54f82a75f 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1854,8 +1854,8 @@ qemuSetUnprivSGIO(virDomainDeviceDefPtr dev) /* By default, filter the SG_IO commands, i.e. set unpriv_sgio to 0. */ val = (disk->sgio == VIR_DOMAIN_DEVICE_SGIO_UNFILTERED); - /* Do not do anything if unpriv_sgio is not supported by the kernel and the - * whitelist is enabled. But if requesting unfiltered access, always call + /* Do not do anything if unpriv_sgio is not supported by the kernel and + * filtering is enabled. But if requesting unfiltered access, always call * virSetDeviceUnprivSGIO, to report an error for unsupported unpriv_sgio. */ if ((virFileExists(sysfs_path) || val == 1) && diff --git a/src/rpc/gendispatch.pl b/src/rpc/gendispatch.pl index 0b2ae59910..6c947beee1 100755 --- a/src/rpc/gendispatch.pl +++ b/src/rpc/gendispatch.pl @@ -171,7 +171,7 @@ sub get_conn_method { if ($proc =~ /Connect.*Network/) { return "remoteGetNetworkConn"; } - # Carefully whitelist a few APIs with NodeDevice name + # Special case a few APIs with NodeDevice name # prefix which actually get handled by the virt drivers if ($proc =~ /Node.*Device/ && !($proc =~ /NodeDeviceReset/ || -- 2.24.1

On Fri, Jun 19, 2020 at 10:32:48 +0100, Daniel Berrange wrote:
The terms can be avoided with simple tweaks.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/libvirt.c | 2 +- src/qemu/qemu_command.c | 2 +- src/qemu/qemu_conf.c | 4 ++-- src/rpc/gendispatch.pl | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-)
Again looks like too many changes crammed together, but given that it's mostly comments: Reviewed-by: Peter Krempa <pkrempa@redhat.com>

On a Friday in 2020, Daniel P. Berrangé wrote:
The terms can be avoided with simple tweaks.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/libvirt.c | 2 +- src/qemu/qemu_command.c | 2 +- src/qemu/qemu_conf.c | 4 ++-- src/rpc/gendispatch.pl | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/src/libvirt.c b/src/libvirt.c index b2d0ba3d23..a0a21fd5d2 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -1025,7 +1025,7 @@ virConnectOpenInternal(const char *name, bool matchScheme = false; size_t s; if (!ret->uri) { - VIR_DEBUG("No URI, skipping driver with URI whitelist"); + VIR_DEBUG("No URI, skipping driver with URI scheme filtering"); continue; } if (embed && !virConnectDriverTab[i]->embeddable) { diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 37113a433a..ff539b1556 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -9526,7 +9526,7 @@ qemuBuildSeccompSandboxCommandLine(virCommandPtr cmd, return 0; }
- /* Seccomp whitelist is opt-in */ + /* Seccomp sandbox is opt-in */
This is incorrect, we use the seccomp sandbox by default on newer QEMUs. Jano

On Fri, Jun 19, 2020 at 01:43:46PM +0200, Ján Tomko wrote:
On a Friday in 2020, Daniel P. Berrangé wrote:
The terms can be avoided with simple tweaks.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/libvirt.c | 2 +- src/qemu/qemu_command.c | 2 +- src/qemu/qemu_conf.c | 4 ++-- src/rpc/gendispatch.pl | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/src/libvirt.c b/src/libvirt.c index b2d0ba3d23..a0a21fd5d2 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -1025,7 +1025,7 @@ virConnectOpenInternal(const char *name, bool matchScheme = false; size_t s; if (!ret->uri) { - VIR_DEBUG("No URI, skipping driver with URI whitelist"); + VIR_DEBUG("No URI, skipping driver with URI scheme filtering"); continue; } if (embed && !virConnectDriverTab[i]->embeddable) { diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 37113a433a..ff539b1556 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -9526,7 +9526,7 @@ qemuBuildSeccompSandboxCommandLine(virCommandPtr cmd, return 0; }
- /* Seccomp whitelist is opt-in */ + /* Seccomp sandbox is opt-in */
This is incorrect, we use the seccomp sandbox by default on newer QEMUs.
This comment is against the code that deals with the old style QEMU seccomp code, which rquires an explicit opt-in. The new style seccom is further up above the diff hunk context seen here. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

On a Monday in 2020, Daniel P. Berrangé wrote:
On Fri, Jun 19, 2020 at 01:43:46PM +0200, Ján Tomko wrote:
On a Friday in 2020, Daniel P. Berrangé wrote:
The terms can be avoided with simple tweaks.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/libvirt.c | 2 +- src/qemu/qemu_command.c | 2 +- src/qemu/qemu_conf.c | 4 ++-- src/rpc/gendispatch.pl | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/src/libvirt.c b/src/libvirt.c index b2d0ba3d23..a0a21fd5d2 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -1025,7 +1025,7 @@ virConnectOpenInternal(const char *name, bool matchScheme = false; size_t s; if (!ret->uri) { - VIR_DEBUG("No URI, skipping driver with URI whitelist"); + VIR_DEBUG("No URI, skipping driver with URI scheme filtering"); continue; } if (embed && !virConnectDriverTab[i]->embeddable) { diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 37113a433a..ff539b1556 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -9526,7 +9526,7 @@ qemuBuildSeccompSandboxCommandLine(virCommandPtr cmd, return 0; }
- /* Seccomp whitelist is opt-in */ + /* Seccomp sandbox is opt-in */
This is incorrect, we use the seccomp sandbox by default on newer QEMUs.
This comment is against the code that deals with the old style QEMU seccomp code, which rquires an explicit opt-in. The new style seccom is further up above the diff hunk context seen here.
I am specifically talking about the context. I wrote the comment to contrast against the new-style sandbox. If you don't want to preserve the contrast in here, just delete the comment. Jano
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

Two methods needs to deal with the fixed filename prefix on the NIC name used by bond devices. The code is clearer if they both make use of a defined constant. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/interface/interface_backend_udev.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/src/interface/interface_backend_udev.c b/src/interface/interface_backend_udev.c index e388f98536..f0594aa59c 100644 --- a/src/interface/interface_backend_udev.c +++ b/src/interface/interface_backend_udev.c @@ -527,8 +527,16 @@ udevInterfaceLookupByMACString(virConnectPtr conn, const char *macstr) return ret; } +/* + * Name prefix for sysfs symlinks that indicate which + * NICs are part of the bonded device + */ +#define BOND_NIC_PREFIX "slave_" + /** - * Helper function for finding bond slaves using scandir() + * Helper function for finding NICs that are members of a + * bond device by using scandir() over the sysfs net interface + * dir * * @param entry - directory entry passed by scandir() * @@ -542,7 +550,7 @@ udevBondScanDirFilter(const struct dirent *entry) * interface sysfs entry and references the slaves as slave_eth0 for * example. */ - if (STRPREFIX(entry->d_name, "slave_")) + if (STRPREFIX(entry->d_name, BOND_NIC_PREFIX)) return 1; return 0; @@ -763,21 +771,15 @@ udevGetIfaceDefBond(struct udev *udev, ifacedef->data.bond.nbItf = slave_count; for (i = 0; i < slave_count; i++) { - /* Names are slave_interface. e.g. slave_eth0 - * so we use the part after the _ - */ - tmp_str = strchr(slave_list[i]->d_name, '_'); - if (!tmp_str || strlen(tmp_str) < 2) { + if (!STRPREFIX(nic_list[i]->d_name, BOND_NIC_PREFIX)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid enslaved interface name '%s' seen for " "bond '%s'"), slave_list[i]->d_name, name); goto error; } - /* go past the _ */ - tmp_str++; ifacedef->data.bond.itf[i] = - udevGetIfaceDef(udev, tmp_str); + udevGetIfaceDef(udev, nic_list[i]->d_name + strlen(BOND_NIC_PREFIX)); if (!ifacedef->data.bond.itf[i]) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not get interface information for '%s', which is " -- 2.24.1

On Fri, Jun 19, 2020 at 10:32:49 +0100, Daniel Berrange wrote:
Two methods needs to deal with the fixed filename prefix on the NIC name used by bond devices. The code is clearer if they both make use of a defined constant.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/interface/interface_backend_udev.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-)
diff --git a/src/interface/interface_backend_udev.c b/src/interface/interface_backend_udev.c index e388f98536..f0594aa59c 100644 --- a/src/interface/interface_backend_udev.c +++ b/src/interface/interface_backend_udev.c @@ -527,8 +527,16 @@ udevInterfaceLookupByMACString(virConnectPtr conn, const char *macstr) return ret; }
+/* + * Name prefix for sysfs symlinks that indicate which + * NICs are part of the bonded device + */ +#define BOND_NIC_PREFIX "slave_" + /** - * Helper function for finding bond slaves using scandir() + * Helper function for finding NICs that are members of a + * bond device by using scandir() over the sysfs net interface + * dir * * @param entry - directory entry passed by scandir() * @@ -542,7 +550,7 @@ udevBondScanDirFilter(const struct dirent *entry) * interface sysfs entry and references the slaves as slave_eth0 for * example. */ - if (STRPREFIX(entry->d_name, "slave_")) + if (STRPREFIX(entry->d_name, BOND_NIC_PREFIX)) return 1;
return 0; @@ -763,21 +771,15 @@ udevGetIfaceDefBond(struct udev *udev, ifacedef->data.bond.nbItf = slave_count;
for (i = 0; i < slave_count; i++) { - /* Names are slave_interface. e.g. slave_eth0 - * so we use the part after the _ - */ - tmp_str = strchr(slave_list[i]->d_name, '_'); - if (!tmp_str || strlen(tmp_str) < 2) { + if (!STRPREFIX(nic_list[i]->d_name, BOND_NIC_PREFIX)) {
Fails to compile: .../libvirt/src/interface/interface_backend_udev.c:774:24: error: 'nic_list' undeclared (first use in this function) 774 | if (!STRPREFIX(nic_list[i]->d_name, BOND_NIC_PREFIX)) { | ^~~~~~~~
virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid enslaved interface name '%s' seen for " "bond '%s'"), slave_list[i]->d_name, name); goto error; } - /* go past the _ */ - tmp_str++;
ifacedef->data.bond.itf[i] = - udevGetIfaceDef(udev, tmp_str); + udevGetIfaceDef(udev, nic_list[i]->d_name + strlen(BOND_NIC_PREFIX)); if (!ifacedef->data.bond.itf[i]) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not get interface information for '%s', which is " -- 2.24.1

We can't change the filenames used in sysfs, but we don't have to use that terminology in our code processing the list of NICs. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- docs/schemas/interface.rng | 2 +- src/interface/interface_backend_udev.c | 43 ++++++++++++-------------- 2 files changed, 21 insertions(+), 24 deletions(-) diff --git a/docs/schemas/interface.rng b/docs/schemas/interface.rng index a4fddaaedc..2530077190 100644 --- a/docs/schemas/interface.rng +++ b/docs/schemas/interface.rng @@ -237,7 +237,7 @@ </optional> <oneOrMore> - <!-- The slave interfaces --> + <!-- The attached interfaces --> <ref name="bare-ethernet-interface"/> </oneOrMore> </interleave> diff --git a/src/interface/interface_backend_udev.c b/src/interface/interface_backend_udev.c index f0594aa59c..f5961a93df 100644 --- a/src/interface/interface_backend_udev.c +++ b/src/interface/interface_backend_udev.c @@ -546,9 +546,7 @@ static int udevBondScanDirFilter(const struct dirent *entry) { /* This is ugly so if anyone has a better suggestion, please improve - * this. Unfortunately the kernel stores everything in the top level - * interface sysfs entry and references the slaves as slave_eth0 for - * example. + * this. */ if (STRPREFIX(entry->d_name, BOND_NIC_PREFIX)) return 1; @@ -591,8 +589,8 @@ udevGetIfaceDefBond(struct udev *udev, const char *name, virInterfaceDef *ifacedef) { - struct dirent **slave_list = NULL; - int slave_count = 0; + struct dirent **nic_list = NULL; + int nic_count = 0; size_t i; const char *tmp_str; int tmp_int; @@ -754,27 +752,26 @@ udevGetIfaceDefBond(struct udev *udev, } ifacedef->data.bond.target = g_strdup(tmp_str); - /* Slaves of the bond */ - /* Get each slave in the bond */ - slave_count = scandir(udev_device_get_syspath(dev), &slave_list, + /* Get each NIC that is a member of the bond */ + nic_count = scandir(udev_device_get_syspath(dev), &nic_list, udevBondScanDirFilter, alphasort); - if (slave_count < 0) { + if (nic_count < 0) { virReportSystemError(errno, - _("Could not get slaves of bond '%s'"), name); + _("Could not get interfaces for bond '%s'"), name); goto error; } - /* Allocate our list of slave devices */ - if (VIR_ALLOC_N(ifacedef->data.bond.itf, slave_count) < 0) + /* Allocate our list of bonded devices */ + if (VIR_ALLOC_N(ifacedef->data.bond.itf, nic_count) < 0) goto error; - ifacedef->data.bond.nbItf = slave_count; + ifacedef->data.bond.nbItf = nic_count; - for (i = 0; i < slave_count; i++) { + for (i = 0; i < nic_count; i++) { if (!STRPREFIX(nic_list[i]->d_name, BOND_NIC_PREFIX)) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("Invalid enslaved interface name '%s' seen for " - "bond '%s'"), slave_list[i]->d_name, name); + _("Invalid bonded interface name '%s' seen for " + "bond '%s'"), nic_list[i]->d_name, name); goto error; } @@ -782,21 +779,21 @@ udevGetIfaceDefBond(struct udev *udev, udevGetIfaceDef(udev, nic_list[i]->d_name + strlen(BOND_NIC_PREFIX)); if (!ifacedef->data.bond.itf[i]) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("Could not get interface information for '%s', which is " - "a enslaved in bond '%s'"), slave_list[i]->d_name, name); + _("Could not get interface information for '%s', which " + "is part of bond '%s'"), nic_list[i]->d_name, name); goto error; } - VIR_FREE(slave_list[i]); + VIR_FREE(nic_list[i]); } - VIR_FREE(slave_list); + VIR_FREE(nic_list); return 0; error: - for (i = 0; slave_count != -1 && i < slave_count; i++) - VIR_FREE(slave_list[i]); - VIR_FREE(slave_list); + for (i = 0; nic_count != -1 && i < nic_count; i++) + VIR_FREE(nic_list[i]); + VIR_FREE(nic_list); return -1; } -- 2.24.1

On Fri, Jun 19, 2020 at 10:32:50 +0100, Daniel Berrange wrote:
We can't change the filenames used in sysfs, but we don't have to use that terminology in our code processing the list of NICs.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- docs/schemas/interface.rng | 2 +- src/interface/interface_backend_udev.c | 43 ++++++++++++-------------- 2 files changed, 21 insertions(+), 24 deletions(-)
diff --git a/docs/schemas/interface.rng b/docs/schemas/interface.rng index a4fddaaedc..2530077190 100644 --- a/docs/schemas/interface.rng +++ b/docs/schemas/interface.rng @@ -237,7 +237,7 @@ </optional>
<oneOrMore> - <!-- The slave interfaces --> + <!-- The attached interfaces --> <ref name="bare-ethernet-interface"/> </oneOrMore> </interleave> diff --git a/src/interface/interface_backend_udev.c b/src/interface/interface_backend_udev.c index f0594aa59c..f5961a93df 100644 --- a/src/interface/interface_backend_udev.c +++ b/src/interface/interface_backend_udev.c @@ -546,9 +546,7 @@ static int udevBondScanDirFilter(const struct dirent *entry) { /* This is ugly so if anyone has a better suggestion, please improve - * this. Unfortunately the kernel stores everything in the top level - * interface sysfs entry and references the slaves as slave_eth0 for - * example. + * this. */ if (STRPREFIX(entry->d_name, BOND_NIC_PREFIX)) return 1; @@ -591,8 +589,8 @@ udevGetIfaceDefBond(struct udev *udev, const char *name, virInterfaceDef *ifacedef) { - struct dirent **slave_list = NULL; - int slave_count = 0; + struct dirent **nic_list = NULL; + int nic_count = 0;
Patch ordering problem with previous one?

On Fri, Jun 19, 2020 at 12:25:49PM +0200, Peter Krempa wrote:
On Fri, Jun 19, 2020 at 10:32:50 +0100, Daniel Berrange wrote:
We can't change the filenames used in sysfs, but we don't have to use that terminology in our code processing the list of NICs.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- docs/schemas/interface.rng | 2 +- src/interface/interface_backend_udev.c | 43 ++++++++++++-------------- 2 files changed, 21 insertions(+), 24 deletions(-)
diff --git a/docs/schemas/interface.rng b/docs/schemas/interface.rng index a4fddaaedc..2530077190 100644 --- a/docs/schemas/interface.rng +++ b/docs/schemas/interface.rng @@ -237,7 +237,7 @@ </optional>
<oneOrMore> - <!-- The slave interfaces --> + <!-- The attached interfaces --> <ref name="bare-ethernet-interface"/> </oneOrMore> </interleave> diff --git a/src/interface/interface_backend_udev.c b/src/interface/interface_backend_udev.c index f0594aa59c..f5961a93df 100644 --- a/src/interface/interface_backend_udev.c +++ b/src/interface/interface_backend_udev.c @@ -546,9 +546,7 @@ static int udevBondScanDirFilter(const struct dirent *entry) { /* This is ugly so if anyone has a better suggestion, please improve - * this. Unfortunately the kernel stores everything in the top level - * interface sysfs entry and references the slaves as slave_eth0 for - * example. + * this. */ if (STRPREFIX(entry->d_name, BOND_NIC_PREFIX)) return 1; @@ -591,8 +589,8 @@ udevGetIfaceDefBond(struct udev *udev, const char *name, virInterfaceDef *ifacedef) { - struct dirent **slave_list = NULL; - int slave_count = 0; + struct dirent **nic_list = NULL; + int nic_count = 0;
Patch ordering problem with previous one?
Opps, yes, will fix. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

Network interfaces are simply attached to a bridge device. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- docs/firewall.html.in | 4 ++-- docs/formatdomain.html.in | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/firewall.html.in b/docs/firewall.html.in index bfb5a47b1c..2d55021d9b 100644 --- a/docs/firewall.html.in +++ b/docs/firewall.html.in @@ -12,7 +12,7 @@ <li>The virtual network driver <br /><br /> This provides an isolated bridge device (ie no physical NICs - enslaved). Guest TAP devices are attached to this bridge. + attached). Guest TAP devices are attached to this bridge. Guests can talk to each other and the host, and optionally the wider world. <br /><br /> @@ -46,7 +46,7 @@ </p> <p>Thus the virtual network driver in libvirt was invented. This takes the form of an isolated bridge device (ie one with no physical NICs - enslaved). The TAP devices associated with the guest NICs are attached + attached). The TAP devices associated with the guest NICs are attached to the bridge device. This immediately allows guests on a single host to talk to each other and to the host OS (modulo host IPtables rules). </p> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 07d0fa5c70..3f4d633909 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -5785,11 +5785,11 @@ <p> Provides a bridge from the VM directly to the LAN. This assumes there is a bridge device on the host which has one or more of the hosts - physical NICs enslaved. The guest VM will have an associated tun device + physical NICs attached. The guest VM will have an associated tun device created with a name of vnetN, which can also be overridden with the <target> element (see <a href="#elementsNICSTargetOverride">overriding the target element</a>). - The tun device will be enslaved to the bridge. The IP range / network + The tun device will be attached to the bridge. The IP range / network configuration is whatever is used on the LAN. This provides the guest VM full incoming & outgoing net access just like a physical machine. </p> -- 2.24.1

On Fri, Jun 19, 2020 at 10:32:51 +0100, Daniel Berrange wrote:
Network interfaces are simply attached to a bridge device.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- docs/firewall.html.in | 4 ++-- docs/formatdomain.html.in | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>

Network interfaces are simply attached to a bridge device. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- tools/virsh-interface.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/tools/virsh-interface.c b/tools/virsh-interface.c index a3cdf8630f..d754998d52 100644 --- a/tools/virsh-interface.c +++ b/tools/virsh-interface.c @@ -922,17 +922,17 @@ cmdInterfaceBridge(vshControl *ctl, const vshCmd *cmd) goto cleanup; } - /* set the type of the inner/slave interface to the original + /* set the type of the attached interface to the original * if_type, and the name to the original if_name. */ if (!xmlSetProp(if_node, BAD_CAST "type", BAD_CAST if_type)) { - vshError(ctl, _("Failed to set new slave interface type to '%s' in xml document"), + vshError(ctl, _("Failed to set new attached interface type to '%s' in xml document"), if_type); goto cleanup; } if (!xmlSetProp(if_node, BAD_CAST "name", BAD_CAST if_name)) { - vshError(ctl, _("Failed to set new slave interface name to '%s' in xml document"), + vshError(ctl, _("Failed to set new attached interface name to '%s' in xml document"), if_name); goto cleanup; } @@ -1010,7 +1010,7 @@ cmdInterfaceBridge(vshControl *ctl, const vshCmd *cmd) */ static const vshCmdInfo info_interface_unbridge[] = { {.name = "help", - .data = N_("undefine a bridge device after detaching its slave device") + .data = N_("undefine a bridge device after detaching its device(s)") }, {.name = "desc", .data = N_("unbridge a network device") @@ -1026,7 +1026,7 @@ static const vshCmdOptDef opts_interface_unbridge[] = { }, {.name = "no-start", .type = VSH_OT_BOOL, - .help = N_("don't start the un-slaved interface immediately (not recommended)") + .help = N_("don't start the detached interface immediately (not recommended)") }, {.name = NULL} }; @@ -1103,8 +1103,8 @@ cmdInterfaceUnbridge(vshControl *ctl, const vshCmd *cmd) goto cleanup; } - /* Change the type and name of the outer/master interface to - * the type/name of the attached slave interface. + /* Change the type and name of the bridge interface to + * the type/name of the attached interface. */ if (!(if_name = virXMLPropString(if_node, "name"))) { vshError(ctl, _("Device attached to bridge %s has no name"), br_name); @@ -1154,7 +1154,7 @@ cmdInterfaceUnbridge(vshControl *ctl, const vshCmd *cmd) xmlDocDumpMemory(xml_doc, &if_xml, &if_xml_size); if (!if_xml || if_xml_size <= 0) { - vshError(ctl, _("Failed to format new xml document for un-enslaved interface %s"), + vshError(ctl, _("Failed to format new xml document for detached interface %s"), if_name); goto cleanup; } -- 2.24.1

On Fri, Jun 19, 2020 at 10:32:52 +0100, Daniel Berrange wrote:
Network interfaces are simply attached to a bridge device.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- tools/virsh-interface.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>

The two sides of a PTY can be referred to as primary and secondary TTYs. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/virfile.c | 40 ++++++++++++++++++++-------------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index 58dfd29304..1eebcf1b22 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -3127,20 +3127,20 @@ virFileBuildPath(const char *dir, const char *name, const char *ext) return path; } -/* Open a non-blocking master side of a pty. If ttyName is not NULL, - * then populate it with the name of the slave. If rawmode is set, - * also put the master side into raw mode before returning. */ +/* Open a non-blocking primary side of a pty. If ttyName is not NULL, + * then populate it with the name of the secondary peer. If rawmode is + * set, also put the primary side into raw mode before returning. */ #ifndef WIN32 int -virFileOpenTty(int *ttymaster, char **ttyName, int rawmode) +virFileOpenTty(int *ttyprimary, char **ttyName, int rawmode) { /* XXX A word of caution - on some platforms (Solaris and HP-UX), - * additional ioctl() calls are needs after opening the slave + * additional ioctl() calls are needs after opening the secondary * before it will cause isatty() to return true. Should we make - * virFileOpenTty also return the opened slave fd, so the caller + * virFileOpenTty also return the opened secondary fd, so the caller * doesn't have to worry about that mess? */ int ret = -1; - int slave = -1; + int secondary = -1; g_autofree char *name = NULL; /* Unfortunately, we can't use the name argument of openpty, since @@ -3148,31 +3148,31 @@ virFileOpenTty(int *ttymaster, char **ttyName, int rawmode) * Likewise, we can't use the termios argument: we have to use * read-modify-write since there is no portable way to initialize * a struct termios without use of tcgetattr. */ - if (openpty(ttymaster, &slave, NULL, NULL, NULL) < 0) + if (openpty(ttyprimary, &secondary, NULL, NULL, NULL) < 0) return -1; /* What a shame that openpty cannot atomically set FD_CLOEXEC, but * that using posix_openpt/grantpt/unlockpt/ptsname is not * thread-safe, and that ptsname_r is not portable. */ - if (virSetNonBlock(*ttymaster) < 0 || - virSetCloseExec(*ttymaster) < 0) + if (virSetNonBlock(*ttyprimary) < 0 || + virSetCloseExec(*ttyprimary) < 0) goto cleanup; - /* While Linux supports tcgetattr on either the master or the - * slave, Solaris requires it to be on the slave. */ + /* While Linux supports tcgetattr on either the primary or the + * secondary, Solaris requires it to be on the secondary. */ if (rawmode) { struct termios ttyAttr; - if (tcgetattr(slave, &ttyAttr) < 0) + if (tcgetattr(secondary, &ttyAttr) < 0) goto cleanup; cfmakeraw(&ttyAttr); - if (tcsetattr(slave, TCSADRAIN, &ttyAttr) < 0) + if (tcsetattr(secondary, TCSADRAIN, &ttyAttr) < 0) goto cleanup; } - /* ttyname_r on the slave is required by POSIX, while ptsname_r on - * the master is a glibc extension, and the POSIX ptsname is not + /* ttyname_r on the secondary is required by POSIX, while ptsname_r on + * the primary is a glibc extension, and the POSIX ptsname is not * thread-safe. Since openpty gave us both descriptors, guess * which way we will determine the name? :) */ if (ttyName) { @@ -3184,7 +3184,7 @@ virFileOpenTty(int *ttymaster, char **ttyName, int rawmode) if (VIR_ALLOC_N(name, len) < 0) goto cleanup; - while ((rc = ttyname_r(slave, name, len)) == ERANGE) { + while ((rc = ttyname_r(secondary, name, len)) == ERANGE) { if (VIR_RESIZE_N(name, len, len, len) < 0) goto cleanup; } @@ -3200,14 +3200,14 @@ virFileOpenTty(int *ttymaster, char **ttyName, int rawmode) cleanup: if (ret != 0) - VIR_FORCE_CLOSE(*ttymaster); - VIR_FORCE_CLOSE(slave); + VIR_FORCE_CLOSE(*ttyprimary); + VIR_FORCE_CLOSE(secondary); return ret; } #else /* WIN32 */ int -virFileOpenTty(int *ttymaster G_GNUC_UNUSED, +virFileOpenTty(int *ttyprimary G_GNUC_UNUSED, char **ttyName G_GNUC_UNUSED, int rawmode G_GNUC_UNUSED) { -- 2.24.1

On Fri, Jun 19, 2020 at 10:32:53 +0100, Daniel Berrange wrote:
The two sides of a PTY can be referred to as primary and secondary TTYs.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/virfile.c | 40 ++++++++++++++++++++-------------------- 1 file changed, 20 insertions(+), 20 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com

When querying QEMU we have to iterate over two nested sets of CPUs. The terms "main vcpu" and "sub vcpu" are a good representation. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/qemu/qemu_monitor.c | 46 ++++++++++++++++++++--------------------- 1 file changed, 23 insertions(+), 23 deletions(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 3ec22b939f..5033cbeabf 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -1734,8 +1734,8 @@ qemuMonitorGetCPUInfoHotplug(struct qemuMonitorQueryHotpluggableCpusEntry *hotpl char *tmp; int order = 1; size_t totalvcpus = 0; - size_t mastervcpu; /* this iterator is used for iterating hotpluggable entities */ - size_t slavevcpu; /* this corresponds to subentries of a hotpluggable entry */ + size_t mainvcpu; /* this iterator is used for iterating hotpluggable entities */ + size_t subvcpu; /* this corresponds to subentries of a hotpluggable entry */ size_t anyvcpu; /* this iterator is used for any vcpu entry in the result */ size_t i; size_t j; @@ -1777,31 +1777,31 @@ qemuMonitorGetCPUInfoHotplug(struct qemuMonitorQueryHotpluggableCpusEntry *hotpl /* transfer appropriate data from the hotpluggable list to corresponding * entries. the entries returned by qemu may in fact describe multiple * logical vcpus in the guest */ - mastervcpu = 0; + mainvcpu = 0; for (i = 0; i < nhotplugvcpus; i++) { - vcpus[mastervcpu].online = !!hotplugvcpus[i].qom_path; - vcpus[mastervcpu].hotpluggable = !!hotplugvcpus[i].alias || - !vcpus[mastervcpu].online; - vcpus[mastervcpu].socket_id = hotplugvcpus[i].socket_id; - vcpus[mastervcpu].die_id = hotplugvcpus[i].die_id; - vcpus[mastervcpu].core_id = hotplugvcpus[i].core_id; - vcpus[mastervcpu].thread_id = hotplugvcpus[i].thread_id; - vcpus[mastervcpu].node_id = hotplugvcpus[i].node_id; - vcpus[mastervcpu].vcpus = hotplugvcpus[i].vcpus; - vcpus[mastervcpu].qom_path = g_steal_pointer(&hotplugvcpus[i].qom_path); - vcpus[mastervcpu].alias = g_steal_pointer(&hotplugvcpus[i].alias); - vcpus[mastervcpu].type = g_steal_pointer(&hotplugvcpus[i].type); - vcpus[mastervcpu].props = g_steal_pointer(&hotplugvcpus[i].props); - vcpus[mastervcpu].id = hotplugvcpus[i].enable_id; - - /* copy state information to slave vcpus */ - for (slavevcpu = mastervcpu + 1; slavevcpu < mastervcpu + hotplugvcpus[i].vcpus; slavevcpu++) { - vcpus[slavevcpu].online = vcpus[mastervcpu].online; - vcpus[slavevcpu].hotpluggable = vcpus[mastervcpu].hotpluggable; + vcpus[mainvcpu].online = !!hotplugvcpus[i].qom_path; + vcpus[mainvcpu].hotpluggable = !!hotplugvcpus[i].alias || + !vcpus[mainvcpu].online; + vcpus[mainvcpu].socket_id = hotplugvcpus[i].socket_id; + vcpus[mainvcpu].die_id = hotplugvcpus[i].die_id; + vcpus[mainvcpu].core_id = hotplugvcpus[i].core_id; + vcpus[mainvcpu].thread_id = hotplugvcpus[i].thread_id; + vcpus[mainvcpu].node_id = hotplugvcpus[i].node_id; + vcpus[mainvcpu].vcpus = hotplugvcpus[i].vcpus; + vcpus[mainvcpu].qom_path = g_steal_pointer(&hotplugvcpus[i].qom_path); + vcpus[mainvcpu].alias = g_steal_pointer(&hotplugvcpus[i].alias); + vcpus[mainvcpu].type = g_steal_pointer(&hotplugvcpus[i].type); + vcpus[mainvcpu].props = g_steal_pointer(&hotplugvcpus[i].props); + vcpus[mainvcpu].id = hotplugvcpus[i].enable_id; + + /* copy state information to sub vcpus */ + for (subvcpu = mainvcpu + 1; subvcpu < mainvcpu + hotplugvcpus[i].vcpus; subvcpu++) { + vcpus[subvcpu].online = vcpus[mainvcpu].online; + vcpus[subvcpu].hotpluggable = vcpus[mainvcpu].hotpluggable; } /* calculate next master vcpu (hotpluggable unit) entry */ - mastervcpu += hotplugvcpus[i].vcpus; + mainvcpu += hotplugvcpus[i].vcpus; } /* match entries from query cpus to the output array taking into account -- 2.24.1

On Fri, Jun 19, 2020 at 10:32:54 +0100, Daniel Berrange wrote:
When querying QEMU we have to iterate over two nested sets of CPUs. The terms "main vcpu" and "sub vcpu" are a good representation.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/qemu/qemu_monitor.c | 46 ++++++++++++++++++++--------------------- 1 file changed, 23 insertions(+), 23 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>

The two sides of a PTY can be referred to as primary and secondary TTYs. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/lxc/lxc_controller.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 2e865e0e7c..59b44dde1f 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -2054,25 +2054,25 @@ static int lxcSetPersonality(virDomainDefPtr def) } /* Create a private tty using the private devpts at PTMX, returning - * the master in *TTYMASTER and the name of the slave, _from the + * the primary in @ttyprimary and the name of the secondary, _from the * perspective of the guest after remounting file systems_, in - * *TTYNAME. Heavily borrowed from glibc, but doesn't require that + * @ttyName. Heavily borrowed from glibc, but doesn't require that * devpts == "/dev/pts" */ static int -lxcCreateTty(virLXCControllerPtr ctrl, int *ttymaster, +lxcCreateTty(virLXCControllerPtr ctrl, int *ttyprimary, char **ttyName, char **ttyHostPath) { int ret = -1; int ptyno; int unlock = 0; - if ((*ttymaster = open(ctrl->devptmx, O_RDWR|O_NOCTTY|O_NONBLOCK)) < 0) + if ((*ttyprimary = open(ctrl->devptmx, O_RDWR|O_NOCTTY|O_NONBLOCK)) < 0) goto cleanup; - if (ioctl(*ttymaster, TIOCSPTLCK, &unlock) < 0) + if (ioctl(*ttyprimary, TIOCSPTLCK, &unlock) < 0) goto cleanup; - if (ioctl(*ttymaster, TIOCGPTN, &ptyno) < 0) + if (ioctl(*ttyprimary, TIOCGPTN, &ptyno) < 0) goto cleanup; /* If mount() succeeded at honoring newinstance, then the kernel @@ -2088,7 +2088,7 @@ lxcCreateTty(virLXCControllerPtr ctrl, int *ttymaster, cleanup: if (ret != 0) { - VIR_FORCE_CLOSE(*ttymaster); + VIR_FORCE_CLOSE(*ttyprimary); g_free(*ttyName); *ttyName = NULL; } -- 2.24.1

On Fri, Jun 19, 2020 at 10:32:55 +0100, Daniel Berrange wrote:
The two sides of a PTY can be referred to as primary and secondary TTYs.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/lxc/lxc_controller.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>

The wiki page we currently link to is just a redirect for back compat. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- docs/apps.html.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/apps.html.in b/docs/apps.html.in index 629d740c28..c24717faa2 100644 --- a/docs/apps.html.in +++ b/docs/apps.html.in @@ -138,7 +138,7 @@ </dl> <dl> - <dt><a href="https://wiki.jenkins-ci.org/display/JENKINS/Libvirt+Slaves+Plugin">Jenkins</a></dt> + <dt><a href="https://plugins.jenkins.io/libvirt-slave/">Jenkins</a></dt> <dd> This plugin for Jenkins adds a way to control guest domains hosted on Xen or QEMU/KVM. You configure a Jenkins Slave, -- 2.24.1

On Fri, Jun 19, 2020 at 10:32:56 +0100, Daniel Berrange wrote:
The wiki page we currently link to is just a redirect for back compat.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- docs/apps.html.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>

Jenkins replaced use of the term 'slave' with 'agent' when describing its architecture. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- docs/apps.html.in | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/apps.html.in b/docs/apps.html.in index c24717faa2..c6d9a24fa7 100644 --- a/docs/apps.html.in +++ b/docs/apps.html.in @@ -141,9 +141,9 @@ <dt><a href="https://plugins.jenkins.io/libvirt-slave/">Jenkins</a></dt> <dd> This plugin for Jenkins adds a way to control guest domains hosted - on Xen or QEMU/KVM. You configure a Jenkins Slave, + on Xen or QEMU/KVM. You configure a Jenkins Agent, selecting the guest domain and hypervisor. When you need to build a - job on a specific Slave, its guest domain is started, then the job is + job on a specific Agent, its guest domain is started, then the job is run. When the build process is finished, the guest domain is shut down, ready to be used again as required. </dd> -- 2.24.1

On Fri, Jun 19, 2020 at 10:32:57 +0100, Daniel Berrange wrote:
Jenkins replaced use of the term 'slave' with 'agent' when describing its architecture.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- docs/apps.html.in | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>

We can't change the attribute names in the public XML schema without breaking back compat, so the best we can do is replace the terms in our internal structs. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/bhyve/bhyve_driver.c | 2 +- src/bhyve/bhyve_parse_command.c | 20 ++++++++++---------- src/conf/domain_conf.c | 24 ++++++++++++------------ src/conf/domain_conf.h | 4 ++-- src/util/virnetdevtap.c | 2 +- src/vbox/vbox_common.c | 4 ++-- 6 files changed, 28 insertions(+), 28 deletions(-) diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c index b6204c7fb9..5a8be9fd0f 100644 --- a/src/bhyve/bhyve_driver.c +++ b/src/bhyve/bhyve_driver.c @@ -1072,7 +1072,7 @@ bhyveDomainOpenConsole(virDomainPtr dom, chr = vm->def->serials[0]; - if (virFDStreamOpenPTY(st, chr->source->data.nmdm.slave, + if (virFDStreamOpenPTY(st, chr->source->data.nmdm.secondary, 0, 0, O_RDWR) < 0) goto cleanup; diff --git a/src/bhyve/bhyve_parse_command.c b/src/bhyve/bhyve_parse_command.c index 76423730d9..3daf2572cb 100644 --- a/src/bhyve/bhyve_parse_command.c +++ b/src/bhyve/bhyve_parse_command.c @@ -290,8 +290,8 @@ bhyveParseBhyveLPCArg(virDomainDefPtr def, goto error; chr->source->type = VIR_DOMAIN_CHR_TYPE_NMDM; - chr->source->data.nmdm.master = NULL; - chr->source->data.nmdm.slave = NULL; + chr->source->data.nmdm.primary = NULL; + chr->source->data.nmdm.secondary = NULL; chr->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL; if (!STRPREFIX(param, "/dev/nmdm")) { @@ -301,24 +301,24 @@ bhyveParseBhyveLPCArg(virDomainDefPtr def, goto error; } - chr->source->data.nmdm.master = g_strdup(param); - chr->source->data.nmdm.slave = g_strdup(chr->source->data.file.path); + chr->source->data.nmdm.primary = g_strdup(param); + chr->source->data.nmdm.secondary = g_strdup(chr->source->data.file.path); - /* If the last character of the master is 'A', the slave will be 'B' + /* If the last character of the primary is 'A', the secondary will be 'B' * and vice versa */ - last = strlen(chr->source->data.nmdm.master) - 1; + last = strlen(chr->source->data.nmdm.primary) - 1; switch (chr->source->data.file.path[last]) { case 'A': - chr->source->data.nmdm.slave[last] = 'B'; + chr->source->data.nmdm.secondary[last] = 'B'; break; case 'B': - chr->source->data.nmdm.slave[last] = 'A'; + chr->source->data.nmdm.secondary[last] = 'A'; break; default: virReportError(VIR_ERR_OPERATION_FAILED, - _("Failed to set slave for %s: last letter not " + _("Failed to set secondary for %s: last letter not " "'A' or 'B'"), - NULLSTR(chr->source->data.nmdm.master)); + NULLSTR(chr->source->data.nmdm.primary)); goto error; } diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e9336fd72d..2e272c44ea 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2596,8 +2596,8 @@ virDomainChrSourceDefClear(virDomainChrSourceDefPtr def) break; case VIR_DOMAIN_CHR_TYPE_NMDM: - VIR_FREE(def->data.nmdm.master); - VIR_FREE(def->data.nmdm.slave); + VIR_FREE(def->data.nmdm.primary); + VIR_FREE(def->data.nmdm.secondary); break; case VIR_DOMAIN_CHR_TYPE_UDP: @@ -2671,8 +2671,8 @@ virDomainChrSourceDefCopy(virDomainChrSourceDefPtr dest, break; case VIR_DOMAIN_CHR_TYPE_NMDM: - dest->data.nmdm.master = g_strdup(src->data.nmdm.master); - dest->data.nmdm.slave = g_strdup(src->data.nmdm.slave); + dest->data.nmdm.primary = g_strdup(src->data.nmdm.primary); + dest->data.nmdm.secondary = g_strdup(src->data.nmdm.secondary); break; } @@ -2724,8 +2724,8 @@ virDomainChrSourceDefIsEqual(const virDomainChrSourceDef *src, return STREQ_NULLABLE(src->data.file.path, tgt->data.file.path); break; case VIR_DOMAIN_CHR_TYPE_NMDM: - return STREQ_NULLABLE(src->data.nmdm.master, tgt->data.nmdm.master) && - STREQ_NULLABLE(src->data.nmdm.slave, tgt->data.nmdm.slave); + return STREQ_NULLABLE(src->data.nmdm.primary, tgt->data.nmdm.primary) && + STREQ_NULLABLE(src->data.nmdm.secondary, tgt->data.nmdm.secondary); break; case VIR_DOMAIN_CHR_TYPE_UDP: return STREQ_NULLABLE(src->data.udp.bindHost, tgt->data.udp.bindHost) && @@ -6220,13 +6220,13 @@ virDomainChrSourceDefValidate(const virDomainChrSourceDef *src_def, break; case VIR_DOMAIN_CHR_TYPE_NMDM: - if (!src_def->data.nmdm.master) { + if (!src_def->data.nmdm.primary) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Missing master path attribute for nmdm device")); return -1; } - if (!src_def->data.nmdm.slave) { + if (!src_def->data.nmdm.secondary) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Missing slave path attribute for nmdm device")); return -1; @@ -13253,8 +13253,8 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, break; case VIR_DOMAIN_CHR_TYPE_NMDM: - def->data.nmdm.master = virXMLPropString(cur, "master"); - def->data.nmdm.slave = virXMLPropString(cur, "slave"); + def->data.nmdm.primary = virXMLPropString(cur, "master"); + def->data.nmdm.secondary = virXMLPropString(cur, "slave"); break; case VIR_DOMAIN_CHR_TYPE_LAST: @@ -26853,8 +26853,8 @@ virDomainChrSourceDefFormat(virBufferPtr buf, case VIR_DOMAIN_CHR_TYPE_NMDM: virBufferEscapeString(buf, "<source master='%s' ", - def->data.nmdm.master); - virBufferEscapeString(buf, "slave='%s'/>\n", def->data.nmdm.slave); + def->data.nmdm.primary); + virBufferEscapeString(buf, "slave='%s'/>\n", def->data.nmdm.secondary); break; case VIR_DOMAIN_CHR_TYPE_UDP: diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 41715c75f2..00a0e02cd1 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1201,8 +1201,8 @@ struct _virDomainChrSourceDef { int append; /* enum virTristateSwitch */ } file; /* pty, file, pipe, or device */ struct { - char *master; - char *slave; + char *primary; + char *secondary; } nmdm; struct { char *host; diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c index 7bd30ea0f9..6272e03f42 100644 --- a/src/util/virnetdevtap.c +++ b/src/util/virnetdevtap.c @@ -687,7 +687,7 @@ int virNetDevTapCreateInBridgePort(const char *brname, /* We need to set the interface MAC before adding it * to the bridge, because the bridge assumes the lowest - * MAC of all enslaved interfaces & we don't want it + * MAC of all member interfaces & we don't want it * seeing the kernel allocate random MAC for the TAP * device before we set our static MAC. */ diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 15f8eb074a..493639d6d9 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -3344,8 +3344,8 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) switch ((enum StorageBus) storageBus) { case StorageBus_IDE: disk->bus = VIR_DOMAIN_DISK_BUS_IDE; - disk->info.addr.drive.bus = devicePort; /* primary, secondary */ - disk->info.addr.drive.unit = deviceSlot; /* master, slave */ + disk->info.addr.drive.bus = devicePort; + disk->info.addr.drive.unit = deviceSlot; break; case StorageBus_SATA: -- 2.24.1

On Fri, Jun 19, 2020 at 10:32:58 +0100, Daniel Berrange wrote:
We can't change the attribute names in the public XML schema without breaking back compat, so the best we can do is replace the terms in our internal structs.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/bhyve/bhyve_driver.c | 2 +- src/bhyve/bhyve_parse_command.c | 20 ++++++++++---------- src/conf/domain_conf.c | 24 ++++++++++++------------ src/conf/domain_conf.h | 4 ++-- src/util/virnetdevtap.c | 2 +- src/vbox/vbox_common.c | 4 ++-- 6 files changed, 28 insertions(+), 28 deletions(-)
[...]
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 41715c75f2..00a0e02cd1 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1201,8 +1201,8 @@ struct _virDomainChrSourceDef { int append; /* enum virTristateSwitch */ } file; /* pty, file, pipe, or device */ struct { - char *master; - char *slave; + char *primary; + char *secondary; } nmdm;
I'm not persuaded that it's worth diverging the internal names from the XML names since we have to stick with those. NACK

Refer to the notion of mount propagation instead which describes the actual behaviour more clearly. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/lxc/lxc_controller.c | 6 +++--- src/util/virprocess.c | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 59b44dde1f..89f9773b2c 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -2116,9 +2116,9 @@ virLXCControllerSetupPrivateNS(void) * * Thus we call unshare(CLONE_NS) so that we can see * the guest's new /dev/pts, without it becoming - * visible to the host OS. We also put the root FS - * into slave mode, just in case it was currently - * marked as shared + * visible to the host OS. We also disable mount + * propagation out of the root FS, in case it was + * currently allowing bi-directional propagation. */ return virProcessSetupPrivateMountNS(); diff --git a/src/util/virprocess.c b/src/util/virprocess.c index afb1f9b79f..a9afa2e665 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -1288,7 +1288,7 @@ virProcessSetupPrivateMountNS(void) if (mount("", "/", "none", MS_SLAVE|MS_REC, NULL) < 0) { virReportSystemError(errno, "%s", - _("Failed to switch root mount into slave mode")); + _("Failed disable mount propagation out of the root filesystem")); return -1; } -- 2.24.1

On Fri, Jun 19, 2020 at 10:32:59 +0100, Daniel Berrange wrote:
Refer to the notion of mount propagation instead which describes the actual behaviour more clearly.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/lxc/lxc_controller.c | 6 +++--- src/util/virprocess.c | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>

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

On Fri, Jun 19, 2020 at 10:33:00 +0100, Daniel Berrange 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@redhat.com> --- build-aux/syntax-check.mk | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
I don't think there's a technical reason forbiding these and it's almost borderline censorship. I refuse to put my R-b on this one.

On Fri, Jun 19, 2020 at 12:47:07PM +0200, Peter Krempa wrote:
On Fri, Jun 19, 2020 at 10:33:00 +0100, Daniel Berrange 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@redhat.com> --- build-aux/syntax-check.mk | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
I don't think there's a technical reason forbiding these and it's almost borderline censorship. I refuse to put my R-b on this one.
Few of the syntax check rules are technical in nature. They're largely about enforcing our desired coding/style policies. Preventing reintroduction of terminology we've just eliminated is totally in scope for what syntax check does. The goal is to automate checks that humans are otherwise bad at doing, because we'll easily miss this kind of thing in manual reviews. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

On a Friday in 2020, Peter Krempa wrote:
On Fri, Jun 19, 2020 at 10:33:00 +0100, Daniel Berrange wrote:
We don't check for "master", because there are too many cases that we're not trying to eliminate at this time.
Even if you consider the terms undesirable, consider using them in the commit summary instead of mentioning what you're not trying to do.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- build-aux/syntax-check.mk | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
I don't think there's a technical reason forbiding these and it's almost borderline censorship. I refuse to put my R-b on this one.
Given how many files are excepted, I think it's a waste of electricty to even check for these. Jano

On Fri, Jun 19, 2020 at 02:11:09PM +0200, Ján Tomko wrote:
On a Friday in 2020, Peter Krempa wrote:
On Fri, Jun 19, 2020 at 10:33:00 +0100, Daniel Berrange wrote:
We don't check for "master", because there are too many cases that we're not trying to eliminate at this time.
Even if you consider the terms undesirable, consider using them in the commit summary instead of mentioning what you're not trying to do.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- build-aux/syntax-check.mk | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
I don't think there's a technical reason forbiding these and it's almost borderline censorship. I refuse to put my R-b on this one.
Given how many files are excepted, I think it's a waste of electricty to even check for these.
We have > 10,000 files in source control, of which only 150 are exempted and time required to check that won't even register in the noise. We shouldn't be relying on reviewers to check things that can trivially be automated, when we know reviewers often miss things. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

On Fri, 2020-06-19 at 13:33 +0100, Daniel P. Berrangé wrote:
On Fri, Jun 19, 2020 at 02:11:09PM +0200, Ján Tomko wrote:
On a Friday in 2020, Peter Krempa wrote:
On Fri, Jun 19, 2020 at 10:33:00 +0100, Daniel Berrange wrote:
We don't check for "master", because there are too many cases that we're not trying to eliminate at this time.
Even if you consider the terms undesirable, consider using them in the commit summary instead of mentioning what you're not trying to do.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- build-aux/syntax-check.mk | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
I don't think there's a technical reason forbiding these and it's almost borderline censorship. I refuse to put my R-b on this one.
Given how many files are excepted, I think it's a waste of electricty to even check for these.
We have > 10,000 files in source control, of which only 150 are exempted and time required to check that won't even register in the noise. We shouldn't be relying on reviewers to check things that can trivially be automated, when we know reviewers often miss things.
Regards, Daniel
Personally, I would be glad to have syntax-check catch a patch of mine if I accidentally use one of these terms that we've decided are undesirable. If it is necessary to use one of these terms in a patch, and I can convince others that it is necessary, I can always submit a patch adding an exception. But it forces me to think about the terminology a bit more carefully, which seems like a reasonable thing. I see no censorship concerns. Jonathon

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

On Fri, Jun 19, 2020 at 08:45:41AM -0300, Daniel Henrique Barboza wrote:
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 */
Yes, that is entirely the intention because there's no need to add such a comment in the first place. It can easily be written as /* This is code is needed because module X is prohibited */
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.
The only allowed usage of the terms is that which already exists in the code which are unable to be eliminated due to interactions with external tools/API. No new instances should be introduced, which is what the syntax-check rule validates. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
participants (5)
-
Daniel Henrique Barboza
-
Daniel P. Berrangé
-
Jonathon Jongsma
-
Ján Tomko
-
Peter Krempa