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

[John Ferlan]

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 at 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));
>  }
>  
>  
>