[PATCH 00/20] Finish the work to stop hardcoding paths at build time

Over the years we've made various changes to stop hardcoding paths at build time, but the work is incomplete. This bit us significantly in Fedora 42 where they have merged the 'sbin' and 'bin' dirs together. In the build environment we have a clean install where '/sbin' and '/usr/sbin' are simply symlinks to '/usr/bin'. Our meson.build logic preferentially looks in '/sbin' and '/usr/sbin' to find programs, and thus it finds *all* binaries in '/sbin' and then hardcodes this path. This happens even for binaries that have always been in '/usr/bin' and NEVER '/sbin' or '/usr/sbin' On fresh Fedora installs this is fine as the symlinks match the build environment. On upgraded Fedora installs, however, '/sbin' and '/usr/sbin' remain separate directories and inside they symlink individual binaries, if-and-only-if the binary was in '/sbin' in Fedora 41. Thus many of the binaries we found in /sbin don't actually exist and libvirt thus breaks. There is really no reason why we should be harcoding any paths at build time. virCommand happily uses virFindFileInPath for any paths which are not already absolute. $PATH includes '/sbin' and '/usr/sbin' when we're running privileged, and when unprivileged we shouldn't need to run binaries from there. Daniel P. Berrangé (20): storage: stop hardcoding paths for mkfs, mount, umount tests: storage hardcoding paths for mount & vgchange util: stop hardcoding numad path util: stop hardcoding bhyve, bhyvectl, bhyveload paths util: stop hardcoding 'ifconfig' path docs: stop setting vars for docs tools build-aux: add missing definition of PERL variable meson: stop setting conf var for required programs util: remove use hardcoded DMIDECODE path meson: remove check for 'ip' program util: remove hardcoded ISCSIADM command path nodedev: remove use hardcoded MDEVCTL path util: remove use hardcoded MM_CTL path util: remove use hardcoded MODPROBE/RMMOD paths util: remove use hardcoded OVS_VSCTL path util: remove use hardcoded TC path meson: stop setting conf var for optional programs storage: stop hardcoding LVM tool paths util: stop hardcoding pkttyagent path openvz: stop hardcoding vzlist/vzctl/vzmigrate paths build-aux/Makefile.in | 1 + build-aux/meson.build | 1 + docs/meson.build | 1 - meson.build | 92 +++------------------ src/bhyve/bhyve_command.c | 6 +- src/locking/lock_driver_lockd.c | 4 +- src/node_device/node_device_driver.c | 14 ++-- src/openvz/openvz_conf.h | 6 +- src/storage/storage_backend_fs.c | 17 +--- src/storage/storage_backend_logical.c | 24 +++--- src/util/virfirewall.h | 1 + src/util/viriscsi.c | 55 +++++++------ src/util/virkmod.c | 4 +- src/util/virnetdevbandwidth.c | 36 ++++----- src/util/virnetdevip.c | 14 ++-- src/util/virnetdevmidonet.c | 4 +- src/util/virnetdevopenvswitch.c | 4 +- src/util/virnuma.c | 2 +- src/util/virpolkit.c | 8 +- src/util/virpolkit.h | 2 - src/util/virsysinfo.c | 4 +- tests/storagepoolxml2argvtest.c | 12 +-- tests/viriscsitest.c | 16 ++-- tests/virkmodtest.c | 4 +- tests/virnetdevbandwidthtest.c | 110 +++++++++++++------------- tests/virnetdevopenvswitchtest.c | 74 ++++++++--------- 26 files changed, 216 insertions(+), 300 deletions(-) -- 2.49.0

From: Daniel P. Berrangé <berrange@redhat.com> This was always undesirable but now causes problems on Fedora 42 where at build time we detect a /sbin path but at runtime this will only exist on upgraded machines, not fresh installs. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- meson.build | 13 ------------- src/storage/storage_backend_fs.c | 17 +++-------------- 2 files changed, 3 insertions(+), 27 deletions(-) diff --git a/meson.build b/meson.build index 37b1caa566..14c98b49a1 100644 --- a/meson.build +++ b/meson.build @@ -1827,23 +1827,10 @@ if conf.has('WITH_LIBVIRTD') endif endif - if fs_enable - mount_prog = find_program('mount', required: get_option('storage_fs'), dirs: libvirt_sbin_path) - umount_prog = find_program('umount', required: get_option('storage_fs'), dirs: libvirt_sbin_path) - mkfs_prog = find_program('mkfs', required: get_option('storage_fs'), dirs: libvirt_sbin_path) - - if not mount_prog.found() or not umount_prog.found() or not mkfs_prog.found() - fs_enable = false - endif - endif - if fs_enable use_storage = true conf.set('WITH_STORAGE_FS', 1) - conf.set_quoted('MOUNT', mount_prog.full_path()) - conf.set_quoted('UMOUNT', umount_prog.full_path()) - conf.set_quoted('MKFS', mkfs_prog.full_path()) endif endif diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index fce395d60f..6ec359625a 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -304,7 +304,7 @@ virStorageBackendFileSystemMount(virStoragePoolObj *pool) if (!(src = virStorageBackendFileSystemGetPoolSource(pool))) return -1; - cmd = virStorageBackendFileSystemMountCmd(MOUNT, def, src); + cmd = virStorageBackendFileSystemMountCmd("mount", def, src); /* Mounting a shared FS might take a long time. Don't hold * the pool locked meanwhile. */ @@ -362,7 +362,7 @@ virStorageBackendFileSystemStop(virStoragePoolObj *pool) if ((rc = virStorageBackendFileSystemIsMounted(pool)) != 1) return rc; - cmd = virCommandNewArgList(UMOUNT, def->target.path, NULL); + cmd = virCommandNewArgList("umount", def->target.path, NULL); return virCommandRun(cmd, NULL); } #endif /* WITH_STORAGE_FS */ @@ -402,18 +402,7 @@ virStorageBackendExecuteMKFS(const char *device, g_autoptr(virCommand) cmd = NULL; g_autofree char *mkfs = NULL; -#if WITH_STORAGE_FS - mkfs = virFindFileInPath(MKFS); -#endif /* WITH_STORAGE_FS */ - - if (!mkfs) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("mkfs is not available on this platform: Failed to make filesystem of type '%1$s' on device '%2$s'"), - format, device); - return -1; - } - - cmd = virCommandNewArgList(mkfs, "-t", format, NULL); + cmd = virCommandNewArgList("mkfs", "-t", format, NULL); /* use the force, otherwise mkfs.xfs won't overwrite existing fs. * Similarly mkfs.ext2, mkfs.ext3, and mkfs.ext4 require supplying -F -- 2.49.0

On Tue, Apr 29, 2025 at 12:14:01 +0100, Daniel P. Berrangé via Devel wrote:
From: Daniel P. Berrangé <berrange@redhat.com>
This was always undesirable but now causes problems on Fedora 42 where at build time we detect a /sbin path but at runtime this will only exist on upgraded machines, not fresh installs.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- meson.build | 13 ------------- src/storage/storage_backend_fs.c | 17 +++-------------- 2 files changed, 3 insertions(+), 27 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>

From: Daniel P. Berrangé <berrange@redhat.com> This is redundant since the tests will strip any path component from the binary name before comparison. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- tests/storagepoolxml2argvtest.c | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/tests/storagepoolxml2argvtest.c b/tests/storagepoolxml2argvtest.c index e8e40d695e..d5c2531ab8 100644 --- a/tests/storagepoolxml2argvtest.c +++ b/tests/storagepoolxml2argvtest.c @@ -6,14 +6,6 @@ #define VIR_FROM_THIS VIR_FROM_NONE -#ifndef MOUNT -# define MOUNT "/usr/bin/mount" -#endif - -#ifndef VGCHANGE -# define VGCHANGE "/usr/sbin/vgchange" -#endif - static int testCompareXMLToArgvFiles(bool shouldFail, const char *poolxml, @@ -46,12 +38,12 @@ testCompareXMLToArgvFiles(bool shouldFail, goto cleanup; } - cmd = virStorageBackendFileSystemMountCmd(MOUNT, def, src); + cmd = virStorageBackendFileSystemMountCmd("mount", def, src); def = NULL; break; case VIR_STORAGE_POOL_LOGICAL: - cmd = virStorageBackendLogicalChangeCmd(VGCHANGE, def, true); + cmd = virStorageBackendLogicalChangeCmd("vgchange", def, true); break; case VIR_STORAGE_POOL_DIR: -- 2.49.0

On Tue, Apr 29, 2025 at 12:14:02 +0100, Daniel P. Berrangé via Devel wrote:
From: Daniel P. Berrangé <berrange@redhat.com>
This is redundant since the tests will strip any path component from the binary name before comparison.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- tests/storagepoolxml2argvtest.c | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>

From: Daniel P. Berrangé <berrange@redhat.com> Change the meson rules to always enable numad if on a Linux host, unless the meson options say not to. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- meson.build | 10 +++------- src/util/virnuma.c | 2 +- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/meson.build b/meson.build index 14c98b49a1..767205f44b 100644 --- a/meson.build +++ b/meson.build @@ -2028,14 +2028,10 @@ if not get_option('nss').disabled() endif endif -if not get_option('numad').disabled() and numactl_dep.found() - numad_prog = find_program('numad', required: get_option('numad'), dirs: libvirt_sbin_path) - if numad_prog.found() - conf.set('WITH_NUMAD', 1) - conf.set_quoted('NUMAD', numad_prog.full_path()) - endif +if not get_option('numad').disabled() and numactl_dep.found() and host_machine.system() == 'linux' + conf.set('WITH_NUMAD', 1) elif get_option('numad').enabled() - error('You must have numactl enabled for numad support.') + error('You must have a Linux host with numactl enabled for numad support.') endif # nwfilter should only be compiled for linux, and only if the diff --git a/src/util/virnuma.c b/src/util/virnuma.c index 9393c20875..67c51630c7 100644 --- a/src/util/virnuma.c +++ b/src/util/virnuma.c @@ -61,7 +61,7 @@ virNumaGetAutoPlacementAdvice(unsigned short vcpus, g_autoptr(virCommand) cmd = NULL; char *output = NULL; - cmd = virCommandNewArgList(NUMAD, "-w", NULL); + cmd = virCommandNewArgList("numad", "-w", NULL); virCommandAddArgFormat(cmd, "%d:%llu", vcpus, VIR_DIV_UP(balloon, 1024)); -- 2.49.0

On Tue, Apr 29, 2025 at 12:14:03 +0100, Daniel P. Berrangé via Devel wrote:
From: Daniel P. Berrangé <berrange@redhat.com>
Change the meson rules to always enable numad if on a Linux host, unless the meson options say not to.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- meson.build | 10 +++------- src/util/virnuma.c | 2 +- 2 files changed, 4 insertions(+), 8 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>

From: Daniel P. Berrangé <berrange@redhat.com> Change the meson rules to always enable numad if on a FreeBSD host, unless the meson options say not to. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- meson.build | 11 +---------- src/bhyve/bhyve_command.c | 6 +++--- 2 files changed, 4 insertions(+), 13 deletions(-) diff --git a/meson.build b/meson.build index 767205f44b..b0b6efee8e 100644 --- a/meson.build +++ b/meson.build @@ -1495,16 +1495,7 @@ if not get_option('driver_libvirtd').disabled() endif if not get_option('driver_bhyve').disabled() and host_machine.system() == 'freebsd' - bhyve_prog = find_program('bhyve', required: get_option('driver_bhyve')) - bhyvectl_prog = find_program('bhyvectl', required: get_option('driver_bhyve')) - bhyveload_prog = find_program('bhyveload', required: get_option('driver_bhyve')) - - if bhyve_prog.found() and bhyvectl_prog.found() and bhyveload_prog.found() - conf.set('WITH_BHYVE', 1) - conf.set_quoted('BHYVE', bhyve_prog.full_path()) - conf.set_quoted('BHYVECTL', bhyvectl_prog.full_path()) - conf.set_quoted('BHYVELOAD', bhyveload_prog.full_path()) - endif + conf.set('WITH_BHYVE', 1) elif get_option('driver_bhyve').enabled() error('The bhyve driver cannot be enabled') endif diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c index 6578a4326c..44c66ea147 100644 --- a/src/bhyve/bhyve_command.c +++ b/src/bhyve/bhyve_command.c @@ -711,7 +711,7 @@ virBhyveProcessBuildBhyveCmd(struct _bhyveConn *driver, virDomainDef *def, * -S 31,uart,stdio \ * vm0 */ - g_autoptr(virCommand) cmd = virCommandNew(BHYVE); + g_autoptr(virCommand) cmd = virCommandNew("bhyve"); size_t i; unsigned nusbcontrollers = 0; unsigned nisacontrollers = 0; @@ -884,7 +884,7 @@ virCommand * virBhyveProcessBuildDestroyCmd(struct _bhyveConn *driver G_GNUC_UNUSED, virDomainDef *def) { - virCommand *cmd = virCommandNew(BHYVECTL); + virCommand *cmd = virCommandNew("bhyvectl"); virCommandAddArg(cmd, "--destroy"); virCommandAddArgPair(cmd, "--vm", def->name); @@ -907,7 +907,7 @@ virBhyveProcessBuildBhyveloadCmd(virDomainDef *def, virDomainDiskDef *disk) { virCommand *cmd; - cmd = virCommandNew(BHYVELOAD); + cmd = virCommandNew("bhyveload"); if (def->os.bootloaderArgs == NULL) { VIR_DEBUG("bhyveload with default arguments"); -- 2.49.0

On Tue, Apr 29, 2025 at 12:14:04 +0100, Daniel P. Berrangé via Devel wrote:
From: Daniel P. Berrangé <berrange@redhat.com>
Change the meson rules to always enable numad if on a FreeBSD host, unless the meson options say not to.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- meson.build | 11 +---------- src/bhyve/bhyve_command.c | 6 +++--- 2 files changed, 4 insertions(+), 13 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>

From: Daniel P. Berrangé <berrange@redhat.com> Change the source to assume use of 'ifconfig' on FreeBSD builds, allowing virCommand to report missing commands at runtime. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- meson.build | 4 ---- src/util/virnetdevip.c | 8 ++++---- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/meson.build b/meson.build index b0b6efee8e..ea3e752f93 100644 --- a/meson.build +++ b/meson.build @@ -837,10 +837,6 @@ required_programs = [ 'xsltproc', ] -if host_machine.system() == 'freebsd' - required_programs += 'ifconfig' -endif - foreach name : required_programs prog = find_program(name, dirs: libvirt_sbin_path) varname = name.underscorify() diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c index f365c1999c..f7a7db620d 100644 --- a/src/util/virnetdevip.c +++ b/src/util/virnetdevip.c @@ -390,8 +390,8 @@ virNetDevIPAddrAdd(const char *ifname, return -1; } -# ifdef IFCONFIG - cmd = virCommandNew(IFCONFIG); +# ifdef __FreeBSD__ + cmd = virCommandNew("ifconfig"); virCommandAddArg(cmd, ifname); if (VIR_SOCKET_ADDR_IS_FAMILY(addr, AF_INET6)) virCommandAddArg(cmd, "inet6"); @@ -431,8 +431,8 @@ virNetDevIPAddrDel(const char *ifname, if (!(addrstr = virSocketAddrFormat(addr))) return -1; -# ifdef IFCONFIG - cmd = virCommandNew(IFCONFIG); +# ifdef __FreeBSD__ + cmd = virCommandNew("ifconfig"); virCommandAddArg(cmd, ifname); if (VIR_SOCKET_ADDR_IS_FAMILY(addr, AF_INET6)) virCommandAddArg(cmd, "inet6"); -- 2.49.0

On Tue, Apr 29, 2025 at 12:14:05 +0100, Daniel P. Berrangé via Devel wrote:
From: Daniel P. Berrangé <berrange@redhat.com>
Change the source to assume use of 'ifconfig' on FreeBSD builds, allowing virCommand to report missing commands at runtime.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- meson.build | 4 ---- src/util/virnetdevip.c | 8 ++++---- 2 files changed, 4 insertions(+), 8 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>

From: Daniel P. Berrangé <berrange@redhat.com> Nothing in the tree references RST2MAN or RST2HTML5 variables, only rst2man_prog & rst2html5_prog, so the former can be removed. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- docs/meson.build | 1 - 1 file changed, 1 deletion(-) diff --git a/docs/meson.build b/docs/meson.build index d7343b6665..4f8982ee90 100644 --- a/docs/meson.build +++ b/docs/meson.build @@ -176,7 +176,6 @@ docs_programs_groups = [ foreach item : docs_programs_groups prog = find_program(item.get('prog'), dirs: libvirt_sbin_path) varname = item.get('name').underscorify() - conf.set_quoted(varname.to_upper(), prog.full_path()) set_variable('@0@_prog'.format(varname), prog) endforeach -- 2.49.0

On Tue, Apr 29, 2025 at 12:14:06 +0100, Daniel P. Berrangé via Devel wrote:
From: Daniel P. Berrangé <berrange@redhat.com>
Nothing in the tree references RST2MAN or RST2HTML5 variables, only rst2man_prog & rst2html5_prog, so the former can be removed.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- docs/meson.build | 1 - 1 file changed, 1 deletion(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>

From: Daniel P. Berrangé <berrange@redhat.com> Currently $(PERL) is expanding to nothing, but the spacing-check.pl test luckily still works via the shebang. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- build-aux/Makefile.in | 1 + build-aux/meson.build | 1 + 2 files changed, 2 insertions(+) diff --git a/build-aux/Makefile.in b/build-aux/Makefile.in index db44ee36a4..f30773e263 100644 --- a/build-aux/Makefile.in +++ b/build-aux/Makefile.in @@ -6,6 +6,7 @@ FLAKE8 = @flake8_path@ BLACK = @black_path@ RUNUTF8 = @runutf8@ PYTHON = @PYTHON3@ +PERL = @PERL@ GREP = @GREP@ SED = @SED@ AWK = @AWK@ diff --git a/build-aux/meson.build b/build-aux/meson.build index 06be6ab6c8..bcd10e89f2 100644 --- a/build-aux/meson.build +++ b/build-aux/meson.build @@ -42,6 +42,7 @@ if git and tests_enabled[0] 'black_path': black_path, 'runutf8': ' '.join(runutf8), 'PYTHON3': python3_prog.full_path(), + 'PERL': perl_prog.full_path(), 'GREP': grep_prog.full_path(), 'SED': sed_prog.full_path(), 'AWK': awk_prog.full_path(), -- 2.49.0

On Tue, Apr 29, 2025 at 12:14:07 +0100, Daniel P. Berrangé via Devel wrote:
From: Daniel P. Berrangé <berrange@redhat.com>
Currently $(PERL) is expanding to nothing, but the spacing-check.pl test luckily still works via the shebang.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- build-aux/Makefile.in | 1 + build-aux/meson.build | 1 + 2 files changed, 2 insertions(+)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>

From: Daniel P. Berrangé <berrange@redhat.com> There is no reference to PERL, PYTHON3, XMLLINT or XSLTPROC conf variables anywhere, only the ${name}_prog meson variables are used. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- meson.build | 1 - 1 file changed, 1 deletion(-) diff --git a/meson.build b/meson.build index ea3e752f93..e1d183df23 100644 --- a/meson.build +++ b/meson.build @@ -840,7 +840,6 @@ required_programs = [ foreach name : required_programs prog = find_program(name, dirs: libvirt_sbin_path) varname = name.underscorify() - conf.set_quoted(varname.to_upper(), prog.full_path()) set_variable('@0@_prog'.format(varname), prog) endforeach -- 2.49.0

On Tue, Apr 29, 2025 at 12:14:08 +0100, Daniel P. Berrangé via Devel wrote:
From: Daniel P. Berrangé <berrange@redhat.com>
There is no reference to PERL, PYTHON3, XMLLINT or XSLTPROC conf variables anywhere, only the ${name}_prog meson variables are used.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- meson.build | 1 - 1 file changed, 1 deletion(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>

From: Daniel P. Berrangé <berrange@redhat.com> Allow virCommand to find 'dmidecode' in $PATH. This command is only usable when running privileged since it relies on reading from a privileged kernel file. Thus we can assume both 'bin' and 'sbin' dirs will be in $PATH and virFindFileInPath will do the right thing to find it when called by virCommand. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- meson.build | 1 - src/util/virsysinfo.c | 4 ++-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/meson.build b/meson.build index e1d183df23..06a69308ac 100644 --- a/meson.build +++ b/meson.build @@ -854,7 +854,6 @@ optional_test_programs = [ ] optional_programs = [ - 'dmidecode', 'ip', 'iscsiadm', 'mdevctl', diff --git a/src/util/virsysinfo.c b/src/util/virsysinfo.c index b7413c3bf5..0f12a8964f 100644 --- a/src/util/virsysinfo.c +++ b/src/util/virsysinfo.c @@ -894,7 +894,7 @@ virSysinfoDMIDecodeOEMString(unsigned int idx, char **str) { g_autofree char *err = NULL; - g_autoptr(virCommand) cmd = virCommandNewArgList(DMIDECODE, "--dump", + g_autoptr(virCommand) cmd = virCommandNewArgList("dmidecode", "--dump", "--oem-string", NULL); virCommandAddArgFormat(cmd, "%u", idx); virCommandSetOutputBuffer(cmd, str); @@ -1199,7 +1199,7 @@ virSysinfoReadDMI(void) g_autofree char *outbuf = NULL; g_autoptr(virCommand) cmd = NULL; - cmd = virCommandNewArgList(DMIDECODE, "-q", "-t", "0,1,2,3,4,11,17", NULL); + cmd = virCommandNewArgList("dmidecode", "-q", "-t", "0,1,2,3,4,11,17", NULL); virCommandSetOutputBuffer(cmd, &outbuf); if (virCommandRun(cmd, NULL) < 0) return NULL; -- 2.49.0

On Tue, Apr 29, 2025 at 12:14:09 +0100, Daniel P. Berrangé via Devel wrote:
From: Daniel P. Berrangé <berrange@redhat.com>
Allow virCommand to find 'dmidecode' in $PATH. This command is only usable when running privileged since it relies on reading from a privileged kernel file. Thus we can assume both 'bin' and 'sbin' dirs will be in $PATH and virFindFileInPath will do the right thing to find it when called by virCommand.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- meson.build | 1 - src/util/virsysinfo.c | 4 ++-- 2 files changed, 2 insertions(+), 3 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>

From: Daniel P. Berrangé <berrange@redhat.com> There are no references to an "IP" conf variable nor a meson 'ip_prog' variable so the check serves no purpose. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- meson.build | 1 - src/util/virnetdevip.c | 6 +++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/meson.build b/meson.build index 06a69308ac..d355767a53 100644 --- a/meson.build +++ b/meson.build @@ -854,7 +854,6 @@ optional_test_programs = [ ] optional_programs = [ - 'ip', 'iscsiadm', 'mdevctl', 'mm-ctl', diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c index f7a7db620d..d4f023ca5c 100644 --- a/src/util/virnetdevip.c +++ b/src/util/virnetdevip.c @@ -404,7 +404,7 @@ virNetDevIPAddrAdd(const char *ifname, virCommandAddArgList(cmd, "broadcast", bcaststr, NULL); virCommandAddArg(cmd, "alias"); # else - cmd = virCommandNew(IP); + cmd = virCommandNew("ip"); virCommandAddArgList(cmd, "addr", "add", NULL); virCommandAddArgFormat(cmd, "%s/%u", addrstr, prefix); if (peerstr) @@ -441,7 +441,7 @@ virNetDevIPAddrDel(const char *ifname, virCommandAddArgFormat(cmd, "%s/%u", addrstr, prefix); virCommandAddArg(cmd, "-alias"); # else - cmd = virCommandNew(IP); + cmd = virCommandNew("ip"); virCommandAddArgList(cmd, "addr", "del", NULL); virCommandAddArgFormat(cmd, "%s/%u", addrstr, prefix); virCommandAddArgList(cmd, "dev", ifname, NULL); @@ -469,7 +469,7 @@ virNetDevIPRouteAdd(const char *ifname, return -1; if (!(gatewaystr = virSocketAddrFormat(gateway))) return -1; - cmd = virCommandNew(IP); + cmd = virCommandNew("ip"); virCommandAddArgList(cmd, "route", "add", NULL); virCommandAddArgFormat(cmd, "%s/%u", addrstr, prefix); virCommandAddArgList(cmd, "via", gatewaystr, "dev", ifname, -- 2.49.0

On Tue, Apr 29, 2025 at 12:14:10 +0100, Daniel P. Berrangé via Devel wrote:
From: Daniel P. Berrangé <berrange@redhat.com>
There are no references to an "IP" conf variable nor a meson 'ip_prog' variable so the check serves no purpose.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- meson.build | 1 - src/util/virnetdevip.c | 6 +++--- 2 files changed, 3 insertions(+), 4 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>

From: Daniel P. Berrangé <berrange@redhat.com> Allow virCommand to find 'dmidecode' in $PATH. This command is only usable when running privileged since it relies on reading from a privileged kernel file. Thus we can assume both 'bin' and 'sbin' dirs will be in $PATH and virFindFileInPath will do the right thing to find it when called by virCommand. Gating the iscsi driver backend on a isciadm probe is likely to do more harm than good as it needlessly disables the code if the dev forgot to install iscsiadm at build time. As a Linux only command it is simpler to gate the feature based on the platform choice and allow missing binaries to be diagnose at runtime. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- meson.build | 5 ++-- src/util/viriscsi.c | 55 ++++++++++++++++++++++---------------------- tests/viriscsitest.c | 16 ++++++------- 3 files changed, 37 insertions(+), 39 deletions(-) diff --git a/meson.build b/meson.build index d355767a53..9759a3322b 100644 --- a/meson.build +++ b/meson.build @@ -854,7 +854,6 @@ optional_test_programs = [ ] optional_programs = [ - 'iscsiadm', 'mdevctl', 'mm-ctl', 'modprobe', @@ -1825,11 +1824,11 @@ if conf.has('WITH_LIBVIRTD') error('Need glusterfs (libgfapi) for gluster storage driver') endif - if not get_option('storage_iscsi').disabled() and iscsiadm_prog.found() + if not get_option('storage_iscsi').disabled() and host_machine.system() == 'linux' use_storage = true conf.set('WITH_STORAGE_ISCSI', 1) elif get_option('storage_iscsi').enabled() - error('We need iscsiadm for iSCSI storage driver') + error('Linux host needed for iSCSI storage driver using iscsiadm') endif if not get_option('storage_iscsi_direct').disabled() and libiscsi_dep.found() diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c index a70c87cbdb..f6b2cdba0c 100644 --- a/src/util/viriscsi.c +++ b/src/util/viriscsi.c @@ -88,8 +88,8 @@ virISCSIGetSession(const char *devpath, int exitstatus = 0; g_autofree char *error = NULL; - g_autoptr(virCommand) cmd = virCommandNewArgList(ISCSIADM, "--mode", - "session", NULL); + g_autoptr(virCommand) cmd = virCommandNewArgList("iscsiadm", "--mode", + "session", NULL); virCommandSetErrorBuffer(cmd, &error); if (virCommandRunRegex(cmd, @@ -123,8 +123,8 @@ virStorageBackendIQNFound(const char *initiatoriqn, g_autofree char *outbuf = NULL; g_autofree char *iface = NULL; g_autofree char *iqn = NULL; - g_autoptr(virCommand) cmd = virCommandNewArgList(ISCSIADM, - "--mode", "iface", NULL); + g_autoptr(virCommand) cmd = virCommandNewArgList("iscsiadm", + "--mode", "iface", NULL); *ifacename = NULL; @@ -193,8 +193,8 @@ virStorageBackendIQNFound(const char *initiatoriqn, error: virReportError(VIR_ERR_INTERNAL_ERROR, - _("malformed output of %1$s: %2$s"), - ISCSIADM, line); + _("malformed output of 'iscsiadm': %1$s"), + line); goto cleanup; } @@ -215,7 +215,7 @@ virStorageBackendCreateIfaceIQN(const char *initiatoriqn, VIR_DEBUG("Attempting to create interface '%s' with IQN '%s'", temp_ifacename, initiatoriqn); - newcmd = virCommandNewArgList(ISCSIADM, + newcmd = virCommandNewArgList("iscsiadm", "--mode", "iface", "--interface", temp_ifacename, "--op", "new", @@ -225,13 +225,12 @@ virStorageBackendCreateIfaceIQN(const char *initiatoriqn, * We will just rely on whether the interface got created * properly. */ if (virCommandRun(newcmd, &exitstatus) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to run command '%1$s' to create new iscsi interface"), - ISCSIADM); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Failed to run command 'iscsiadm' to create new iscsi interface")); return -1; } - updatecmd = virCommandNewArgList(ISCSIADM, + updatecmd = virCommandNewArgList("iscsiadm", "--mode", "iface", "--interface", temp_ifacename, "--op", "update", @@ -244,8 +243,8 @@ virStorageBackendCreateIfaceIQN(const char *initiatoriqn, * rely on whether iface file got updated properly. */ if (virCommandRun(updatecmd, &exitstatus) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to run command '%1$s' to update iscsi interface with IQN '%2$s'"), - ISCSIADM, initiatoriqn); + _("Failed to run command 'iscsiadm' to update iscsi interface with IQN '%1$s'"), + initiatoriqn); return -1; } @@ -273,7 +272,7 @@ virISCSIConnection(const char *portal, const char **extraargv) { const char *const baseargv[] = { - ISCSIADM, + "iscsiadm", "--mode", "node", "--portal", portal, "--targetname", target, @@ -343,11 +342,11 @@ virISCSIConnectionLogout(const char *portal, int virISCSIRescanLUNs(const char *session) { - g_autoptr(virCommand) cmd = virCommandNewArgList(ISCSIADM, - "--mode", "session", - "-r", session, - "-R", - NULL); + g_autoptr(virCommand) cmd = virCommandNewArgList("iscsiadm", + "--mode", "session", + "-r", session, + "-R", + NULL); return virCommandRun(cmd, NULL); } @@ -396,11 +395,11 @@ virISCSIScanTargetsInternal(const char *portal, int vars[] = { 2 }; struct virISCSITargetList list = { 0 }; size_t i; - g_autoptr(virCommand) cmd = virCommandNewArgList(ISCSIADM, - "--mode", "discovery", - "--type", "sendtargets", - "--portal", portal, - NULL); + g_autoptr(virCommand) cmd = virCommandNewArgList("iscsiadm", + "--mode", "discovery", + "--type", "sendtargets", + "--portal", portal, + NULL); if (!persist) { virCommandAddArgList(cmd, @@ -512,7 +511,7 @@ virISCSINodeNew(const char *portal, g_autoptr(virCommand) cmd = NULL; int status; - cmd = virCommandNewArgList(ISCSIADM, + cmd = virCommandNewArgList("iscsiadm", "--mode", "node", "--portal", portal, "--targetname", target, @@ -528,8 +527,8 @@ virISCSINodeNew(const char *portal, if (status != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("%1$s failed new mode for target '%2$s' with status '%3$d'"), - ISCSIADM, target, status); + _("'iscsiadm': failed new mode for target '%1$s' with status '%2$d'"), + target, status); return -1; } @@ -546,7 +545,7 @@ virISCSINodeUpdate(const char *portal, g_autoptr(virCommand) cmd = NULL; int status; - cmd = virCommandNewArgList(ISCSIADM, + cmd = virCommandNewArgList("iscsiadm", "--mode", "node", "--portal", portal, "--target", target, diff --git a/tests/viriscsitest.c b/tests/viriscsitest.c index f2539e47b0..ee29b6f259 100644 --- a/tests/viriscsitest.c +++ b/tests/viriscsitest.c @@ -82,7 +82,7 @@ static void testIscsiadmCb(const char *const*args, { struct testIscsiadmCbData *data = opaque; - if (args[0] && STREQ(args[0], ISCSIADM) && + if (args[0] && STREQ(args[0], "iscsiadm") && args[1] && STREQ(args[1], "--mode") && args[2] && STREQ(args[2], "session") && args[3] == NULL) { @@ -90,7 +90,7 @@ static void testIscsiadmCb(const char *const*args, *output = g_strdup(iscsiadmSessionOutputNonFlash); else *output = g_strdup(iscsiadmSessionOutput); - } else if (args[0] && STREQ(args[0], ISCSIADM) && + } else if (args[0] && STREQ(args[0], "iscsiadm") && args[1] && STREQ(args[1], "--mode") && args[2] && STREQ(args[2], "discovery") && args[3] && STREQ(args[3], "--type") && @@ -101,7 +101,7 @@ static void testIscsiadmCb(const char *const*args, args[8] && STREQ(args[8], "nonpersistent") && args[9] == NULL) { *output = g_strdup(iscsiadmSendtargetsOutput); - } else if (args[0] && STREQ(args[0], ISCSIADM) && + } else if (args[0] && STREQ(args[0], "iscsiadm") && args[1] && STREQ(args[1], "--mode") && args[2] && STREQ(args[2], "node") && args[3] && STREQ(args[3], "--portal") && @@ -120,7 +120,7 @@ static void testIscsiadmCb(const char *const*args, * target: iqn.2004-06.example:example1:iscsi.test, \ * portal: 10.20.30.40:3260,1] successful. */ - } else if (args[0] && STREQ(args[0], ISCSIADM) && + } else if (args[0] && STREQ(args[0], "iscsiadm") && args[1] && STREQ(args[1], "--mode") && args[2] && STREQ(args[2], "iface") && args[3] == NULL) { @@ -128,7 +128,7 @@ static void testIscsiadmCb(const char *const*args, *output = g_strdup(iscsiadmIfaceIfaceOutput); else *output = g_strdup(iscsiadmIfaceDefaultOutput); - } else if (args[0] && STREQ(args[0], ISCSIADM) && + } else if (args[0] && STREQ(args[0], "iscsiadm") && args[1] && STREQ(args[1], "--mode") && args[2] && STREQ(args[2], "iface") && args[3] && STREQ(args[3], "--interface") && @@ -142,7 +142,7 @@ static void testIscsiadmCb(const char *const*args, * New interface libvirt-iface-03020100 added */ data->iface_created = true; - } else if (args[0] && STREQ(args[0], ISCSIADM) && + } else if (args[0] && STREQ(args[0], "iscsiadm") && args[1] && STREQ(args[1], "--mode") && args[2] && STREQ(args[2], "iface") && args[3] && STREQ(args[3], "--interface") && @@ -160,7 +160,7 @@ static void testIscsiadmCb(const char *const*args, * * libvirt-iface-03020100 updated. */ - } else if (args[0] && STREQ(args[0], ISCSIADM) && + } else if (args[0] && STREQ(args[0], "iscsiadm") && args[1] && STREQ(args[1], "--mode") && args[2] && STREQ(args[2], "discovery") && args[3] && STREQ(args[3], "--type") && @@ -172,7 +172,7 @@ static void testIscsiadmCb(const char *const*args, args[9] == NULL && data->iface_created) { *output = g_strdup(iscsiadmSendtargetsOutput); - } else if (args[0] && STREQ(args[0], ISCSIADM) && + } else if (args[0] && STREQ(args[0], "iscsiadm") && args[1] && STREQ(args[1], "--mode") && args[2] && STREQ(args[2], "node") && args[3] && STREQ(args[3], "--portal") && -- 2.49.0

On Tue, Apr 29, 2025 at 12:14:11 +0100, Daniel P. Berrangé via Devel wrote:
From: Daniel P. Berrangé <berrange@redhat.com>
Allow virCommand to find 'dmidecode' in $PATH. This command is only usable when running privileged since it relies on reading from a privileged kernel file. Thus we can assume both 'bin' and 'sbin' dirs will be in $PATH and virFindFileInPath will do the right thing to find it when called by virCommand.
This was copied from a previous commit message but makes no sense here.
Gating the iscsi driver backend on a isciadm probe is likely to do more harm than good as it needlessly disables the code if the dev forgot to install iscsiadm at build time. As a Linux only command it is simpler to gate the feature based on the platform choice and allow missing binaries to be diagnose at runtime.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- meson.build | 5 ++-- src/util/viriscsi.c | 55 ++++++++++++++++++++++---------------------- tests/viriscsitest.c | 16 ++++++------- 3 files changed, 37 insertions(+), 39 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>

From: Daniel P. Berrangé <berrange@redhat.com> Allow virCommand to find 'mdevctl' in $PATH. This command is only used when running privileged in which case both 'bin' and 'sbin' dirs will be in $PATH, so virFindFileInPath will do the right thing to find it. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/node_device/node_device_driver.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 123b16a292..2c9e749495 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -744,14 +744,14 @@ nodeDeviceGetMdevctlCommand(virNodeDeviceDef *def, case MDEVCTL_CMD_CREATE: /* now is the time to make sure "create" is replaced with "start" on * mdevctl cmdline */ - cmd = virCommandNewArgList(MDEVCTL, "start", NULL); + cmd = virCommandNewArgList("mdevctl", "start", NULL); break; case MDEVCTL_CMD_STOP: case MDEVCTL_CMD_START: case MDEVCTL_CMD_DEFINE: case MDEVCTL_CMD_UNDEFINE: case MDEVCTL_CMD_MODIFY: - cmd = virCommandNewArgList(MDEVCTL, subcommand, NULL); + cmd = virCommandNewArgList("mdevctl", subcommand, NULL); break; case MDEVCTL_CMD_LAST: default: @@ -901,7 +901,7 @@ nodeDeviceGetMdevctlModifySupportCheck(void) g_autoptr(virCommand) cmd = NULL; const char *subcommand = virMdevctlCommandTypeToString(MDEVCTL_CMD_MODIFY); - cmd = virCommandNewArgList(MDEVCTL, + cmd = virCommandNewArgList("mdevctl", subcommand, "--defined", "--live", @@ -1130,7 +1130,7 @@ nodeDeviceGetMdevctlSetAutostartCommand(virNodeDeviceDef *def, bool autostart, char **errmsg) { - virCommand *cmd = virCommandNewArgList(MDEVCTL, + virCommand *cmd = virCommandNewArgList("mdevctl", "modify", "--uuid", def->caps->data.mdev.uuid, @@ -1176,7 +1176,7 @@ nodeDeviceGetMdevctlListCommand(bool defined, char **output, char **errmsg) { - virCommand *cmd = virCommandNewArgList(MDEVCTL, + virCommand *cmd = virCommandNewArgList("mdevctl", "list", "--dumpjson", NULL); @@ -1894,8 +1894,8 @@ nodeDeviceUpdateMediatedDevices(virNodeDeviceDriverState *node_driver) virMdevctlForEachData data = { 0, }; size_t i; - if (!(mdevctl = virFindFileInPath(MDEVCTL))) { - VIR_DEBUG(MDEVCTL " not found. Skipping update of mediated devices."); + if (!(mdevctl = virFindFileInPath("mdevctl"))) { + VIR_DEBUG("'mdevctl' not found. Skipping update of mediated devices."); return 0; } -- 2.49.0

On Tue, Apr 29, 2025 at 12:14:12 +0100, Daniel P. Berrangé via Devel wrote:
From: Daniel P. Berrangé <berrange@redhat.com>
Allow virCommand to find 'mdevctl' in $PATH. This command is only used when running privileged in which case both 'bin' and 'sbin' dirs will be in $PATH, so virFindFileInPath will do the right thing to find it.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/node_device/node_device_driver.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
Missing hunk that got misplaced to 13/20 removing 'mdevctl' from 'optional_programs'. With the above fixed: Reviewed-by: Peter Krempa <pkrempa@redhat.com>

From: Daniel P. Berrangé <berrange@redhat.com> Allow virCommand to find 'mm-ctl' in $PATH. This command is only used when running privileged in which case both 'bin' and 'sbin' dirs will be in $PATH, so virFindFileInPath will do the right thing to find it. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- meson.build | 2 -- src/util/virnetdevmidonet.c | 4 ++-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/meson.build b/meson.build index 9759a3322b..2a7c264114 100644 --- a/meson.build +++ b/meson.build @@ -854,8 +854,6 @@ optional_test_programs = [ ] optional_programs = [ - 'mdevctl', - 'mm-ctl', 'modprobe', 'ovs-vsctl', 'rmmod', diff --git a/src/util/virnetdevmidonet.c b/src/util/virnetdevmidonet.c index 7a285d2bab..a6f1dd1a8f 100644 --- a/src/util/virnetdevmidonet.c +++ b/src/util/virnetdevmidonet.c @@ -43,7 +43,7 @@ virNetDevMidonetBindPort(const char *ifname, virUUIDFormat(virtualport->interfaceID, virtportuuid); - cmd = virCommandNew(MM_CTL); + cmd = virCommandNew("mm-ctl"); virCommandAddArgList(cmd, "--bind-port", virtportuuid, ifname, NULL); @@ -73,7 +73,7 @@ virNetDevMidonetUnbindPort(const virNetDevVPortProfile *virtualport) virUUIDFormat(virtualport->interfaceID, virtportuuid); - cmd = virCommandNew(MM_CTL); + cmd = virCommandNew("mm-ctl"); virCommandAddArgList(cmd, "--unbind-port", virtportuuid, NULL); if (virCommandRun(cmd, NULL) < 0) { -- 2.49.0

On Tue, Apr 29, 2025 at 12:14:13 +0100, Daniel P. Berrangé via Devel wrote:
From: Daniel P. Berrangé <berrange@redhat.com>
Allow virCommand to find 'mm-ctl' in $PATH. This command is only used when running privileged in which case both 'bin' and 'sbin' dirs will be in $PATH, so virFindFileInPath will do the right thing to find it.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- meson.build | 2 -- src/util/virnetdevmidonet.c | 4 ++-- 2 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/meson.build b/meson.build index 9759a3322b..2a7c264114 100644 --- a/meson.build +++ b/meson.build @@ -854,8 +854,6 @@ optional_test_programs = [ ]
optional_programs = [ - 'mdevctl',
12/20
- 'mm-ctl', 'modprobe', 'ovs-vsctl', 'rmmod',
Reviewed-by: Peter Krempa <pkrempa@redhat.com>

From: Daniel P. Berrangé <berrange@redhat.com> Allow virCommand to find 'modprobe' & 'rmmod' in $PATH. These commands are only used when running privileged in which case both 'bin' and 'sbin' dirs will be in $PATH, so virFindFileInPath will do the right thing to find them. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- meson.build | 2 -- src/util/virkmod.c | 4 ++-- tests/virkmodtest.c | 4 ++-- 3 files changed, 4 insertions(+), 6 deletions(-) diff --git a/meson.build b/meson.build index 2a7c264114..3022f0b984 100644 --- a/meson.build +++ b/meson.build @@ -854,9 +854,7 @@ optional_test_programs = [ ] optional_programs = [ - 'modprobe', 'ovs-vsctl', - 'rmmod', 'tc', ] + optional_test_programs diff --git a/src/util/virkmod.c b/src/util/virkmod.c index 4c709f8659..111627f4b1 100644 --- a/src/util/virkmod.c +++ b/src/util/virkmod.c @@ -29,7 +29,7 @@ doModprobe(const char *opts, const char *module, char **outbuf, char **errbuf) { g_autoptr(virCommand) cmd = NULL; - cmd = virCommandNewArgList(MODPROBE, opts, NULL); + cmd = virCommandNewArgList("modprobe", opts, NULL); if (module) virCommandAddArg(cmd, module); if (outbuf) @@ -48,7 +48,7 @@ doRmmod(const char *module, char **errbuf) { g_autoptr(virCommand) cmd = NULL; - cmd = virCommandNewArgList(RMMOD, module, NULL); + cmd = virCommandNewArgList("rmmod", module, NULL); virCommandSetErrorBuffer(cmd, errbuf); if (virCommandRun(cmd, NULL) < 0) diff --git a/tests/virkmodtest.c b/tests/virkmodtest.c index ec28ef1282..eb9a2aa740 100644 --- a/tests/virkmodtest.c +++ b/tests/virkmodtest.c @@ -63,7 +63,7 @@ testKModLoad(const void *args G_GNUC_UNUSED) return -1; } - if (checkOutput(&buf, MODPROBE " -b " MODNAME "\n") < 0) + if (checkOutput(&buf, "modprobe -b " MODNAME "\n") < 0) return -1; return 0; @@ -85,7 +85,7 @@ testKModUnload(const void *args G_GNUC_UNUSED) return -1; } - if (checkOutput(&buf, RMMOD " " MODNAME "\n") < 0) + if (checkOutput(&buf, "rmmod " MODNAME "\n") < 0) return -1; return 0; -- 2.49.0

On Tue, Apr 29, 2025 at 12:14:14 +0100, Daniel P. Berrangé via Devel wrote:
From: Daniel P. Berrangé <berrange@redhat.com>
Allow virCommand to find 'modprobe' & 'rmmod' in $PATH. These commands are only used when running privileged in which case both 'bin' and 'sbin' dirs will be in $PATH, so virFindFileInPath will do the right thing to find them.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- meson.build | 2 -- src/util/virkmod.c | 4 ++-- tests/virkmodtest.c | 4 ++-- 3 files changed, 4 insertions(+), 6 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>

From: Daniel P. Berrangé <berrange@redhat.com> Allow virCommand to find 'ovs-vsctl' in $PATH. This command is only used when running privileged in which case both 'bin' and 'sbin' dirs will be in $PATH, so virFindFileInPath will do the right thing to find it. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- meson.build | 1 - src/util/virnetdevopenvswitch.c | 4 +- tests/virnetdevbandwidthtest.c | 52 +++++++++++----------- tests/virnetdevopenvswitchtest.c | 74 ++++++++++++++++---------------- 4 files changed, 65 insertions(+), 66 deletions(-) diff --git a/meson.build b/meson.build index 3022f0b984..0515820cdd 100644 --- a/meson.build +++ b/meson.build @@ -854,7 +854,6 @@ optional_test_programs = [ ] optional_programs = [ - 'ovs-vsctl', 'tc', ] + optional_test_programs diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c index 0f5d23b794..868d6d26ba 100644 --- a/src/util/virnetdevopenvswitch.c +++ b/src/util/virnetdevopenvswitch.c @@ -56,7 +56,7 @@ virNetDevOpenvswitchSetTimeout(unsigned int timeout) static virCommand * virNetDevOpenvswitchCreateCmd(char **errbuf) { - virCommand *cmd = virCommandNew(OVS_VSCTL); + virCommand *cmd = virCommandNew("ovs-vsctl"); virCommandAddArgFormat(cmd, "--timeout=%u", virNetDevOpenvswitchTimeout); if (errbuf) @@ -552,7 +552,7 @@ virNetDevOpenvswitchGetVhostuserIfname(const char *path, g_autofree char *absoluteOvsVsctlPath = NULL; int status; - if (!(absoluteOvsVsctlPath = virFindFileInPath(OVS_VSCTL))) { + if (!(absoluteOvsVsctlPath = virFindFileInPath("ovs-vsctl"))) { /* If there is no 'ovs-vsctl' then the interface is * probably not an OpenVSwitch interface and the @path to * socket was created by some DPDK testing script (e.g. diff --git a/tests/virnetdevbandwidthtest.c b/tests/virnetdevbandwidthtest.c index 31aa7f469d..b7cf546d9f 100644 --- a/tests/virnetdevbandwidthtest.c +++ b/tests/virnetdevbandwidthtest.c @@ -152,13 +152,13 @@ mymain(void) TC " class add dev eth0 parent 1: classid 1:1 htb rate 1024kbps quantum 87\n" TC " qdisc add dev eth0 parent 1:1 handle 2: sfq perturb 10\n" TC " filter add dev eth0 parent 1:0 protocol all prio 1 handle 1 fw flowid 1\n", - OVS_VSCTL " --timeout=5 --no-heading --columns=_uuid find queue 'external-ids:vm-id=\"" VMUUID "\"' 'external-ids:ifname=\"eth0\"'\n" - OVS_VSCTL " --timeout=5 --no-heading --columns=_uuid find qos 'external-ids:vm-id=\"" VMUUID "\"' 'external-ids:ifname=\"eth0\"'\n" - OVS_VSCTL " --timeout=5 set port eth0 qos=@qos1 'external-ids:vm-id=\"" VMUUID "\"' 'external-ids:ifname=\"eth0\"' --" - " --id=@qos1 create qos type=linux-htb other_config:min-rate=8192000 queues:0=@queue0 'external-ids:vm-id=\"" VMUUID "\"'" - " 'external-ids:ifname=\"eth0\"' --" - " --id=@queue0 create queue other_config:min-rate=8192000 'external-ids:vm-id=\"" VMUUID "\"' 'external-ids:ifname=\"eth0\"'\n" - OVS_VSCTL " --timeout=5 set Interface eth0 ingress_policing_rate=0 ingress_policing_burst=0\n"); + "ovs-vsctl --timeout=5 --no-heading --columns=_uuid find queue 'external-ids:vm-id=\"" VMUUID "\"' 'external-ids:ifname=\"eth0\"'\n" + "ovs-vsctl --timeout=5 --no-heading --columns=_uuid find qos 'external-ids:vm-id=\"" VMUUID "\"' 'external-ids:ifname=\"eth0\"'\n" + "ovs-vsctl --timeout=5 set port eth0 qos=@qos1 'external-ids:vm-id=\"" VMUUID "\"' 'external-ids:ifname=\"eth0\"' --" + " --id=@qos1 create qos type=linux-htb other_config:min-rate=8192000 queues:0=@queue0 'external-ids:vm-id=\"" VMUUID "\"'" + " 'external-ids:ifname=\"eth0\"' --" + " --id=@queue0 create queue other_config:min-rate=8192000 'external-ids:vm-id=\"" VMUUID "\"' 'external-ids:ifname=\"eth0\"'\n" + "ovs-vsctl --timeout=5 set Interface eth0 ingress_policing_rate=0 ingress_policing_burst=0\n"); DO_TEST_SET("<bandwidth>" " <outbound average='1024'/>" @@ -168,9 +168,9 @@ mymain(void) TC " qdisc add dev eth0 ingress\n" TC " filter add dev eth0 parent ffff: protocol all u32 match u32 0 0" " police rate 1024kbps burst 1024kb mtu 64kb drop flowid :1\n", - OVS_VSCTL " --timeout=5 --no-heading --columns=_uuid find queue 'external-ids:vm-id=\"" VMUUID "\"' 'external-ids:ifname=\"eth0\"'\n" - OVS_VSCTL " --timeout=5 --no-heading --columns=_uuid find qos 'external-ids:vm-id=\"" VMUUID "\"' 'external-ids:ifname=\"eth0\"'\n" - OVS_VSCTL " --timeout=5 set Interface eth0 ingress_policing_rate=8192\n"); + "ovs-vsctl --timeout=5 --no-heading --columns=_uuid find queue 'external-ids:vm-id=\"" VMUUID "\"' 'external-ids:ifname=\"eth0\"'\n" + "ovs-vsctl --timeout=5 --no-heading --columns=_uuid find qos 'external-ids:vm-id=\"" VMUUID "\"' 'external-ids:ifname=\"eth0\"'\n" + "ovs-vsctl --timeout=5 set Interface eth0 ingress_policing_rate=8192\n"); DO_TEST_SET("<bandwidth>" " <inbound average='1' peak='2' floor='3' burst='4'/>" @@ -186,14 +186,14 @@ mymain(void) TC " qdisc add dev eth0 ingress\n" TC " filter add dev eth0 parent ffff: protocol all u32 match u32 0 0" " police rate 5kbps burst 7kb mtu 64kb drop flowid :1\n", - OVS_VSCTL " --timeout=5 --no-heading --columns=_uuid find queue 'external-ids:vm-id=\"" VMUUID "\"' 'external-ids:ifname=\"eth0\"'\n" - OVS_VSCTL " --timeout=5 --no-heading --columns=_uuid find qos 'external-ids:vm-id=\"" VMUUID "\"' 'external-ids:ifname=\"eth0\"'\n" - OVS_VSCTL " --timeout=5 set port eth0 qos=@qos1 'external-ids:vm-id=\"" VMUUID "\"' 'external-ids:ifname=\"eth0\"' --" - " --id=@qos1 create qos type=linux-htb other_config:min-rate=8000 other_config:burst=32768 other_config:max-rate=16000" - " queues:0=@queue0 'external-ids:vm-id=\"" VMUUID "\"' 'external-ids:ifname=\"eth0\"' --" - " --id=@queue0 create queue other_config:min-rate=8000 other_config:burst=32768 other_config:max-rate=16000" - " 'external-ids:vm-id=\"" VMUUID "\"' 'external-ids:ifname=\"eth0\"'\n" - OVS_VSCTL " --timeout=5 set Interface eth0 ingress_policing_rate=40 ingress_policing_burst=56\n"); + "ovs-vsctl --timeout=5 --no-heading --columns=_uuid find queue 'external-ids:vm-id=\"" VMUUID "\"' 'external-ids:ifname=\"eth0\"'\n" + "ovs-vsctl --timeout=5 --no-heading --columns=_uuid find qos 'external-ids:vm-id=\"" VMUUID "\"' 'external-ids:ifname=\"eth0\"'\n" + "ovs-vsctl --timeout=5 set port eth0 qos=@qos1 'external-ids:vm-id=\"" VMUUID "\"' 'external-ids:ifname=\"eth0\"' --" + " --id=@qos1 create qos type=linux-htb other_config:min-rate=8000 other_config:burst=32768 other_config:max-rate=16000" + " queues:0=@queue0 'external-ids:vm-id=\"" VMUUID "\"' 'external-ids:ifname=\"eth0\"' --" + " --id=@queue0 create queue other_config:min-rate=8000 other_config:burst=32768 other_config:max-rate=16000" + " 'external-ids:vm-id=\"" VMUUID "\"' 'external-ids:ifname=\"eth0\"'\n" + "ovs-vsctl --timeout=5 set Interface eth0 ingress_policing_rate=40 ingress_policing_burst=56\n"); DO_TEST_SET("<bandwidth>" " <inbound average='4294967295'/>" @@ -210,14 +210,14 @@ mymain(void) TC " filter add dev eth0 parent ffff: protocol all u32 match" " u32 0 0 police rate 4294967295kbps burst 4194303kb mtu 64kb" " drop flowid :1\n", - OVS_VSCTL " --timeout=5 --no-heading --columns=_uuid find queue 'external-ids:vm-id=\"" VMUUID "\"' 'external-ids:ifname=\"eth0\"'\n" - OVS_VSCTL " --timeout=5 --no-heading --columns=_uuid find qos 'external-ids:vm-id=\"" VMUUID "\"' 'external-ids:ifname=\"eth0\"'\n" - OVS_VSCTL " --timeout=5 set port eth0 qos=@qos1 'external-ids:vm-id=\"" VMUUID "\"' 'external-ids:ifname=\"eth0\"' --" - " --id=@qos1 create qos type=linux-htb other_config:min-rate=34359738360000" - " queues:0=@queue0 'external-ids:vm-id=\"" VMUUID "\"' 'external-ids:ifname=\"eth0\"' --" - " --id=@queue0 create queue other_config:min-rate=34359738360000 'external-ids:vm-id=\"" VMUUID "\"'" - " 'external-ids:ifname=\"eth0\"'\n" - OVS_VSCTL " --timeout=5 set Interface eth0 ingress_policing_rate=34359738360\n"); + "ovs-vsctl --timeout=5 --no-heading --columns=_uuid find queue 'external-ids:vm-id=\"" VMUUID "\"' 'external-ids:ifname=\"eth0\"'\n" + "ovs-vsctl --timeout=5 --no-heading --columns=_uuid find qos 'external-ids:vm-id=\"" VMUUID "\"' 'external-ids:ifname=\"eth0\"'\n" + "ovs-vsctl --timeout=5 set port eth0 qos=@qos1 'external-ids:vm-id=\"" VMUUID "\"' 'external-ids:ifname=\"eth0\"' --" + " --id=@qos1 create qos type=linux-htb other_config:min-rate=34359738360000" + " queues:0=@queue0 'external-ids:vm-id=\"" VMUUID "\"' 'external-ids:ifname=\"eth0\"' --" + " --id=@queue0 create queue other_config:min-rate=34359738360000 'external-ids:vm-id=\"" VMUUID "\"'" + " 'external-ids:ifname=\"eth0\"'\n" + "ovs-vsctl --timeout=5 set Interface eth0 ingress_policing_rate=34359738360\n"); return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } diff --git a/tests/virnetdevopenvswitchtest.c b/tests/virnetdevopenvswitchtest.c index 6e93f5e65a..ffd75ad044 100644 --- a/tests/virnetdevopenvswitchtest.c +++ b/tests/virnetdevopenvswitchtest.c @@ -259,22 +259,22 @@ mymain(void) DO_TEST_SET(("<bandwidth>" " <inbound average='20000'/>" "</bandwidth>"), - (OVS_VSCTL " --timeout=5 --no-heading --columns=_uuid find queue" - " 'external-ids:vm-id=\"66616b65-7575-6964-0000-000000000000\"'" - " 'external-ids:ifname=\"tap-fake\"'\n" - OVS_VSCTL " --timeout=5 --no-heading --columns=_uuid find qos" - " 'external-ids:vm-id=\"66616b65-7575-6964-0000-000000000000\"'" - " 'external-ids:ifname=\"tap-fake\"'\n" - OVS_VSCTL " --timeout=5 set port tap-fake qos=@qos1" - " 'external-ids:vm-id=\"66616b65-7575-6964-0000-000000000000\"'" - " 'external-ids:ifname=\"tap-fake\"'" - " -- --id=@qos1 create qos type=linux-htb other_config:min-rate=160000000" - " queues:0=@queue0 'external-ids:vm-id=\"66616b65-7575-6964-0000-000000000000\"'" - " 'external-ids:ifname=\"tap-fake\"'" - " -- --id=@queue0 create queue other_config:min-rate=160000000 " - "'external-ids:vm-id=\"66616b65-7575-6964-0000-000000000000\"'" - " 'external-ids:ifname=\"tap-fake\"'\n" - OVS_VSCTL " --timeout=5 set Interface tap-fake ingress_policing_rate=0 ingress_policing_burst=0\n")); + ("ovs-vsctl --timeout=5 --no-heading --columns=_uuid find queue" + " 'external-ids:vm-id=\"66616b65-7575-6964-0000-000000000000\"'" + " 'external-ids:ifname=\"tap-fake\"'\n" + "ovs-vsctl --timeout=5 --no-heading --columns=_uuid find qos" + " 'external-ids:vm-id=\"66616b65-7575-6964-0000-000000000000\"'" + " 'external-ids:ifname=\"tap-fake\"'\n" + "ovs-vsctl --timeout=5 set port tap-fake qos=@qos1" + " 'external-ids:vm-id=\"66616b65-7575-6964-0000-000000000000\"'" + " 'external-ids:ifname=\"tap-fake\"'" + " -- --id=@qos1 create qos type=linux-htb other_config:min-rate=160000000" + " queues:0=@queue0 'external-ids:vm-id=\"66616b65-7575-6964-0000-000000000000\"'" + " 'external-ids:ifname=\"tap-fake\"'" + " -- --id=@queue0 create queue other_config:min-rate=160000000 " + "'external-ids:vm-id=\"66616b65-7575-6964-0000-000000000000\"'" + " 'external-ids:ifname=\"tap-fake\"'\n" + "ovs-vsctl --timeout=5 set Interface tap-fake ingress_policing_rate=0 ingress_policing_burst=0\n")); DO_TEST_SET(NULL, NULL); @@ -283,25 +283,25 @@ mymain(void) DO_TEST_SET(("<bandwidth>" " <inbound average='0' />" "</bandwidth>"), - (OVS_VSCTL " --timeout=5 --no-heading --columns=_uuid find queue" - " 'external-ids:vm-id=\"66616b65-7575-6964-0000-000000000000\"'" - " 'external-ids:ifname=\"tap-fake\"'\n" - OVS_VSCTL " --timeout=5 --no-heading --columns=_uuid find qos" - " 'external-ids:vm-id=\"66616b65-7575-6964-0000-000000000000\"'" - " 'external-ids:ifname=\"tap-fake\"'\n" - OVS_VSCTL " --timeout=5 set Interface tap-fake ingress_policing_rate=0 ingress_policing_burst=0\n")); + ("ovs-vsctl --timeout=5 --no-heading --columns=_uuid find queue" + " 'external-ids:vm-id=\"66616b65-7575-6964-0000-000000000000\"'" + " 'external-ids:ifname=\"tap-fake\"'\n" + "ovs-vsctl --timeout=5 --no-heading --columns=_uuid find qos" + " 'external-ids:vm-id=\"66616b65-7575-6964-0000-000000000000\"'" + " 'external-ids:ifname=\"tap-fake\"'\n" + "ovs-vsctl --timeout=5 set Interface tap-fake ingress_policing_rate=0 ingress_policing_burst=0\n")); DO_TEST_SET(("<bandwidth>" " <inbound average='0' />" " <outbound average='5000' />" "</bandwidth>"), - (OVS_VSCTL " --timeout=5 --no-heading --columns=_uuid find queue" - " 'external-ids:vm-id=\"66616b65-7575-6964-0000-000000000000\"'" - " 'external-ids:ifname=\"tap-fake\"'\n" - OVS_VSCTL " --timeout=5 --no-heading --columns=_uuid find qos" - " 'external-ids:vm-id=\"66616b65-7575-6964-0000-000000000000\"'" - " 'external-ids:ifname=\"tap-fake\"'\n" - OVS_VSCTL " --timeout=5 set Interface tap-fake ingress_policing_rate=40000\n")); + ("ovs-vsctl --timeout=5 --no-heading --columns=_uuid find queue" + " 'external-ids:vm-id=\"66616b65-7575-6964-0000-000000000000\"'" + " 'external-ids:ifname=\"tap-fake\"'\n" + "ovs-vsctl --timeout=5 --no-heading --columns=_uuid find qos" + " 'external-ids:vm-id=\"66616b65-7575-6964-0000-000000000000\"'" + " 'external-ids:ifname=\"tap-fake\"'\n" + "ovs-vsctl --timeout=5 set Interface tap-fake ingress_policing_rate=40000\n")); #define DO_TEST_CLEAR_QOS(Iface, Vmid, Exp_cmd, ...) \ do { \ @@ -316,13 +316,13 @@ mymain(void) } while (0) DO_TEST_CLEAR_QOS(("fake-iface"), vm_id, - (OVS_VSCTL " --timeout=5 --no-heading --columns=_uuid find queue" - " 'external-ids:vm-id=\"66616b65-7575-6964-0000-000000000000\"'" - " 'external-ids:ifname=\"fake-iface\"'\n" - OVS_VSCTL " --timeout=5 --no-heading --columns=_uuid find qos" - " 'external-ids:vm-id=\"66616b65-7575-6964-0000-000000000000\"'" - " 'external-ids:ifname=\"fake-iface\"'\n" - OVS_VSCTL " --timeout=5 set Interface fake-iface ingress_policing_rate=0 ingress_policing_burst=0\n")); + ("ovs-vsctl --timeout=5 --no-heading --columns=_uuid find queue" + " 'external-ids:vm-id=\"66616b65-7575-6964-0000-000000000000\"'" + " 'external-ids:ifname=\"fake-iface\"'\n" + "ovs-vsctl --timeout=5 --no-heading --columns=_uuid find qos" + " 'external-ids:vm-id=\"66616b65-7575-6964-0000-000000000000\"'" + " 'external-ids:ifname=\"fake-iface\"'\n" + "ovs-vsctl --timeout=5 set Interface fake-iface ingress_policing_rate=0 ingress_policing_burst=0\n")); return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 2.49.0

On Tue, Apr 29, 2025 at 12:14:15 +0100, Daniel P. Berrangé via Devel wrote:
From: Daniel P. Berrangé <berrange@redhat.com>
Allow virCommand to find 'ovs-vsctl' in $PATH. This command is only used when running privileged in which case both 'bin' and 'sbin' dirs will be in $PATH, so virFindFileInPath will do the right thing to find it.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- meson.build | 1 - src/util/virnetdevopenvswitch.c | 4 +- tests/virnetdevbandwidthtest.c | 52 +++++++++++----------- tests/virnetdevopenvswitchtest.c | 74 ++++++++++++++++---------------- 4 files changed, 65 insertions(+), 66 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>

From: Daniel P. Berrangé <berrange@redhat.com> Allow virCommand to find 'tc' in $PATH. This command is only used when running privileged in which case both 'bin' and 'sbin' dirs will be in $PATH, so virFindFileInPath will do the right thing to find it. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- meson.build | 10 ++---- src/util/virfirewall.h | 1 + src/util/virnetdevbandwidth.c | 36 ++++++++++----------- tests/virnetdevbandwidthtest.c | 58 +++++++++++++++++----------------- 4 files changed, 50 insertions(+), 55 deletions(-) diff --git a/meson.build b/meson.build index 0515820cdd..4223f5ab86 100644 --- a/meson.build +++ b/meson.build @@ -845,7 +845,7 @@ endforeach # optional programs -optional_test_programs = [ +optional_programs = [ 'augparse', 'black', 'flake8', @@ -853,10 +853,6 @@ optional_test_programs = [ 'pytest', ] -optional_programs = [ - 'tc', -] + optional_test_programs - missing_optional_programs = [] foreach name : optional_programs prog = find_program(name, required: false, dirs: libvirt_sbin_path) @@ -865,9 +861,7 @@ foreach name : optional_programs prog_path = prog.full_path() else prog_path = name - if name in optional_test_programs - missing_optional_programs += [ name ] - endif + missing_optional_programs += [ name ] endif conf.set_quoted(varname.to_upper(), prog_path) diff --git a/src/util/virfirewall.h b/src/util/virfirewall.h index d42e60884b..07391bea67 100644 --- a/src/util/virfirewall.h +++ b/src/util/virfirewall.h @@ -30,6 +30,7 @@ #define IPTABLES "iptables" #define IP6TABLES "ip6tables" #define NFT "nft" +#define TC "tc" typedef struct _virFirewall virFirewall; diff --git a/src/util/virnetdevbandwidth.c b/src/util/virnetdevbandwidth.c index 5c6a65528c..b141a38b10 100644 --- a/src/util/virnetdevbandwidth.c +++ b/src/util/virnetdevbandwidth.c @@ -126,7 +126,7 @@ virNetDevBandwidthManipulateFilter(const char *ifname, filter_id = g_strdup_printf("800::%u", 800 + id); if (remove_old) { - g_autoptr(virCommand) cmd = virCommandNew(TC); + g_autoptr(virCommand) cmd = virCommandNew("tc"); int cmd_ret = 0; virCommandAddArgList(cmd, "filter", "del", "dev", ifname, @@ -138,7 +138,7 @@ virNetDevBandwidthManipulateFilter(const char *ifname, } if (create_new) { - g_autoptr(virCommand) cmd = virCommandNew(TC); + g_autoptr(virCommand) cmd = virCommandNew("tc"); virMacAddrGetRaw(ifmac_ptr, ifmac); mac[0] = g_strdup_printf("0x%02x%02x%02x%02x", ifmac[2], @@ -334,7 +334,7 @@ virNetDevBandwidthSet(const char *ifname, */ if (hierarchical_class) { virCommandFree(cmd); - cmd = virCommandNew(TC); + cmd = virCommandNew("tc"); virCommandAddArgList(cmd, "class", "add", "dev", ifname, "parent", "1:", "classid", "1:1", "htb", "rate", average, "ceil", peak ? peak : average, NULL); @@ -343,7 +343,7 @@ virNetDevBandwidthSet(const char *ifname, goto cleanup; } virCommandFree(cmd); - cmd = virCommandNew(TC); + cmd = virCommandNew("tc"); virCommandAddArgList(cmd, "class", "add", "dev", ifname, "parent", hierarchical_class ? "1:1" : "1:", "classid", hierarchical_class ? "1:2" : "1:1", "htb", @@ -359,7 +359,7 @@ virNetDevBandwidthSet(const char *ifname, goto cleanup; virCommandFree(cmd); - cmd = virCommandNew(TC); + cmd = virCommandNew("tc"); virCommandAddArgList(cmd, "qdisc", "add", "dev", ifname, "parent", hierarchical_class ? "1:2" : "1:1", "handle", "2:", "sfq", "perturb", @@ -369,7 +369,7 @@ virNetDevBandwidthSet(const char *ifname, goto cleanup; virCommandFree(cmd); - cmd = virCommandNew(TC); + cmd = virCommandNew("tc"); virCommandAddArgList(cmd, "filter", "add", "dev", ifname, "parent", "1:0", "protocol", "all", "prio", "1", "handle", "1", "fw", "flowid", "1", NULL); @@ -397,7 +397,7 @@ virNetDevBandwidthSet(const char *ifname, } virCommandFree(cmd); - cmd = virCommandNew(TC); + cmd = virCommandNew("tc"); virCommandAddArgList(cmd, "qdisc", "add", "dev", ifname, "ingress", NULL); @@ -405,7 +405,7 @@ virNetDevBandwidthSet(const char *ifname, goto cleanup; virCommandFree(cmd); - cmd = virCommandNew(TC); + cmd = virCommandNew("tc"); /* Set filter to match all ingress traffic */ virCommandAddArgList(cmd, "filter", "add", "dev", ifname, "parent", "ffff:", "protocol", "all", "u32", "match", "u32", @@ -448,13 +448,13 @@ virNetDevBandwidthClear(const char *ifname) if (!ifname) return 0; - rootcmd = virCommandNew(TC); + rootcmd = virCommandNew("tc"); virCommandAddArgList(rootcmd, "qdisc", "del", "dev", ifname, "root", NULL); if (virCommandRun(rootcmd, &dummy) < 0) ret = -1; - ingresscmd = virCommandNew(TC); + ingresscmd = virCommandNew("tc"); virCommandAddArgList(ingresscmd, "qdisc", "del", "dev", ifname, "ingress", NULL); if (virCommandRun(ingresscmd, &dummy) < 0) @@ -593,7 +593,7 @@ virNetDevBandwidthPlug(const char *brname, net_bandwidth->in->peak : net_bandwidth->in->average); - cmd1 = virCommandNew(TC); + cmd1 = virCommandNew("tc"); virCommandAddArgList(cmd1, "class", "add", "dev", brname, "parent", "1:1", "classid", class_id, "htb", "rate", floor, "ceil", ceil, NULL); @@ -602,7 +602,7 @@ virNetDevBandwidthPlug(const char *brname, if (virCommandRun(cmd1, NULL) < 0) return -1; - cmd2 = virCommandNew(TC); + cmd2 = virCommandNew("tc"); virCommandAddArgList(cmd2, "qdisc", "add", "dev", brname, "parent", class_id, "handle", qdisc_id, "sfq", "perturb", "10", NULL); @@ -644,7 +644,7 @@ virNetDevBandwidthUnplug(const char *brname, class_id = g_strdup_printf("1:%x", id); qdisc_id = g_strdup_printf("%x:", id); - cmd1 = virCommandNew(TC); + cmd1 = virCommandNew("tc"); virCommandAddArgList(cmd1, "qdisc", "del", "dev", brname, "handle", qdisc_id, NULL); @@ -657,7 +657,7 @@ virNetDevBandwidthUnplug(const char *brname, NULL, true, false) < 0) return -1; - cmd2 = virCommandNew(TC); + cmd2 = virCommandNew("tc"); virCommandAddArgList(cmd2, "class", "del", "dev", brname, "classid", class_id, NULL); @@ -698,7 +698,7 @@ virNetDevBandwidthUpdateRate(const char *ifname, bandwidth->in->peak : bandwidth->in->average); - cmd = virCommandNew(TC); + cmd = virCommandNew("tc"); virCommandAddArgList(cmd, "class", "change", "dev", ifname, "classid", class_id, "htb", "rate", rate, "ceil", ceil, NULL); @@ -772,7 +772,7 @@ virNetDevBandwidthSetRootQDisc(const char *ifname, /* Ideally, we would have a netlink implementation and just * call it here. But honestly, I tried and failed miserably. * Fallback to spawning tc. */ - cmd = virCommandNewArgList(TC, "qdisc", "add", "dev", ifname, + cmd = virCommandNewArgList("tc", "qdisc", "add", "dev", ifname, "root", "handle", "0:", qdisc, NULL); @@ -811,7 +811,7 @@ virNetDevBandWidthAddTxFilterParentQdisc(const char *ifname, /* first check it the qdisc with handle 1: was already added for * this interface by someone else */ - testCmd = virCommandNew(TC); + testCmd = virCommandNew("tc"); virCommandAddArgList(testCmd, "qdisc", "show", "dev", ifname, "handle", "1:", NULL); virCommandSetOutputBuffer(testCmd, &testResult); @@ -826,7 +826,7 @@ virNetDevBandWidthAddTxFilterParentQdisc(const char *ifname, */ if (!(testResult && strstr(testResult, "qdisc") && strstr(testResult, " 1: "))) { /* didn't find qdisc in output, so we need to add one */ - g_autoptr(virCommand) addCmd = virCommandNew(TC); + g_autoptr(virCommand) addCmd = virCommandNew("tc"); virCommandAddArgList(addCmd, "qdisc", "add", "dev", ifname, "root", "handle", "1:", "htb", "default", diff --git a/tests/virnetdevbandwidthtest.c b/tests/virnetdevbandwidthtest.c index b7cf546d9f..9624da9b96 100644 --- a/tests/virnetdevbandwidthtest.c +++ b/tests/virnetdevbandwidthtest.c @@ -145,13 +145,13 @@ mymain(void) DO_TEST_SET("<bandwidth>" " <inbound average='1024'/>" "</bandwidth>", - TC " qdisc del dev eth0 root\n" - TC " qdisc del dev eth0 ingress\n" - TC " qdisc show dev eth0 handle 1:\n" - TC " qdisc add dev eth0 root handle 1: htb default 1\n" - TC " class add dev eth0 parent 1: classid 1:1 htb rate 1024kbps quantum 87\n" - TC " qdisc add dev eth0 parent 1:1 handle 2: sfq perturb 10\n" - TC " filter add dev eth0 parent 1:0 protocol all prio 1 handle 1 fw flowid 1\n", + "tc qdisc del dev eth0 root\n" + "tc qdisc del dev eth0 ingress\n" + "tc qdisc show dev eth0 handle 1:\n" + "tc qdisc add dev eth0 root handle 1: htb default 1\n" + "tc class add dev eth0 parent 1: classid 1:1 htb rate 1024kbps quantum 87\n" + "tc qdisc add dev eth0 parent 1:1 handle 2: sfq perturb 10\n" + "tc filter add dev eth0 parent 1:0 protocol all prio 1 handle 1 fw flowid 1\n", "ovs-vsctl --timeout=5 --no-heading --columns=_uuid find queue 'external-ids:vm-id=\"" VMUUID "\"' 'external-ids:ifname=\"eth0\"'\n" "ovs-vsctl --timeout=5 --no-heading --columns=_uuid find qos 'external-ids:vm-id=\"" VMUUID "\"' 'external-ids:ifname=\"eth0\"'\n" "ovs-vsctl --timeout=5 set port eth0 qos=@qos1 'external-ids:vm-id=\"" VMUUID "\"' 'external-ids:ifname=\"eth0\"' --" @@ -163,10 +163,10 @@ mymain(void) DO_TEST_SET("<bandwidth>" " <outbound average='1024'/>" "</bandwidth>", - TC " qdisc del dev eth0 root\n" - TC " qdisc del dev eth0 ingress\n" - TC " qdisc add dev eth0 ingress\n" - TC " filter add dev eth0 parent ffff: protocol all u32 match u32 0 0" + "tc qdisc del dev eth0 root\n" + "tc qdisc del dev eth0 ingress\n" + "tc qdisc add dev eth0 ingress\n" + "tc filter add dev eth0 parent ffff: protocol all u32 match u32 0 0" " police rate 1024kbps burst 1024kb mtu 64kb drop flowid :1\n", "ovs-vsctl --timeout=5 --no-heading --columns=_uuid find queue 'external-ids:vm-id=\"" VMUUID "\"' 'external-ids:ifname=\"eth0\"'\n" "ovs-vsctl --timeout=5 --no-heading --columns=_uuid find qos 'external-ids:vm-id=\"" VMUUID "\"' 'external-ids:ifname=\"eth0\"'\n" @@ -176,15 +176,15 @@ mymain(void) " <inbound average='1' peak='2' floor='3' burst='4'/>" " <outbound average='5' peak='6' burst='7'/>" "</bandwidth>", - TC " qdisc del dev eth0 root\n" - TC " qdisc del dev eth0 ingress\n" - TC " qdisc show dev eth0 handle 1:\n" - TC " qdisc add dev eth0 root handle 1: htb default 1\n" - TC " class add dev eth0 parent 1: classid 1:1 htb rate 1kbps ceil 2kbps burst 4kb quantum 1\n" - TC " qdisc add dev eth0 parent 1:1 handle 2: sfq perturb 10\n" - TC " filter add dev eth0 parent 1:0 protocol all prio 1 handle 1 fw flowid 1\n" - TC " qdisc add dev eth0 ingress\n" - TC " filter add dev eth0 parent ffff: protocol all u32 match u32 0 0" + "tc qdisc del dev eth0 root\n" + "tc qdisc del dev eth0 ingress\n" + "tc qdisc show dev eth0 handle 1:\n" + "tc qdisc add dev eth0 root handle 1: htb default 1\n" + "tc class add dev eth0 parent 1: classid 1:1 htb rate 1kbps ceil 2kbps burst 4kb quantum 1\n" + "tc qdisc add dev eth0 parent 1:1 handle 2: sfq perturb 10\n" + "tc filter add dev eth0 parent 1:0 protocol all prio 1 handle 1 fw flowid 1\n" + "tc qdisc add dev eth0 ingress\n" + "tc filter add dev eth0 parent ffff: protocol all u32 match u32 0 0" " police rate 5kbps burst 7kb mtu 64kb drop flowid :1\n", "ovs-vsctl --timeout=5 --no-heading --columns=_uuid find queue 'external-ids:vm-id=\"" VMUUID "\"' 'external-ids:ifname=\"eth0\"'\n" "ovs-vsctl --timeout=5 --no-heading --columns=_uuid find qos 'external-ids:vm-id=\"" VMUUID "\"' 'external-ids:ifname=\"eth0\"'\n" @@ -199,15 +199,15 @@ mymain(void) " <inbound average='4294967295'/>" " <outbound average='4294967295'/>" "</bandwidth>", - TC " qdisc del dev eth0 root\n" - TC " qdisc del dev eth0 ingress\n" - TC " qdisc show dev eth0 handle 1:\n" - TC " qdisc add dev eth0 root handle 1: htb default 1\n" - TC " class add dev eth0 parent 1: classid 1:1 htb rate 4294967295kbps quantum 366503875\n" - TC " qdisc add dev eth0 parent 1:1 handle 2: sfq perturb 10\n" - TC " filter add dev eth0 parent 1:0 protocol all prio 1 handle 1 fw flowid 1\n" - TC " qdisc add dev eth0 ingress\n" - TC " filter add dev eth0 parent ffff: protocol all u32 match" + "tc qdisc del dev eth0 root\n" + "tc qdisc del dev eth0 ingress\n" + "tc qdisc show dev eth0 handle 1:\n" + "tc qdisc add dev eth0 root handle 1: htb default 1\n" + "tc class add dev eth0 parent 1: classid 1:1 htb rate 4294967295kbps quantum 366503875\n" + "tc qdisc add dev eth0 parent 1:1 handle 2: sfq perturb 10\n" + "tc filter add dev eth0 parent 1:0 protocol all prio 1 handle 1 fw flowid 1\n" + "tc qdisc add dev eth0 ingress\n" + "tc filter add dev eth0 parent ffff: protocol all u32 match" " u32 0 0 police rate 4294967295kbps burst 4194303kb mtu 64kb" " drop flowid :1\n", "ovs-vsctl --timeout=5 --no-heading --columns=_uuid find queue 'external-ids:vm-id=\"" VMUUID "\"' 'external-ids:ifname=\"eth0\"'\n" -- 2.49.0

On Tue, Apr 29, 2025 at 12:14:16 +0100, Daniel P. Berrangé via Devel wrote:
From: Daniel P. Berrangé <berrange@redhat.com>
Allow virCommand to find 'tc' in $PATH. This command is only used when running privileged in which case both 'bin' and 'sbin' dirs will be in $PATH, so virFindFileInPath will do the right thing to find it.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- meson.build | 10 ++---- src/util/virfirewall.h | 1 + src/util/virnetdevbandwidth.c | 36 ++++++++++----------- tests/virnetdevbandwidthtest.c | 58 +++++++++++++++++----------------- 4 files changed, 50 insertions(+), 55 deletions(-)
diff --git a/meson.build b/meson.build index 0515820cdd..4223f5ab86 100644 --- a/meson.build +++ b/meson.build @@ -845,7 +845,7 @@ endforeach
# optional programs
-optional_test_programs = [ +optional_programs = [ 'augparse', 'black', 'flake8', @@ -853,10 +853,6 @@ optional_test_programs = [ 'pytest', ]
-optional_programs = [ - 'tc', -] + optional_test_programs - missing_optional_programs = [] foreach name : optional_programs prog = find_program(name, required: false, dirs: libvirt_sbin_path) @@ -865,9 +861,7 @@ foreach name : optional_programs prog_path = prog.full_path() else prog_path = name - if name in optional_test_programs - missing_optional_programs += [ name ] - endif + missing_optional_programs += [ name ] endif
I had to dig a bit what happened here. Please mention in the commit message that as there are no optional programs for runtime we're left with optional programs for testing (which are also moaned about in the configure output).
conf.set_quoted(varname.to_upper(), prog_path) diff --git a/src/util/virfirewall.h b/src/util/virfirewall.h index d42e60884b..07391bea67 100644 --- a/src/util/virfirewall.h +++ b/src/util/virfirewall.h @@ -30,6 +30,7 @@ #define IPTABLES "iptables" #define IP6TABLES "ip6tables" #define NFT "nft" +#define TC "tc"
There are just two places referencing the TC macro after this. Do we still need it? src/util/virfirewall.c=VIR_ENUM_IMPL(virFirewallLayerCommand, src/util/virfirewall.c: TC, src/util/virfirewall.c=virFirewallCmdNftablesApply(virFirewall *firewall G_GNUC_UNUSED, src/util/virfirewall.c: cmd = virCommandNew(TC); The rest looks good. If you justify/fix the thing above and mention the build system change in the commit message: Reviewed-by: Peter Krempa <pkrempa@redhat.com>

On Tue, Apr 29, 2025 at 02:38:45PM +0200, Peter Krempa wrote:
On Tue, Apr 29, 2025 at 12:14:16 +0100, Daniel P. Berrangé via Devel wrote:
From: Daniel P. Berrangé <berrange@redhat.com>
Allow virCommand to find 'tc' in $PATH. This command is only used when running privileged in which case both 'bin' and 'sbin' dirs will be in $PATH, so virFindFileInPath will do the right thing to find it.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- meson.build | 10 ++---- src/util/virfirewall.h | 1 + src/util/virnetdevbandwidth.c | 36 ++++++++++----------- tests/virnetdevbandwidthtest.c | 58 +++++++++++++++++----------------- 4 files changed, 50 insertions(+), 55 deletions(-)
diff --git a/src/util/virfirewall.h b/src/util/virfirewall.h index d42e60884b..07391bea67 100644 --- a/src/util/virfirewall.h +++ b/src/util/virfirewall.h @@ -30,6 +30,7 @@ #define IPTABLES "iptables" #define IP6TABLES "ip6tables" #define NFT "nft" +#define TC "tc"
There are just two places referencing the TC macro after this. Do we still need it?
src/util/virfirewall.c=VIR_ENUM_IMPL(virFirewallLayerCommand, src/util/virfirewall.c: TC, src/util/virfirewall.c=virFirewallCmdNftablesApply(virFirewall *firewall G_GNUC_UNUSED, src/util/virfirewall.c: cmd = virCommandNew(TC);
I was borderline on this - in the end since we have NFT/IPTABLES/IP6TABLES constants I went for consistency within virfirewall.c With 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 Tue, Apr 29, 2025 at 13:40:31 +0100, Daniel P. Berrangé wrote:
On Tue, Apr 29, 2025 at 02:38:45PM +0200, Peter Krempa wrote:
On Tue, Apr 29, 2025 at 12:14:16 +0100, Daniel P. Berrangé via Devel wrote:
From: Daniel P. Berrangé <berrange@redhat.com>
Allow virCommand to find 'tc' in $PATH. This command is only used when running privileged in which case both 'bin' and 'sbin' dirs will be in $PATH, so virFindFileInPath will do the right thing to find it.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- meson.build | 10 ++---- src/util/virfirewall.h | 1 + src/util/virnetdevbandwidth.c | 36 ++++++++++----------- tests/virnetdevbandwidthtest.c | 58 +++++++++++++++++----------------- 4 files changed, 50 insertions(+), 55 deletions(-)
diff --git a/src/util/virfirewall.h b/src/util/virfirewall.h index d42e60884b..07391bea67 100644 --- a/src/util/virfirewall.h +++ b/src/util/virfirewall.h @@ -30,6 +30,7 @@ #define IPTABLES "iptables" #define IP6TABLES "ip6tables" #define NFT "nft" +#define TC "tc"
There are just two places referencing the TC macro after this. Do we still need it?
src/util/virfirewall.c=VIR_ENUM_IMPL(virFirewallLayerCommand, src/util/virfirewall.c: TC, src/util/virfirewall.c=virFirewallCmdNftablesApply(virFirewall *firewall G_GNUC_UNUSED, src/util/virfirewall.c: cmd = virCommandNew(TC);
I was borderline on this - in the end since we have NFT/IPTABLES/IP6TABLES constants I went for consistency within virfirewall.c
Fair enough keep it as is.

From: Daniel P. Berrangé <berrange@redhat.com> There is no reference to AUGPARSE, BLACK, FLAKE8, PDWTAGS or PYTEST conf variables anywhere, only the ${name}_prog meson variables are used. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- meson.build | 1 - 1 file changed, 1 deletion(-) diff --git a/meson.build b/meson.build index 4223f5ab86..68b955a02c 100644 --- a/meson.build +++ b/meson.build @@ -864,7 +864,6 @@ foreach name : optional_programs missing_optional_programs += [ name ] endif - conf.set_quoted(varname.to_upper(), prog_path) set_variable('@0@_prog'.format(varname), prog) endforeach -- 2.49.0

On Tue, Apr 29, 2025 at 12:14:17 +0100, Daniel P. Berrangé via Devel wrote:
From: Daniel P. Berrangé <berrange@redhat.com>
There is no reference to AUGPARSE, BLACK, FLAKE8, PDWTAGS or PYTEST conf variables anywhere, only the ${name}_prog meson variables are used.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- meson.build | 1 - 1 file changed, 1 deletion(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>

From: Daniel P. Berrangé <berrange@redhat.com> Change the mesonm rules to always enable the LVM driver if on a Linux host, unless the meson options say not to. The virCommand APIs will return suitable runtime errors if the tools are not installed. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- meson.build | 30 +++++---------------------- src/locking/lock_driver_lockd.c | 4 ++-- src/storage/storage_backend_logical.c | 24 ++++++++++----------- 3 files changed, 19 insertions(+), 39 deletions(-) diff --git a/meson.build b/meson.build index 68b955a02c..e23777819d 100644 --- a/meson.build +++ b/meson.build @@ -1826,32 +1826,12 @@ if conf.has('WITH_LIBVIRTD') error('Need libiscsi for iscsi-direct storage driver') endif - if not get_option('storage_lvm').disabled() + if not get_option('storage_lvm').disabled() and host_machine.system() == 'linux' lvm_enable = true - lvm_progs = [ - 'pvcreate', 'vgcreate', 'lvcreate', - 'pvremove', 'vgremove', 'lvremove', - 'lvchange', 'vgchange', 'vgscan', - 'pvs', 'vgs', 'lvs', - ] - foreach name : lvm_progs - set_variable( - '@0@_prog'.format(name), - find_program(name, required: get_option('storage_lvm'), dirs: libvirt_sbin_path) - ) - if not get_variable('@0@_prog'.format(name)).found() - lvm_enable = false - endif - endforeach - - if lvm_enable - use_storage = true - conf.set('WITH_STORAGE_LVM', 1) - - foreach name : lvm_progs - conf.set_quoted(name.to_upper(), get_variable('@0@_prog'.format(name)).full_path()) - endforeach - endif + use_storage = true + conf.set('WITH_STORAGE_LVM', 1) + elif get_option('storage_lvm').enabled() + error('Linux host needed for LVM storage driver') endif if not get_option('storage_mpath').disabled() and host_machine.system() == 'linux' and devmapper_dep.found() diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c index 0b6c720477..a3bb285eec 100644 --- a/src/locking/lock_driver_lockd.c +++ b/src/locking/lock_driver_lockd.c @@ -433,7 +433,7 @@ static int virLockManagerLockDaemonNew(virLockManager *lock, } -#ifdef LVS +#ifdef __linux__ static int virLockManagerGetLVMKey(const char *path, char **key) @@ -446,7 +446,7 @@ virLockManagerGetLVMKey(const char *path, int ret = -1; g_autoptr(virCommand) cmd = NULL; - cmd = virCommandNewArgList(LVS, "--noheadings", + cmd = virCommandNewArgList("lvs", "--noheadings", "--unbuffered", "--nosuffix", "--options", "uuid", path, NULL diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index 6acbc37f18..51e9337820 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -49,7 +49,7 @@ virStorageBackendLogicalSetActive(virStoragePoolObj *pool, g_autoptr(virCommand) cmd = NULL; int ret; - cmd = virStorageBackendLogicalChangeCmd(VGCHANGE, def, on); + cmd = virStorageBackendLogicalChangeCmd("vgchange", def, on); virObjectUnlock(pool); ret = virCommandRun(cmd, NULL); @@ -70,7 +70,7 @@ virStorageBackendLogicalRemoveDevice(const char *path) { g_autoptr(virCommand) cmd = NULL; - cmd = virCommandNewArgList(PVREMOVE, path, NULL); + cmd = virCommandNewArgList("pvremove", path, NULL); if (virCommandRun(cmd, NULL) < 0) VIR_INFO("Failed to pvremove logical device '%s'", path); } @@ -100,7 +100,7 @@ virStorageBackendLogicalInitializeDevice(const char *path) * Initialize the physical volume because vgcreate is not * clever enough todo this for us :-( */ - pvcmd = virCommandNewArgList(PVCREATE, path, NULL); + pvcmd = virCommandNewArgList("pvcreate", path, NULL); return virCommandRun(pvcmd, NULL); } @@ -380,7 +380,7 @@ virStorageBackendLogicalFindLVs(virStoragePoolObj *pool, }; g_autoptr(virCommand) cmd = NULL; - cmd = virCommandNewArgList(LVS, + cmd = virCommandNewArgList("lvs", "--separator", "#", "--noheadings", "--units", "b", @@ -483,11 +483,11 @@ virStorageBackendLogicalGetPoolSources(virStoragePoolSourceList *sourceList) * that might be hanging around, so if this fails for some reason, the * worst that happens is that scanning doesn't pick everything up */ - vgcmd = virCommandNew(VGSCAN); + vgcmd = virCommandNew("vgscan"); if (virCommandRun(vgcmd, NULL) < 0) VIR_WARN("Failure when running vgscan to refresh physical volumes"); - pvcmd = virCommandNewArgList(PVS, + pvcmd = virCommandNewArgList("pvs", "--noheadings", "-o", "pv_name,vg_name", NULL, NULL); @@ -658,7 +658,7 @@ virStorageBackendLogicalBuildPool(virStoragePoolObj *pool, VIR_STORAGE_POOL_BUILD_NO_OVERWRITE, cleanup); - vgcmd = virCommandNewArgList(VGCREATE, def->source.name, NULL); + vgcmd = virCommandNewArgList("vgcreate", def->source.name, NULL); for (i = 0; i < def->source.ndevice; i++) { const char *path = def->source.devices[i].path; @@ -720,7 +720,7 @@ virStorageBackendLogicalRefreshPool(virStoragePoolObj *pool) if (virStorageBackendLogicalFindLVs(pool, NULL) < 0) return -1; - cmd = virCommandNewArgList(VGS, + cmd = virCommandNewArgList("vgs", "--separator", ":", "--noheadings", "--units", "b", @@ -769,7 +769,7 @@ virStorageBackendLogicalDeletePool(virStoragePoolObj *pool, virCheckFlags(0, -1); /* first remove the volume group */ - cmd = virCommandNewArgList(VGREMOVE, + cmd = virCommandNewArgList("vgremove", "-f", def->source.name, NULL); if (virCommandRun(cmd, NULL) < 0) @@ -795,8 +795,8 @@ virStorageBackendLogicalDeleteVol(virStoragePoolObj *pool G_GNUC_UNUSED, virWaitForDevices(); - lvchange_cmd = virCommandNewArgList(LVCHANGE, "-aln", vol->target.path, NULL); - lvremove_cmd = virCommandNewArgList(LVREMOVE, "-f", vol->target.path, NULL); + lvchange_cmd = virCommandNewArgList("lvchange", "-aln", vol->target.path, NULL); + lvremove_cmd = virCommandNewArgList("lvremove", "-f", vol->target.path, NULL); if (virCommandRun(lvremove_cmd, NULL) < 0) { if (virCommandRun(lvchange_cmd, NULL) < 0) { @@ -825,7 +825,7 @@ virStorageBackendLogicalLVCreate(virStorageVolDef *vol, return -1; } - cmd = virCommandNewArgList(LVCREATE, + cmd = virCommandNewArgList("lvcreate", "--name", vol->name, NULL); virCommandAddArg(cmd, "-L"); -- 2.49.0

On Tue, Apr 29, 2025 at 12:14:18 +0100, Daniel P. Berrangé via Devel wrote:
From: Daniel P. Berrangé <berrange@redhat.com>
Change the mesonm rules to always enable the LVM driver if on a
s/mesonm/meson/
Linux host, unless the meson options say not to.
The virCommand APIs will return suitable runtime errors if the tools are not installed.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- meson.build | 30 +++++---------------------- src/locking/lock_driver_lockd.c | 4 ++-- src/storage/storage_backend_logical.c | 24 ++++++++++----------- 3 files changed, 19 insertions(+), 39 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>

From: Daniel P. Berrangé <berrange@redhat.com> Allow virCommand to find it in $PATH Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/virpolkit.c | 8 +++++--- src/util/virpolkit.h | 2 -- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/util/virpolkit.c b/src/util/virpolkit.c index 0a637db9ba..81fa5d0989 100644 --- a/src/util/virpolkit.c +++ b/src/util/virpolkit.c @@ -189,7 +189,7 @@ virPolkitAgentCreate(void) agent = g_new0(virPolkitAgent, 1); - agent->cmd = virCommandNewArgList(PKTTYAGENT, "--process", NULL); + agent->cmd = virCommandNewArgList("pkttyagent", "--process", NULL); virCommandAddArgFormat(agent->cmd, "%lld", (long long int) getpid()); virCommandAddArg(agent->cmd, "--notify-fd"); @@ -234,11 +234,13 @@ virPolkitAgentAvailable(void) { const char *termid = ctermid(NULL); VIR_AUTOCLOSE fd = -1; + g_autofree char *agent = NULL; - if (!virFileIsExecutable(PKTTYAGENT)) + if (!termid) return false; - if (!termid) + agent = virFindFileInPath("pkttyagent"); + if (!agent) return false; /* diff --git a/src/util/virpolkit.h b/src/util/virpolkit.h index 7bcd040e5e..afc0005365 100644 --- a/src/util/virpolkit.h +++ b/src/util/virpolkit.h @@ -24,8 +24,6 @@ #include "internal.h" #include "vircommand.h" -#define PKTTYAGENT "/usr/bin/pkttyagent" - int virPolkitCheckAuth(const char *actionid, pid_t pid, unsigned long long startTime, -- 2.49.0

On Tue, Apr 29, 2025 at 12:14:19 +0100, Daniel P. Berrangé via Devel wrote:
From: Daniel P. Berrangé <berrange@redhat.com>
Allow virCommand to find it in $PATH
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> ---
Now we can suggest a workaround in https://gitlab.com/libvirt/libvirt/-/issues/757 defining a fake agent in $PATH :)
src/util/virpolkit.c | 8 +++++--- src/util/virpolkit.h | 2 --
Reviewed-by: Peter Krempa <pkrempa@redhat.com>

From: Daniel P. Berrangé <berrange@redhat.com> Allow virCommand to find them in $PATH, which will always include 'sbin' since the openvz driver only operates in system mode. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/openvz/openvz_conf.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/openvz/openvz_conf.h b/src/openvz/openvz_conf.h index 9c3eff9aaf..83244ba60e 100644 --- a/src/openvz/openvz_conf.h +++ b/src/openvz/openvz_conf.h @@ -29,9 +29,9 @@ /* OpenVZ commands - Replace with wrapper scripts later? */ -#define VZLIST "/usr/sbin/vzlist" -#define VZCTL "/usr/sbin/vzctl" -#define VZMIGRATE "/usr/sbin/vzmigrate" +#define VZLIST "vzlist" +#define VZCTL "vzctl" +#define VZMIGRATE "vzmigrate" #define VZ_CONF_FILE "/etc/vz/vz.conf" #define VZCTL_BRIDGE_MIN_VERSION ((3 * 1000 * 1000) + (0 * 1000) + 22 + 1) -- 2.49.0

On Tue, Apr 29, 2025 at 12:14:20 +0100, Daniel P. Berrangé via Devel wrote:
From: Daniel P. Berrangé <berrange@redhat.com>
Allow virCommand to find them in $PATH, which will always include 'sbin' since the openvz driver only operates in system mode.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/openvz/openvz_conf.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>

On a Tuesday in 2025, Daniel P. Berrangé via Devel wrote:
Over the years we've made various changes to stop hardcoding paths at build time, but the work is incomplete.
This bit us significantly in Fedora 42 where they have merged the 'sbin' and 'bin' dirs together. In the build environment we have a clean install where '/sbin' and '/usr/sbin' are simply symlinks to '/usr/bin'.
Our meson.build logic preferentially looks in '/sbin' and '/usr/sbin' to find programs, and thus it finds *all* binaries in '/sbin' and then hardcodes this path. This happens even for binaries that have always been in '/usr/bin' and NEVER '/sbin' or '/usr/sbin'
On fresh Fedora installs this is fine as the symlinks match the build environment.
On upgraded Fedora installs, however, '/sbin' and '/usr/sbin' remain separate directories and inside they symlink individual binaries, if-and-only-if the binary was in '/sbin' in Fedora 41.
Thus many of the binaries we found in /sbin don't actually exist and libvirt thus breaks.
There is really no reason why we should be harcoding any paths at build time. virCommand happily uses virFindFileInPath for any paths which are not already absolute. $PATH includes '/sbin' and '/usr/sbin' when we're running privileged, and when unprivileged we shouldn't need to run binaries from there.
Does anyone happen to remember the original reason? The only thing I can think of is saving the lookup time, but that has to be negligible compared to the cost of fork and the command actually doing something Jano

On Tue, Apr 29, 2025 at 02:57:13PM +0200, Ján Tomko wrote:
On a Tuesday in 2025, Daniel P. Berrangé via Devel wrote:
Over the years we've made various changes to stop hardcoding paths at build time, but the work is incomplete.
This bit us significantly in Fedora 42 where they have merged the 'sbin' and 'bin' dirs together. In the build environment we have a clean install where '/sbin' and '/usr/sbin' are simply symlinks to '/usr/bin'.
Our meson.build logic preferentially looks in '/sbin' and '/usr/sbin' to find programs, and thus it finds *all* binaries in '/sbin' and then hardcodes this path. This happens even for binaries that have always been in '/usr/bin' and NEVER '/sbin' or '/usr/sbin'
On fresh Fedora installs this is fine as the symlinks match the build environment.
On upgraded Fedora installs, however, '/sbin' and '/usr/sbin' remain separate directories and inside they symlink individual binaries, if-and-only-if the binary was in '/sbin' in Fedora 41.
Thus many of the binaries we found in /sbin don't actually exist and libvirt thus breaks.
There is really no reason why we should be harcoding any paths at build time. virCommand happily uses virFindFileInPath for any paths which are not already absolute. $PATH includes '/sbin' and '/usr/sbin' when we're running privileged, and when unprivileged we shouldn't need to run binaries from there.
Does anyone happen to remember the original reason?
The only thing I can think of is saving the lookup time, but that has to be negligible compared to the cost of fork and the command actually doing something
I've not checked, but I expect that a very very very long time ago virCommand would not lookup binaries in $PATH, and then we just cargo-culted the configure.ac checks, later meson.build checks, without thinking about it more. Yes, we've got the cost of a few stat() calls, but as you say, it isn't worth worrying about that in the context of a heavy fork+exec. With 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 (3)
-
Daniel P. Berrangé
-
Ján Tomko
-
Peter Krempa