4:24 a.m.
My own take on [1], with some additional cleanups on top.
[1] https://listman.redhat.com/archives/libvir-list/2022-March/229697.html
Andrea Bolognani (3):
qemu: Clean up virQEMUCapsFindBinaryForArch()
qemu: Don't assume that /usr/libexec/qemu-kvm exists
qemu: Dissolve virQEMUCapsFindBinaryForArch()
src/qemu/qemu_capabilities.c | 42 +++++++++++++-----------------------
1 file changed, 15 insertions(+), 27 deletions(-)
--
2.35.1
4:24 a.m.
New subject: [libvirt PATCH 1/3] qemu: Clean up virQEMUCapsFindBinaryForArch()
If we get to the bottom of the function we know that none of the
attempts to locate a QEMU binary has been successful, so we can
simply return NULL directly.
This makes it unnecessary variable used to store the path, for
which we can use a more descriptive name.
Lastly, comparing with NULL explicitly is somewhat uncommon in
libvirt and more verbose than the equivalent implicit comparison,
so get rid of it.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/qemu/qemu_capabilities.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 6b4ed08499..68edf94ba5 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -919,7 +919,7 @@ static char *
virQEMUCapsFindBinaryForArch(virArch hostarch,
virArch guestarch)
{
- char *ret = NULL;
+ char *binary;
const char *archstr;
virArch target;
@@ -928,24 +928,24 @@ virQEMUCapsFindBinaryForArch(virArch hostarch,
* to avoid using qemu-system-arm (and thus TCG) instead */
if (hostarch == VIR_ARCH_AARCH64 && guestarch == VIR_ARCH_ARMV7L) {
archstr = virQEMUCapsArchToString(hostarch);
- if ((ret = virQEMUCapsFindBinary("qemu-system-%s", archstr)) != NULL)
- return ret;
+ if ((binary = virQEMUCapsFindBinary("qemu-system-%s", archstr)))
+ return binary;
}
/* First attempt: try the guest architecture as it is */
archstr = virQEMUCapsArchToString(guestarch);
- if ((ret = virQEMUCapsFindBinary("qemu-system-%s", archstr)) != NULL)
- return ret;
+ if ((binary = virQEMUCapsFindBinary("qemu-system-%s", archstr)))
+ return binary;
/* Second attempt: try looking up by target instead */
target = virQEMUCapsFindTarget(hostarch, guestarch);
if (target != guestarch) {
archstr = virQEMUCapsArchToString(target);
- if ((ret = virQEMUCapsFindBinary("qemu-system-%s", archstr)) != NULL)
- return ret;
+ if ((binary = virQEMUCapsFindBinary("qemu-system-%s", archstr)))
+ return binary;
}
- return ret;
+ return NULL;
}
--
2.35.1
4:24 a.m.
New subject: [libvirt PATCH 2/3] qemu: Don't assume that /usr/libexec/qemu-kvm exists
On a machine where no QEMU binary is installed, we end up logging
libvirtd: Cannot check QEMU binary /usr/libexec/qemu-kvm:
No such file or directory
which is not very useful in general, and downright misleading in
the case of operating systems that are not derived from RHEL.
This is a consequence of treating that specific path in a different
way from all other possible QEMU binary paths, and specifically of
not checking whether the file actually exists but sort of assuming
that it must do if we haven't found another QEMU binary earlier.
Address the issue by trying this path out in
virQEMUCapsFindBinaryForArch(), along with all the other possible
ones, and making sure it exists before returning it.
Reported-by: Jim Fehlig <jfehlig(a)suse.com>
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/qemu/qemu_capabilities.c | 20 ++++++++------------
1 file changed, 8 insertions(+), 12 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 68edf94ba5..41e9a3a3f5 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -945,6 +945,13 @@ virQEMUCapsFindBinaryForArch(virArch hostarch,
return binary;
}
+ /* 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)) {
+ if ((binary = virFindFileInPath("/usr/libexec/qemu-kvm")))
+ return binary;
+ }
+
return NULL;
}
@@ -953,18 +960,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);
}
--
2.35.1
4:24 a.m.
New subject: [libvirt PATCH 3/3] qemu: Dissolve virQEMUCapsFindBinaryForArch()
With the recent changes, virQEMUCapsGetDefaultEmulator() has
become a trivial wrapper around this function, as well as its
only caller. Clean up the situation by merging the two.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/qemu/qemu_capabilities.c | 14 +++-----------
1 file changed, 3 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 41e9a3a3f5..25d029d0cc 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -915,9 +915,9 @@ virQEMUCapsFindBinary(const char *format,
return ret;
}
-static char *
-virQEMUCapsFindBinaryForArch(virArch hostarch,
- virArch guestarch)
+char *
+virQEMUCapsGetDefaultEmulator(virArch hostarch,
+ virArch guestarch)
{
char *binary;
const char *archstr;
@@ -956,14 +956,6 @@ virQEMUCapsFindBinaryForArch(virArch hostarch,
}
-char *
-virQEMUCapsGetDefaultEmulator(virArch hostarch,
- virArch guestarch)
-{
- return virQEMUCapsFindBinaryForArch(hostarch, guestarch);
-}
-
-
static int
virQEMUCapsInitGuest(virCaps *caps,
virFileCache *cache,
--
2.35.1
4:27 a.m.
On Thu, Mar 31, 2022 at 11:24:03AM +0200, Andrea Bolognani wrote:
My own take on [1], with some additional cleanups on top.
[1] https://listman.redhat.com/archives/libvir-list/2022-March/229697.html
Andrea Bolognani (3):
qemu: Clean up virQEMUCapsFindBinaryForArch()
qemu: Don't assume that /usr/libexec/qemu-kvm exists
qemu: Dissolve virQEMUCapsFindBinaryForArch()
src/qemu/qemu_capabilities.c | 42 +++++++++++++-----------------------
1 file changed, 15 insertions(+), 27 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange(a)redhat.com>
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 :|
6:55 a.m.
On Thu, Mar 31, 2022 at 10:27:07AM +0100, Daniel P. Berrangé wrote:
On Thu, Mar 31, 2022 at 11:24:03AM +0200, Andrea Bolognani wrote:
> My own take on [1], with some additional cleanups on top.
>
> [1] https://listman.redhat.com/archives/libvir-list/2022-March/229697.html
>
> Andrea Bolognani (3):
> qemu: Clean up virQEMUCapsFindBinaryForArch()
> qemu: Don't assume that /usr/libexec/qemu-kvm exists
> qemu: Dissolve virQEMUCapsFindBinaryForArch()
>
> src/qemu/qemu_capabilities.c | 42 +++++++++++++-----------------------
> 1 file changed, 15 insertions(+), 27 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange(a)redhat.com>
Thanks! I'll wait for Jim to confirm that the series addresses the
issue he reported (and for the freeze to be over ;) before pushing.
--
Andrea Bolognani / Red Hat / Virtualization
3:48 p.m.
On 3/31/22 05:55, Andrea Bolognani wrote:
On Thu, Mar 31, 2022 at 10:27:07AM +0100, Daniel P. Berrangé wrote:
> On Thu, Mar 31, 2022 at 11:24:03AM +0200, Andrea Bolognani wrote:
>> My own take on [1], with some additional cleanups on top.
>>
>> [1] https://listman.redhat.com/archives/libvir-list/2022-March/229697.html
>>
>> Andrea Bolognani (3):
>> qemu: Clean up virQEMUCapsFindBinaryForArch()
>> qemu: Don't assume that /usr/libexec/qemu-kvm exists
>> qemu: Dissolve virQEMUCapsFindBinaryForArch()
>>
>> src/qemu/qemu_capabilities.c | 42 +++++++++++++-----------------------
>> 1 file changed, 15 insertions(+), 27 deletions(-)
>
> Reviewed-by: Daniel P. Berrangé <berrange(a)redhat.com>
Thanks! I'll wait for Jim to confirm that the series addresses the
issue he reported (and for the freeze to be over ;) before pushing.
I also reviewed the series, but you are probably more interested to know it
solved the problem for me :-).
Tested-by: Jim Fehlig <jfehlig(a)suse.com>
Regards,
Jim
4:35 a.m.
On Thu, Mar 31, 2022 at 02:48:52PM -0600, Jim Fehlig wrote:
On 3/31/22 05:55, Andrea Bolognani wrote:
> Thanks! I'll wait for Jim to confirm that the series addresses the
> issue he reported (and for the freeze to be over ;) before pushing.
I also reviewed the series, but you are probably more interested to know it
solved the problem for me :-).
Tested-by: Jim Fehlig <jfehlig(a)suse.com>
Great, that's what I was hoping to hear!
If you reviewed the series, do you want me to add your Reviewed-by
too? One can be both tester *and* reviewer :)
--
Andrea Bolognani / Red Hat / Virtualization
8:53 a.m.
On 4/1/22 03:35, Andrea Bolognani wrote:
On Thu, Mar 31, 2022 at 02:48:52PM -0600, Jim Fehlig wrote:
> On 3/31/22 05:55, Andrea Bolognani wrote:
>> Thanks! I'll wait for Jim to confirm that the series addresses the
>> issue he reported (and for the freeze to be over ;) before pushing.
>
> I also reviewed the series, but you are probably more interested to know it
> solved the problem for me :-).
>
> Tested-by: Jim Fehlig <jfehlig(a)suse.com>
Great, that's what I was hoping to hear!
If you reviewed the series, do you want me to add your Reviewed-by
too? One can be both tester *and* reviewer :)
Nah, T-b is fine. Thanks!
960
days inactive
961
days old
8 comments
3 participants
participants (3)
-
Andrea Bolognani -
Daniel P. Berrangé -
Jim Fehlig