On Wed, Nov 21, 2018 at 05:04:07PM +0100, Jiri Denemark wrote:
On Wed, Nov 21, 2018 at 17:01:44 +0300, Roman Bolshakov wrote:
From: Roman Bolshakov <roolebo@gmail.com>
virQEMUCapsFormatCache/virQEMUCapsLoadCache adds/reads KVM CPUs to/from capabilities cache regardless of QEMU_CAPS_KVM. That can cause undesired side-effects when KVM CPUs are present in the cache on a platform that doesn't support it, e.g. macOS or Linux without KVM support.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> Signed-off-by: Roman Bolshakov <roolebo@gmail.com>
This doesn't look like a patch written by Daniel so why did you include the Signed-off-by line? Or did I miss anything?
Daniel kindly helped to root cause an issue I had with qemucapabilitiestest in v1: https://www.redhat.com/archives/libvir-list/2018-November/msg00740.html and provided a diff that resolves the issue: https://www.redhat.com/archives/libvir-list/2018-November/msg00767.html Should I remove his Signed-off-by tag?
--- src/qemu/qemu_capabilities.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index fde27010e4..4ba8369e3a 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -3467,11 +3467,13 @@ virQEMUCapsLoadCache(virArch hostArch, } VIR_FREE(str);
- if (virQEMUCapsLoadHostCPUModelInfo(qemuCaps, ctxt, VIR_DOMAIN_VIRT_KVM) < 0 || + if ((virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM) && + virQEMUCapsLoadHostCPUModelInfo(qemuCaps, ctxt, VIR_DOMAIN_VIRT_KVM) < 0) || virQEMUCapsLoadHostCPUModelInfo(qemuCaps, ctxt, VIR_DOMAIN_VIRT_QEMU) < 0) goto cleanup;
I don't think we should introduce these guards in all the places. All the loading and formatting functions should return success if the appropriate info is not available, so you should just make sure the relevant info is NULL in qemuCaps.
Do you mean the capabilities checks should be moved inside the functions? Either way they're needed to avoid loading KVM cpus into QEMU caps cache on the hosts without KVM support.
@@ -3584,7 +3586,8 @@ virQEMUCapsLoadCache(virArch hostArch, if (virQEMUCapsParseSEVInfo(qemuCaps, ctxt) < 0) goto cleanup;
- virQEMUCapsInitHostCPUModel(qemuCaps, hostArch, VIR_DOMAIN_VIRT_KVM); + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM)) + virQEMUCapsInitHostCPUModel(qemuCaps, hostArch, VIR_DOMAIN_VIRT_KVM);
Please, follow our coding style, i.e., indent by 4 spaces.
virQEMUCapsInitHostCPUModel(qemuCaps, hostArch, VIR_DOMAIN_VIRT_QEMU);
ret = 0;
...
Will do, thank you for catching this! Best regards, Roman