[PATCH] qemu: capabilities: Remove check for /usr/libexec/qemu-kvm
A downstream packaging bug resulted in a scenario where no aarch64 emulator binary was installed on a kvm host. Querying capabilities on the host succeeds and the capabilities correctly report that no <guest>'s are supported, but the following error is logged libvirtd: Cannot check QEMU binary /usr/libexec/qemu-kvm: No such file or directory This error is confusing and not very helpful. Additionally, comments in the associated code note that /usr/libexec/qemu-kvm is disto-specific, which suggests the logic is better suited for a downstream patch. Removing the check for /usr/libexec/qemu-kvm leaves virQEMUCapsGetDefaultEmulator() as nothing more than a needless wrapper around virQEMUCapsFindBinaryForArch. Drop virQEMUCapsGetDefaultEmulator() and call virQEMUCapsFindBinaryForArch() directly in its place, which squelches the unhelpful error. Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/qemu/qemu_capabilities.c | 25 +++---------------------- src/qemu/qemu_capabilities.h | 5 +++-- src/qemu/qemu_domain.c | 2 +- 3 files changed, 7 insertions(+), 25 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 6b4ed08499..c866c5acf6 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -915,7 +915,7 @@ virQEMUCapsFindBinary(const char *format, return ret; } -static char * +char * virQEMUCapsFindBinaryForArch(virArch hostarch, virArch guestarch) { @@ -949,25 +949,6 @@ virQEMUCapsFindBinaryForArch(virArch hostarch, } -char * -virQEMUCapsGetDefaultEmulator(virArch hostarch, - virArch guestarch) -{ - char *binary = NULL; - /* Check for existence of base emulator, or alternate base - * which can be used with magic cpu choice - */ - binary = virQEMUCapsFindBinaryForArch(hostarch, guestarch); - - /* RHEL doesn't follow the usual naming for QEMU binaries and ships - * a single binary named qemu-kvm outside of $PATH instead */ - if (virQEMUCapsGuestIsNative(hostarch, guestarch) && !binary) - binary = g_strdup("/usr/libexec/qemu-kvm"); - - return binary; -} - - static int virQEMUCapsInitGuest(virCaps *caps, virFileCache *cache, @@ -978,7 +959,7 @@ virQEMUCapsInitGuest(virCaps *caps, virQEMUCaps *qemuCaps = NULL; int ret = -1; - binary = virQEMUCapsGetDefaultEmulator(hostarch, guestarch); + binary = virQEMUCapsFindBinaryForArch(hostarch, guestarch); /* Ignore binary if extracting version info fails */ if (binary) { @@ -5878,7 +5859,7 @@ virQEMUCapsCacheLookupDefault(virFileCache *cache, } if (!binary) { - probedbinary = virQEMUCapsGetDefaultEmulator(hostarch, arch); + probedbinary = virQEMUCapsFindBinaryForArch(hostarch, arch); binary = probedbinary; } if (!binary) { diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 948029d60d..d0e776a5c4 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -752,8 +752,6 @@ const char *virQEMUCapsGetMachineDefaultRAMid(virQEMUCaps *qemuCaps, void virQEMUCapsFilterByMachineType(virQEMUCaps *qemuCaps, virDomainVirtType virtType, const char *machineType); -char * virQEMUCapsGetDefaultEmulator(virArch hostarch, - virArch guestarch); virFileCache *virQEMUCapsCacheNew(const char *libDir, const char *cacheDir, @@ -789,6 +787,9 @@ bool virQEMUCapsSupportsGICVersion(virQEMUCaps *qemuCaps, const char *virQEMUCapsGetPreferredMachine(virQEMUCaps *qemuCaps, virDomainVirtType virtType); +char *virQEMUCapsFindBinaryForArch(virArch hostarch, + virArch guestarch); + int virQEMUCapsInitGuestFromBinary(virCaps *caps, const char *binary, virQEMUCaps *qemuCaps, diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 18d403e099..81b56b4233 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4607,7 +4607,7 @@ qemuDomainDefPostParseBasic(virDomainDef *def, /* check for emulator and create a default one if needed */ if (!def->emulator) { - if (!(def->emulator = virQEMUCapsGetDefaultEmulator( + if (!(def->emulator = virQEMUCapsFindBinaryForArch( driver->hostarch, def->os.arch))) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("No emulator found for arch '%s'"), -- 2.35.1
On Mon, Mar 28, 2022 at 02:38:38PM -0600, Jim Fehlig wrote:
A downstream packaging bug resulted in a scenario where no aarch64 emulator binary was installed on a kvm host. Querying capabilities on the host succeeds and the capabilities correctly report that no <guest>'s are supported, but the following error is logged
libvirtd: Cannot check QEMU binary /usr/libexec/qemu-kvm: No such file or directory
This error is confusing and not very helpful. Additionally, comments in the associated code note that /usr/libexec/qemu-kvm is disto-specific, which suggests the logic is better suited for a downstream patch. Removing the check for /usr/libexec/qemu-kvm leaves virQEMUCapsGetDefaultEmulator() as nothing more than a needless wrapper around virQEMUCapsFindBinaryForArch. Drop virQEMUCapsGetDefaultEmulator() and call virQEMUCapsFindBinaryForArch() directly in its place, which squelches the unhelpful error.
I agree that the message being logged is not very useful, but I don't think the approach you take here is the correct one: we want upstream libvirt to work out of the box when built on a variety of distros, including RHEL and derivatives, and your patch breaks that. I think the diff you're looking for is more along the lines of the (completely untested) one below. You can then rename virQEMUCapsFindBinaryForArch() to virQEMUCapsGetDefaultEmulator() in a second patch to avoid leaving unnecessary wrappers around. diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 6b4ed08499..e35a89f944 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -945,6 +945,12 @@ virQEMUCapsFindBinaryForArch(virArch hostarch, return ret; } + if (virQEMUCapsGuestIsNative(hostarch, guestarch)) { + if ((ret = virFindFileInPath("/usr/libexec/qemu-kvm")) != NULL) { + return ret; + } + } + return ret; } @@ -953,18 +959,7 @@ char * virQEMUCapsGetDefaultEmulator(virArch hostarch, virArch guestarch) { - char *binary = NULL; - /* Check for existence of base emulator, or alternate base - * which can be used with magic cpu choice - */ - binary = virQEMUCapsFindBinaryForArch(hostarch, guestarch); - - /* RHEL doesn't follow the usual naming for QEMU binaries and ships - * a single binary named qemu-kvm outside of $PATH instead */ - if (virQEMUCapsGuestIsNative(hostarch, guestarch) && !binary) - binary = g_strdup("/usr/libexec/qemu-kvm"); - - return binary; + return virQEMUCapsFindBinaryForArch(hostarch, guestarch); } -- Andrea Bolognani / Red Hat / Virtualization
On Wed, Mar 30, 2022 at 09:59:11AM -0700, Andrea Bolognani wrote:
On Mon, Mar 28, 2022 at 02:38:38PM -0600, Jim Fehlig wrote:
A downstream packaging bug resulted in a scenario where no aarch64 emulator binary was installed on a kvm host. Querying capabilities on the host succeeds and the capabilities correctly report that no <guest>'s are supported, but the following error is logged
libvirtd: Cannot check QEMU binary /usr/libexec/qemu-kvm: No such file or directory
This error is confusing and not very helpful. Additionally, comments in the associated code note that /usr/libexec/qemu-kvm is disto-specific, which suggests the logic is better suited for a downstream patch. Removing the check for /usr/libexec/qemu-kvm leaves virQEMUCapsGetDefaultEmulator() as nothing more than a needless wrapper around virQEMUCapsFindBinaryForArch. Drop virQEMUCapsGetDefaultEmulator() and call virQEMUCapsFindBinaryForArch() directly in its place, which squelches the unhelpful error.
I agree that the message being logged is not very useful, but I don't think the approach you take here is the correct one: we want upstream libvirt to work out of the box when built on a variety of distros, including RHEL and derivatives, and your patch breaks that.
I think the diff you're looking for is more along the lines of the (completely untested) one below. You can then rename virQEMUCapsFindBinaryForArch() to virQEMUCapsGetDefaultEmulator() in a second patch to avoid leaving unnecessary wrappers around.
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 6b4ed08499..e35a89f944 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -945,6 +945,12 @@ virQEMUCapsFindBinaryForArch(virArch hostarch, return ret; }
+ if (virQEMUCapsGuestIsNative(hostarch, guestarch)) { + if ((ret = virFindFileInPath("/usr/libexec/qemu-kvm")) != NULL) { + return ret; + } + } + return ret; }
That overwrites the existing 'ret' value that we want to keep when qemu-kvm isn't present. Needs if (!ret && virQEMUCapsGuestIsNative(hostarch, guestarch)) { ... 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 Wed, Mar 30, 2022 at 06:13:58PM +0100, Daniel P. Berrangé wrote:
On Wed, Mar 30, 2022 at 09:59:11AM -0700, Andrea Bolognani wrote:
+ if (virQEMUCapsGuestIsNative(hostarch, guestarch)) { + if ((ret = virFindFileInPath("/usr/libexec/qemu-kvm")) != NULL) { + return ret; + } + } + return ret; }
That overwrites the existing 'ret' value that we want to keep when qemu-kvm isn't present. Needs
if (!ret && virQEMUCapsGuestIsNative(hostarch, guestarch)) { ...
Does it? All previous attempts immediately return ret if it's not NULL, so if we get to this point we know that there's no existing value to preserve. -- Andrea Bolognani / Red Hat / Virtualization
On Wed, Mar 30, 2022 at 10:39:09AM -0700, Andrea Bolognani wrote:
On Wed, Mar 30, 2022 at 06:13:58PM +0100, Daniel P. Berrangé wrote:
On Wed, Mar 30, 2022 at 09:59:11AM -0700, Andrea Bolognani wrote:
+ if (virQEMUCapsGuestIsNative(hostarch, guestarch)) { + if ((ret = virFindFileInPath("/usr/libexec/qemu-kvm")) != NULL) { + return ret; + } + } + return ret; }
That overwrites the existing 'ret' value that we want to keep when qemu-kvm isn't present. Needs
if (!ret && virQEMUCapsGuestIsNative(hostarch, guestarch)) { ...
Does it? All previous attempts immediately return ret if it's not NULL, so if we get to this point we know that there's no existing value to preserve.
Honestly I didn't look at the earlier context, just assumed that 'return ret' was meaningful. If 'ret' is always NULL, then we should be writing 'return NULL' rather than 'return ret' to make it clear we've not expected any value here. 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 Thu, Mar 31, 2022 at 09:37:42AM +0100, Daniel P. Berrangé wrote:
On Wed, Mar 30, 2022 at 10:39:09AM -0700, Andrea Bolognani wrote:
On Wed, Mar 30, 2022 at 06:13:58PM +0100, Daniel P. Berrangé wrote:
On Wed, Mar 30, 2022 at 09:59:11AM -0700, Andrea Bolognani wrote:
+ if (virQEMUCapsGuestIsNative(hostarch, guestarch)) { + if ((ret = virFindFileInPath("/usr/libexec/qemu-kvm")) != NULL) { + return ret; + } + } + return ret; }
That overwrites the existing 'ret' value that we want to keep when qemu-kvm isn't present. Needs
if (!ret && virQEMUCapsGuestIsNative(hostarch, guestarch)) { ...
Does it? All previous attempts immediately return ret if it's not NULL, so if we get to this point we know that there's no existing value to preserve.
Honestly I didn't look at the earlier context, just assumed that 'return ret' was meaningful. If 'ret' is always NULL, then we should be writing 'return NULL' rather than 'return ret' to make it clear we've not expected any value here.
Totally agree, but it should be a separate patch. Let me just prepare and post it right now :) -- Andrea Bolognani / Red Hat / Virtualization
On 3/30/22 10:59, Andrea Bolognani wrote:
On Mon, Mar 28, 2022 at 02:38:38PM -0600, Jim Fehlig wrote:
A downstream packaging bug resulted in a scenario where no aarch64 emulator binary was installed on a kvm host. Querying capabilities on the host succeeds and the capabilities correctly report that no <guest>'s are supported, but the following error is logged
libvirtd: Cannot check QEMU binary /usr/libexec/qemu-kvm: No such file or directory
This error is confusing and not very helpful. Additionally, comments in the associated code note that /usr/libexec/qemu-kvm is disto-specific, which suggests the logic is better suited for a downstream patch. Removing the check for /usr/libexec/qemu-kvm leaves virQEMUCapsGetDefaultEmulator() as nothing more than a needless wrapper around virQEMUCapsFindBinaryForArch. Drop virQEMUCapsGetDefaultEmulator() and call virQEMUCapsFindBinaryForArch() directly in its place, which squelches the unhelpful error.
I agree that the message being logged is not very useful, but I don't think the approach you take here is the correct one: we want upstream libvirt to work out of the box when built on a variety of distros, including RHEL and derivatives, and your patch breaks that.
Do the minimum RHEL and derivatives supported upstream still provide /usr/libexec/qemu-kvm? Regards, Jim
On Fri, Apr 01, 2022 at 03:52:34PM -0600, Jim Fehlig wrote:
On 3/30/22 10:59, Andrea Bolognani wrote:
I agree that the message being logged is not very useful, but I don't think the approach you take here is the correct one: we want upstream libvirt to work out of the box when built on a variety of distros, including RHEL and derivatives, and your patch breaks that.
Do the minimum RHEL and derivatives supported upstream still provide /usr/libexec/qemu-kvm?
Yup. And the path that other distros as well as upstream QEMU use doesn't exist, so we are forced to have this special handling. $ podman run --rm quay.io/centos/centos:stream8 \ sh -c 'yum install -y qemu-kvm-core && rpm -ql qemu-kvm-core' 2>&1 \ | grep -E '/usr/libexec/qemu-kvm|/usr/bin/qemu-system-x86_64' /usr/libexec/qemu-kvm $ podman run --rm quay.io/centos/centos:stream9 \ sh -c 'yum install -y qemu-kvm-core && rpm -ql qemu-kvm-core' 2>&1 \ | grep -E '/usr/libexec/qemu-kvm|/usr/bin/qemu-system-x86_64' /usr/libexec/qemu-kvm Luckily, now that thanks to your report I've cleaned things up it's not actually that bad :) -- Andrea Bolognani / Red Hat / Virtualization
participants (3)
-
Andrea Bolognani -
Daniel P. Berrangé -
Jim Fehlig