[libvirt PATCH 0/5] qemu: Find helpers at runtime
This removes the need to have them present in the build environment and makes things more flexible. Note that we currently *do not* have the helpers available in most CI environments, or have BuildRequires for them in the spec file. That only works for Fedora and RHEL because the hardcoded fallback paths happen to match those used on those distributions: everywhere else, the choice is to either ensure that the additional packages are installed in the build environment or to produce a build of libvirt that can't use the corresponding features out of the box. Andrea Bolognani (5): util: Small refactor util: Introduce virFileFindInPathFull() qemu: Find helpers at runtime meson: Stop looking for QEMU helpers qemu: Update documentation for qemu.conf keys meson.build | 24 --------------- src/libvirt_private.syms | 1 + src/qemu/qemu.conf.in | 8 +++-- src/qemu/qemu_conf.c | 4 +++ src/qemu/qemu_interface.c | 15 ++++++++-- src/qemu/qemu_process.c | 17 ++++++++--- src/qemu/test_libvirtd_qemu.aug.in | 4 +-- src/util/virfile.c | 47 ++++++++++++++++++++++++++---- src/util/virfile.h | 3 ++ 9 files changed, 83 insertions(+), 40 deletions(-) -- 2.40.1
Prepare for further changes. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/util/virfile.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index 228482e8f8..8e94d19e45 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -1743,13 +1743,15 @@ virFindFileInPath(const char *file) return NULL; path = g_find_program_in_path(file); - if (!path) - return NULL; - /* Workaround for a bug in g_find_program_in_path() not returning absolute - * path as documented. TODO drop it once we require GLib >= 2.69.0 - */ - return g_canonicalize_filename(path, NULL); + if (path) { + /* Workaround for a bug in g_find_program_in_path() not returning absolute + * path as documented. TODO drop it once we require GLib >= 2.69.0 + */ + return g_canonicalize_filename(path, NULL); + } + + return NULL; } -- 2.40.1
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virfile.c | 33 +++++++++++++++++++++++++++++++++ src/util/virfile.h | 3 +++ 3 files changed, 37 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 62c296bf5f..27b8e111aa 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2358,6 +2358,7 @@ virFileWrapperFdFree; virFileWrapperFdNew; virFileWriteStr; virFindFileInPath; +virFindFileInPathFull; # util/virfilecache.h diff --git a/src/util/virfile.c b/src/util/virfile.c index 8e94d19e45..6b4d4c3522 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -1737,6 +1737,25 @@ virFileIsLink(const char *linkpath) */ char * virFindFileInPath(const char *file) +{ + return virFindFileInPathFull(file, NULL); +} + +/* virFindFileInPathFull: + * @file: name of the program + * @extraDirs: NULL-terminated list of additional directories + * + * Like virFindFileInPath(), but in addition to searching $PATH also + * looks into all directories listed in @extraDirs. This is useful to + * locate helpers that are installed outside of $PATH. + * + * The returned path must be freed by the caller. + * + * Returns: absolute path of the program or NULL + */ +char * +virFindFileInPathFull(const char *file, + const char *const *extraDirs) { g_autofree char *path = NULL; if (file == NULL) @@ -1751,6 +1770,20 @@ virFindFileInPath(const char *file) return g_canonicalize_filename(path, NULL); } + if (extraDirs) { + while (*extraDirs) { + g_autofree char *extraPath = NULL; + + extraPath = g_strdup_printf("%s/%s", *extraDirs, file); + + if (virFileIsExecutable(extraPath)) { + return g_steal_pointer(&extraPath); + } + + extraDirs++; + } + } + return NULL; } diff --git a/src/util/virfile.h b/src/util/virfile.h index f7a31d9f57..6a14173625 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -189,6 +189,9 @@ int virFileIsLink(const char *linkpath) char *virFindFileInPath(const char *file) G_NO_INLINE; +char *virFindFileInPathFull(const char *file, + const char *const *extraDirs) + G_NO_INLINE; char *virFileFindResource(const char *filename, const char *builddir, -- 2.40.1
Use the recently introduced virFindFileInPathFull() function to discover the path for qemu-bridge-helper and qemu-pr-helper at runtime. Note that it's still possible for the administrator to prevent this lookup and use arbitrary binaries by setting the appropriate keys in qemu.conf: this simply removes the need to perform the lookup at build time, and thus to have the helpers installed in the build environment. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_interface.c | 15 +++++++++++++-- src/qemu/qemu_process.c | 17 +++++++++++++---- 2 files changed, 26 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_interface.c b/src/qemu/qemu_interface.c index e395bfcc5b..e875de48ee 100644 --- a/src/qemu/qemu_interface.c +++ b/src/qemu/qemu_interface.c @@ -327,7 +327,14 @@ qemuCreateInBridgePortWithHelper(virQEMUDriverConfig *cfg, int *tapfd, unsigned int flags) { + const char *const bridgeHelperDirs[] = { + "/usr/libexec", + "/usr/lib/qemu", + "/usr/lib", + NULL, + }; g_autoptr(virCommand) cmd = NULL; + g_autofree char *bridgeHelperPath = NULL; char *errbuf = NULL, *cmdstr = NULL; int pair[2] = { -1, -1 }; @@ -339,13 +346,17 @@ qemuCreateInBridgePortWithHelper(virQEMUDriverConfig *cfg, return -1; } - if (!virFileIsExecutable(cfg->bridgeHelperName)) { + bridgeHelperPath = virFindFileInPathFull(cfg->bridgeHelperName, bridgeHelperDirs); + + if (!bridgeHelperPath) { virReportSystemError(errno, _("'%1$s' is not a suitable bridge helper"), cfg->bridgeHelperName); return -1; } - cmd = virCommandNew(cfg->bridgeHelperName); + VIR_DEBUG("Using qemu-bridge-helper: %s", bridgeHelperPath); + + cmd = virCommandNew(bridgeHelperPath); if (flags & VIR_NETDEV_TAP_CREATE_VNET_HDR) virCommandAddArgFormat(cmd, "--use-vnet"); virCommandAddArgFormat(cmd, "--br=%s", brname); diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 952814d663..bb4cf35226 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2818,10 +2818,15 @@ qemuProcessStartPRDaemonHook(void *opaque) int qemuProcessStartManagedPRDaemon(virDomainObj *vm) { + const char *const prHelperDirs[] = { + "/usr/libexec", + NULL, + }; qemuDomainObjPrivate *priv = vm->privateData; virQEMUDriver *driver = priv->driver; g_autoptr(virQEMUDriverConfig) cfg = NULL; int errfd = -1; + g_autofree char *prHelperPath = NULL; g_autofree char *pidfile = NULL; g_autofree char *socketPath = NULL; pid_t cpid = -1; @@ -2832,12 +2837,16 @@ qemuProcessStartManagedPRDaemon(virDomainObj *vm) cfg = virQEMUDriverGetConfig(driver); - if (!virFileIsExecutable(cfg->prHelperName)) { + prHelperPath = virFindFileInPathFull(cfg->prHelperName, prHelperDirs); + + if (!prHelperPath) { virReportSystemError(errno, _("'%1$s' is not a suitable pr helper"), cfg->prHelperName); goto cleanup; } + VIR_DEBUG("Using qemu-pr-helper: %s", prHelperPath); + if (!(pidfile = qemuProcessBuildPRHelperPidfilePath(vm))) goto cleanup; @@ -2853,7 +2862,7 @@ qemuProcessStartManagedPRDaemon(virDomainObj *vm) goto cleanup; } - if (!(cmd = virCommandNewArgList(cfg->prHelperName, + if (!(cmd = virCommandNewArgList(prHelperPath, "-k", socketPath, NULL))) goto cleanup; @@ -2881,7 +2890,7 @@ qemuProcessStartManagedPRDaemon(virDomainObj *vm) if (virPidFileReadPath(pidfile, &cpid) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("pr helper %1$s didn't show up"), - cfg->prHelperName); + prHelperPath); goto cleanup; } @@ -2899,7 +2908,7 @@ qemuProcessStartManagedPRDaemon(virDomainObj *vm) if (saferead(errfd, errbuf, sizeof(errbuf) - 1) < 0) { virReportSystemError(errno, _("pr helper %1$s died unexpectedly"), - cfg->prHelperName); + prHelperPath); } else { virReportError(VIR_ERR_OPERATION_FAILED, _("pr helper died and reported: %1$s"), errbuf); -- 2.40.1
Now that we're performing the lookup at runtime, doing it at build time is no longer necessary. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- meson.build | 24 ------------------------ src/qemu/qemu_conf.c | 4 ++++ 2 files changed, 4 insertions(+), 24 deletions(-) diff --git a/meson.build b/meson.build index 24f52113fa..6e65e9eec3 100644 --- a/meson.build +++ b/meson.build @@ -1655,30 +1655,6 @@ if not get_option('driver_qemu').disabled() conf.set_quoted('QEMU_USER', qemu_user) conf.set_quoted('QEMU_GROUP', qemu_group) - qemu_bridge_prog = find_program( - 'qemu-bridge-helper', - dirs: [ '/usr/libexec', '/usr/lib/qemu', '/usr/lib' ], - required: false - ) - if qemu_bridge_prog.found() - qemu_bridge_path = qemu_bridge_prog.full_path() - else - qemu_bridge_path = '/usr/libexec/qemu-bridge-helper' - endif - conf.set_quoted('QEMU_BRIDGE_HELPER', qemu_bridge_path) - - qemu_pr_prog = find_program( - 'qemu-pr-helper', - dirs: [ '/usr/bin', '/usr/libexec' ], - required: false - ) - if qemu_pr_prog.found() - qemu_pr_path = qemu_pr_prog.full_path() - else - qemu_pr_path = '/usr/bin/qemu-pr-helper' - endif - conf.set_quoted('QEMU_PR_HELPER', qemu_pr_path) - qemu_slirp_prog = find_program( 'slirp-helper', dirs: [ '/usr/bin', '/usr/libexec' ], diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index d9460d82e0..c76ae7ac93 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -97,6 +97,10 @@ VIR_ONCE_GLOBAL_INIT(virQEMUConfig); #endif +#define QEMU_BRIDGE_HELPER "qemu-bridge-helper" +#define QEMU_PR_HELPER "qemu-pr-helper" + + virQEMUDriverConfig *virQEMUDriverConfigNew(bool privileged, const char *root) { -- 2.40.1
Reflect the new default value, and explain that a runtime lookup will be performed if the value is not an absolute path. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu.conf.in | 8 ++++++-- src/qemu/test_libvirtd_qemu.aug.in | 4 ++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu.conf.in b/src/qemu/qemu.conf.in index 3895d42514..e1a9a5e56d 100644 --- a/src/qemu/qemu.conf.in +++ b/src/qemu/qemu.conf.in @@ -650,7 +650,9 @@ # is used to create <source type='bridge'> interfaces when libvirtd is # running unprivileged. libvirt invokes the helper directly, instead # of using "-netdev bridge", for security reasons. -#bridge_helper = "/usr/libexec/qemu-bridge-helper" +# If this is not an absolute path, the program will be searched for +# in $PATH as well as a few additional directories. +#bridge_helper = "qemu-bridge-helper" # If enabled, libvirt will have QEMU set its process name to @@ -899,7 +901,9 @@ # Path to the SCSI persistent reservations helper. This helper is # used whenever <reservations/> are enabled for SCSI LUN devices. -#pr_helper = "/usr/bin/qemu-pr-helper" +# If this is not an absolute path, the program will be searched for +# in $PATH as well as a few additional directories. +#pr_helper = "qemu-pr-helper" # Path to the SLIRP networking helper. #slirp_helper = "/usr/bin/slirp-helper" diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qemu.aug.in index 1dbd692921..af99331886 100644 --- a/src/qemu/test_libvirtd_qemu.aug.in +++ b/src/qemu/test_libvirtd_qemu.aug.in @@ -75,7 +75,7 @@ module Test_libvirtd_qemu = { "auto_dump_bypass_cache" = "0" } { "auto_start_bypass_cache" = "0" } { "hugetlbfs_mount" = "/dev/hugepages" } -{ "bridge_helper" = "/usr/libexec/qemu-bridge-helper" } +{ "bridge_helper" = "qemu-bridge-helper" } { "set_process_name" = "1" } { "max_processes" = "0" } { "max_files" = "0" } @@ -107,7 +107,7 @@ module Test_libvirtd_qemu = { "1" = "mount" } } { "memory_backing_dir" = "/var/lib/libvirt/qemu/ram" } -{ "pr_helper" = "/usr/bin/qemu-pr-helper" } +{ "pr_helper" = "qemu-pr-helper" } { "slirp_helper" = "/usr/bin/slirp-helper" } { "dbus_daemon" = "/usr/bin/dbus-daemon" } { "swtpm_user" = "tss" } -- 2.40.1
On Fri, May 05, 2023 at 08:01:41PM +0200, Andrea Bolognani wrote:
This removes the need to have them present in the build environment and makes things more flexible.
Note that we currently *do not* have the helpers available in most CI environments, or have BuildRequires for them in the spec file. That only works for Fedora and RHEL because the hardcoded fallback paths happen to match those used on those distributions: everywhere else, the choice is to either ensure that the additional packages are installed in the build environment or to produce a build of libvirt that can't use the corresponding features out of the box.
Andrea Bolognani (5): util: Small refactor util: Introduce virFileFindInPathFull() qemu: Find helpers at runtime meson: Stop looking for QEMU helpers qemu: Update documentation for qemu.conf keys
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
meson.build | 24 --------------- src/libvirt_private.syms | 1 + src/qemu/qemu.conf.in | 8 +++-- src/qemu/qemu_conf.c | 4 +++ src/qemu/qemu_interface.c | 15 ++++++++-- src/qemu/qemu_process.c | 17 ++++++++--- src/qemu/test_libvirtd_qemu.aug.in | 4 +-- src/util/virfile.c | 47 ++++++++++++++++++++++++++---- src/util/virfile.h | 3 ++ 9 files changed, 83 insertions(+), 40 deletions(-)
-- 2.40.1
participants (2)
-
Andrea Bolognani -
Martin Kletzander