On 03/31/2017 01:54 PM, Jiri Denemark wrote:
We will need to store two more host CPU models and nested structs look better than separate items with long complicated names.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_capabilities.c | 65 ++++++++++++++++++++++++++------------------ 1 file changed, 39 insertions(+), 26 deletions(-)
There's two patches in here... One to have a separate virQEMUCapsCopyHostCPUData and the other to create the 'hostCPU'
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index b8e4e47b6..c75aa570c 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -414,8 +414,10 @@ struct _virQEMUCaps { * re-computed from the other fields or external data sources every * time we probe QEMU or load the results from the cache. */ - virCPUDefPtr kvmCPUModel; - virCPUDefPtr tcgCPUModel; + struct { + virCPUDefPtr kvm; + virCPUDefPtr tcg; + } hostCPU; };
Considering what you do in the next patch... Further pushing down kvm & tcg into another struct, why not introduce virQEMUCapsCPUModel now and make it a bit "flatter": struct virQEMUCapsCPUModel capsCPUModel; where virQEMUCapsCPUModel is: virCPUDefPtr kvmFull; virCPUDefPtr tcgFull; and eventually adds virCPUDefPtr kvmMigrate; virCPUDefPtr tcgMigrate; You could also eventually have use the Ptr and pass by & rather than seeing things like "hostCpu.kvm.full", the helper routines would take a virQEMUCapsCPUModelPtr and be able to use "capsCPUModel->kvmFull", "capsCPUModel->tcgFull", etc. I only say this because when I see "a.b.c" in code, I have two thoughts - one is this a union and is there anyway to "hide" the 'path.to.the.data.we.want'...
struct virQEMUCapsSearchData { @@ -2082,6 +2084,31 @@ virQEMUCapsNew(void) }
+static int +virQEMUCapsCopyHostCPUData(virQEMUCapsPtr dst, + virQEMUCapsPtr src) +{ + + if (src->kvmCPUModelInfo && + !(dst->kvmCPUModelInfo = qemuMonitorCPUModelInfoCopy(src->kvmCPUModelInfo))) + return -1; + + if (src->tcgCPUModelInfo && + !(dst->tcgCPUModelInfo = qemuMonitorCPUModelInfoCopy(src->tcgCPUModelInfo))) + return -1; + + if (src->hostCPU.kvm && + !(dst->hostCPU.kvm = virCPUDefCopy(src->hostCPU.kvm))) + return -1; + + if (src->hostCPU.tcg && + !(dst->hostCPU.tcg = virCPUDefCopy(src->hostCPU.tcg))) + return -1; + + return 0; +} + + virQEMUCapsPtr virQEMUCapsNewCopy(virQEMUCapsPtr qemuCaps) { virQEMUCapsPtr ret = virQEMUCapsNew(); @@ -2119,20 +2146,7 @@ virQEMUCapsPtr virQEMUCapsNewCopy(virQEMUCapsPtr qemuCaps) goto error; }
- if (qemuCaps->kvmCPUModel && - !(ret->kvmCPUModel = virCPUDefCopy(qemuCaps->kvmCPUModel))) - goto error; - - if (qemuCaps->tcgCPUModel && - !(ret->tcgCPUModel = virCPUDefCopy(qemuCaps->tcgCPUModel))) - goto error; - - if (qemuCaps->kvmCPUModelInfo && - !(ret->kvmCPUModelInfo = qemuMonitorCPUModelInfoCopy(qemuCaps->kvmCPUModelInfo))) - goto error; - - if (qemuCaps->tcgCPUModelInfo && - !(ret->tcgCPUModelInfo = qemuMonitorCPUModelInfoCopy(qemuCaps->tcgCPUModelInfo))) + if (virQEMUCapsCopyHostCPUData(ret, qemuCaps) < 0) goto error;
The above is "somewhat" it's own hunk with some code motion and some change to use the hostCpu.{kvm|tcg}
if (VIR_ALLOC_N(ret->machineTypes, qemuCaps->nmachineTypes) < 0) @@ -2183,8 +2197,8 @@ void virQEMUCapsDispose(void *obj)
qemuMonitorCPUModelInfoFree(qemuCaps->kvmCPUModelInfo); qemuMonitorCPUModelInfoFree(qemuCaps->tcgCPUModelInfo); - virCPUDefFree(qemuCaps->kvmCPUModel); - virCPUDefFree(qemuCaps->tcgCPUModel); + virCPUDefFree(qemuCaps->hostCPU.kvm); + virCPUDefFree(qemuCaps->hostCPU.tcg);
Hmmm. perhaps a 3rd possible patch... Considering patch 5 and a single capsCPUModel, this could become a helper that knows how to virCPUDefFree each of the structure elements.
}
void @@ -2413,9 +2427,9 @@ virQEMUCapsGetHostModel(virQEMUCapsPtr qemuCaps, virDomainVirtType type) { if (type == VIR_DOMAIN_VIRT_KVM) - return qemuCaps->kvmCPUModel; + return qemuCaps->hostCPU.kvm; else - return qemuCaps->tcgCPUModel; + return qemuCaps->hostCPU.tcg; }
@@ -3296,9 +3310,9 @@ virQEMUCapsInitHostCPUModel(virQEMUCapsPtr qemuCaps, }
if (type == VIR_DOMAIN_VIRT_KVM) - qemuCaps->kvmCPUModel = cpu; + qemuCaps->hostCPU.kvm = cpu; else - qemuCaps->tcgCPUModel = cpu; + qemuCaps->hostCPU.tcg = cpu;
Seems to me this hunk "could" be it's own helper too - something like virQEMUCapsSetHostModel... especially once migrate CPU is added...
cleanup: virCPUDefFree(hostCPU); @@ -4053,10 +4067,9 @@ virQEMUCapsReset(virQEMUCapsPtr qemuCaps) qemuCaps->kvmCPUModelInfo = NULL; qemuCaps->tcgCPUModelInfo = NULL;
- virCPUDefFree(qemuCaps->kvmCPUModel); - virCPUDefFree(qemuCaps->tcgCPUModel); - qemuCaps->kvmCPUModel = NULL; - qemuCaps->tcgCPUModel = NULL; + virCPUDefFree(qemuCaps->hostCPU.kvm); + virCPUDefFree(qemuCaps->hostCPU.tcg);
Similar usage of single helper to free everything. John
+ memset(&qemuCaps->hostCPU, 0, sizeof(qemuCaps->hostCPU)); }