[libvirt] [PATCH 4/7] qemu: Move qemuCaps->{kvm, tcg}CPUModel into a struct

Jiri Denemark posted 7 patches 182 weeks ago
Only 6 patches received!

[libvirt] [PATCH 4/7] qemu: Move qemuCaps->{kvm, tcg}CPUModel into a struct

Posted by Jiri Denemark 182 weeks ago
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

Re: [libvirt] [PATCH 4/7] qemu: Move qemuCaps->{kvm, tcg}CPUModel into a struct

Posted by John Ferlan 182 weeks ago

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

Re: [libvirt] [PATCH 4/7] qemu: Move qemuCaps->{kvm, tcg}CPUModel into a struct

Posted by Jiri Denemark 181 weeks ago
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

Re: [libvirt] [PATCH 4/7] qemu: Move qemuCaps->{kvm, tcg}CPUModel into a struct

Posted by Ján Tomko 181 weeks ago
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

Re: [libvirt] [PATCH 4/7] qemu: Move qemuCaps->{kvm, tcg}CPUModel into a struct

Posted by Jiri Denemark 181 weeks ago
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