Re: [libvirt] [PATCHv2 01/16] qemu: Add KVM CPUs into cache only if KVM is present
On Wed, Nov 21, 2018 at 20:50:50 +0300, Roman Bolshakov wrote:
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(a)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(a)redhat.com>
> > Signed-off-by: Roman Bolshakov <roolebo(a)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
I see, I missed the diff.
Should I remove his Signed-off-by tag?
Dunno, I guess it's up to Daniel. But if the final patch is going to
look very differently anyway, I don't see a reason to keep the 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?
virQEMUCapsLoadHostCPUModelInfo does (not literally, but effectively)
hostCPUNode = virXPathNode("./hostCPU[@type='kvm']", ctxt);
if (!hostCPUNode)
return 0;
virQEMUCapsLoadCPUModels does
n = virXPathNodeSet("./cpu[@type='kvm']", ctxt, &nodes);
if (n == 0)
return 0;
virQEMUCapsInitHostCPUModel always fills in something and your check
should probably remain in place for it
virQEMUCapsFormatHostCPUModelInfo does
virQEMUCapsHostCPUDataPtr cpuData = &qemuCaps->kvmCPU;
qemuMonitorCPUModelInfoPtr model = cpuData->info;
if (!model)
return;
virQEMUCapsFormatCPUModels
cpus = qemuCaps->kvmCPUModels;
if (!cpus)
return;
So to me it looks like all functions are ready to see NULL pointers and
just do nothing if that's the case. Thus the only thing this patch
should need to do is to make sure virQEMUCapsInitHostCPUModel does not
set something non-NULL there.
Jirka