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