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(-)
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;
};
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;
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);
}
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;
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);
+ memset(&qemuCaps->hostCPU, 0, sizeof(qemuCaps->hostCPU));
}
--
2.12.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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));
> }
>
>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Apr 04, 2017 at 12:34:49 -0400, John Ferlan wrote: > > > 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' Oops, I accidentally pushed this series when I wanted to push the "qemu: Properly reset all migration capabilities" one :-( Are you OK with me sending the additional changes as follow-up patches rather than reverting the patches and pushing the updated ones? Jirka -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Fri, Apr 07, 2017 at 01:07:52PM +0200, Jiri Denemark wrote: >On Tue, Apr 04, 2017 at 12:34:49 -0400, John Ferlan wrote: >> >> >> 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' > >Oops, I accidentally pushed this series when I wanted to push the >"qemu: Properly reset all migration capabilities" one :-( Are you OK >with me sending the additional changes as follow-up patches rather than >reverting the patches and pushing the updated ones? > Reverting the patches and doing them right would IMO look better, especially if we ever need to backport them somewhere. Jan -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Fri, Apr 07, 2017 at 13:18:53 +0200, Ján Tomko wrote: > On Fri, Apr 07, 2017 at 01:07:52PM +0200, Jiri Denemark wrote: > >On Tue, Apr 04, 2017 at 12:34:49 -0400, John Ferlan wrote: > >> > >> > >> 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' > > > >Oops, I accidentally pushed this series when I wanted to push the > >"qemu: Properly reset all migration capabilities" one :-( Are you OK > >with me sending the additional changes as follow-up patches rather than > >reverting the patches and pushing the updated ones? > > > > Reverting the patches and doing them right would IMO look better, > especially if we ever need to backport them somewhere. OK, I guess it's probably also going to be easier to make the changes and review the resulting patches. I've sent the reverts... Jirka -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.