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@xxxxxxxxx> > > > > 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@xxxxxxxxxx> > > Signed-off-by: Roman Bolshakov <roolebo@xxxxxxxxx> > > 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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
