On Wed, Nov 21, 2018 at 07:43:43PM +0100, Jiri Denemark wrote:
> 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:
> > > > 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;
>
I agree, virQEMUCapsLoadHostCPUModelInfo and virQEMUCapsLoadCPUModels
don't need the check.
> 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.
Unfortunately, that won't work for the patch series. kvmCPUModels is renamed to
accelCPUModels and kvmCPU is renamed to accelCPU in PATCH 6.
So, virQEMUCapsFormatHostCPUModelInfo looks like:
if (virQEMUCapsTypeIsAccelerated(type))
cpuData = qemuCaps->accelCPU;
else
cpuData = qemuCaps->tcgCPU;
and virQEMUCapsFormatCPUModels looks like:
if (virQEMUCapsTypeIsAccelerated(type))
cpus = qemuCaps->accelCPUModels;
else
cpus = qemuCaps->tcgCPUModels;
Without the check we'd return CPUs for KVM domain on the platform that doesn't
support it.
Thank you,
Roman
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
