[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v2 14/33] cpu_x86: Drop virCPUx86MakeData and use virCPUDataNew




On 02/15/2017 11:44 AM, Jiri Denemark wrote:
> Signed-off-by: Jiri Denemark <jdenemar redhat com>
> ---
> 
> Notes:
>     Version 2:
>     - no change
> 
>  src/cpu/cpu_x86.c              | 147 ++++++++++++++++++-----------------------
>  src/cpu/cpu_x86.h              |   3 -
>  src/libvirt_private.syms       |   1 -
>  src/libxl/libxl_capabilities.c |  18 ++---
>  src/qemu/qemu_monitor_json.c   |  33 ++++-----
>  5 files changed, 92 insertions(+), 110 deletions(-)
> 

[...]

> diff --git a/src/libxl/libxl_capabilities.c b/src/libxl/libxl_capabilities.c
> index 2bbd2d1b4..622e9f6bb 100644
> --- a/src/libxl/libxl_capabilities.c
> +++ b/src/libxl/libxl_capabilities.c
> @@ -65,14 +65,14 @@ struct guest_arch {
>  #define XEN_CAP_REGEX "(xen|hvm)-[[:digit:]]+\\.[[:digit:]]+-(aarch64|armv7l|x86_32|x86_64|ia64|powerpc64)(p|be)?"
>  
>  static int
> -libxlCapsAddCPUID(virCPUx86Data *data, virCPUx86CPUID *cpuid, ssize_t ncaps)
> +libxlCapsAddCPUID(virCPUDataPtr data, virCPUx86CPUID *cpuid, ssize_t ncaps)
>  {
>      size_t i;
>  
>      for (i = 0; i < ncaps; i++) {
>          virCPUx86CPUID *c = &cpuid[i];
>  
> -        if (virCPUx86DataAddCPUID(data, c) < 0) {
> +        if (virCPUx86DataAddCPUID(&data->data.x86, c) < 0) {
>              VIR_DEBUG("Failed to add CPUID(%x,%x)", c->eax_in, c->ecx_in);
>              return -1;
>          }
> @@ -112,7 +112,6 @@ libxlCapsNodeData(virCPUDefPtr cpu, libxl_hwcap hwcap,
>  {
>      ssize_t ncaps;
>      virCPUDataPtr cpudata = NULL;
> -    virCPUx86Data data = VIR_CPU_X86_DATA_INIT;
>      virCPUx86CPUID cpuid[] = {
>          { .eax_in = 0x00000001,
>            .edx = hwcap[0] },
> @@ -131,20 +130,23 @@ libxlCapsNodeData(virCPUDefPtr cpu, libxl_hwcap hwcap,
>          { .eax_in = 0x80000007, .ecx_in = 0U, .edx = hwcap[7] },
>      };
>  
> +    if (!(cpudata = virCPUDataNew(cpu->arch)))
> +        goto error;
> +
>      ncaps = ARRAY_CARDINALITY(cpuid);
> -    if (libxlCapsAddCPUID(&data, cpuid, ncaps) < 0)
> +    if (libxlCapsAddCPUID(cpudata, cpuid, ncaps) < 0)
>          goto error;
>  
>      ncaps = ARRAY_CARDINALITY(cpuid_ver1);
>      if (version > LIBXL_HWCAP_V0 &&
> -        libxlCapsAddCPUID(&data, cpuid_ver1, ncaps) < 0)
> +        libxlCapsAddCPUID(cpudata, cpuid_ver1, ncaps) < 0)
>          goto error;
>  
> -    cpudata = virCPUx86MakeData(cpu->arch, &data);
> +    return cpudata;
>  
>   error:
> -    virCPUx86DataClear(&data);
> -    return cpudata;
> +    cpuDataFree(cpudata);

Why is this not x86FreeCPUData ?  Or should other code use the
cpuDataFree?  Am I missing something subtle?

> +    return NULL;
>  }
>  
>  /* hw_caps is an array of 32-bit words whose meaning is listed in
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 415761525..b4da12167 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -6565,37 +6565,35 @@ qemuMonitorJSONParseCPUx86FeatureWord(virJSONValuePtr data,
>  }
>  
>  
> -static int
> -qemuMonitorJSONParseCPUx86Features(virJSONValuePtr data,
> -                                   virCPUDataPtr *cpudata)
> +static virCPUDataPtr
> +qemuMonitorJSONParseCPUx86Features(virJSONValuePtr data)
>  {
> -    virCPUx86Data x86Data = VIR_CPU_X86_DATA_INIT;
> +    virCPUDataPtr cpudata = NULL;
>      virCPUx86CPUID cpuid;
>      size_t i;
>      ssize_t n;
> -    int ret = -1;
>  
>      if (!data || (n = virJSONValueArraySize(data)) < 0) {
>          virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                         _("invalid array of CPUID features"));
> -        return -1;
> +        goto error;

Could just return NULL, right?

>      }
>  
> +    if (!(cpudata = virCPUDataNew(VIR_ARCH_X86_64)))
> +        goto error;
> +
>      for (i = 0; i < n; i++) {
>          if (qemuMonitorJSONParseCPUx86FeatureWord(virJSONValueArrayGet(data, i),
>                                                    &cpuid) < 0 ||
> -            virCPUx86DataAddCPUID(&x86Data, &cpuid) < 0)
> -            goto cleanup;
> +            virCPUx86DataAddCPUID(&cpudata->data.x86, &cpuid) < 0)
> +            goto error;
>      }
>  
> -    if (!(*cpudata = virCPUx86MakeData(VIR_ARCH_X86_64, &x86Data)))
> -        goto cleanup;
> +    return cpudata;
>  
> -    ret = 0;
> -
> - cleanup:
> -    virCPUx86DataClear(&x86Data);
> -    return ret;
> + error:
> +    cpuDataFree(cpudata);

Similar query about x86FreeCPUData usage.

Otherwise, things look fine... Perhaps a moot point since path 16 makes
a common API...  Perhaps the only reason to change would be if bisection
breaks.

ACK - I'll leave it to you to handle the calls properly

John

> +    return NULL;
>  }
>  
>  
> @@ -6622,7 +6620,10 @@ qemuMonitorJSONGetCPUx86Data(qemuMonitorPtr mon,
>          goto cleanup;
>  
>      data = virJSONValueObjectGetArray(reply, "return");
> -    ret = qemuMonitorJSONParseCPUx86Features(data, cpudata);
> +    if (!(*cpudata = qemuMonitorJSONParseCPUx86Features(data)))
> +        goto cleanup;
> +
> +    ret = 0;
>  
>   cleanup:
>      virJSONValueFree(cmd);
> 


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]