On Fri, Nov 23, 2018 at 17:16:12 +0300, Roman Bolshakov wrote:
> 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.
And how does different name change the behavior?
> 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.
It won't return anything because the code will make sure accelCPUModels
and accelCPU will be NULL when no accel method is supported.
Jirka
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
