Re: [libvirt] [PATCHv2 01/16] qemu: Add KVM CPUs into cache only if KVM is present

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.