[PATCH V2 4/5] cpu: Add helper funtions to parse vendor and model

[Daniel P. Berrangé]

On Thu, Apr 02, 2020 at 05:03:57PM +0800, Zhenyu Zheng wrote:
> Add helper functions to parse vendor and model from
> xml for ARM arch, and use them as callbacks when
> load cpu maps.
> 
> Signed-off-by: Zhenyu Zheng <zhengzhenyulixi at gmail.com>
> ---
>  src/cpu/cpu_arm.c | 176 +++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 174 insertions(+), 2 deletions(-)
> 
> diff --git a/src/cpu/cpu_arm.c b/src/cpu/cpu_arm.c
> index c757c24a37..88b4d91946 100644
> --- a/src/cpu/cpu_arm.c
> +++ b/src/cpu/cpu_arm.c
> @@ -206,6 +206,178 @@ virCPUarmMapFeatureParse(xmlXPathContextPtr ctxt G_GNUC_UNUSED,
>      return 0;
>  }
>  
> +static virCPUarmVendorPtr
> +armVendorFindByID(virCPUarmMapPtr map,
> +                  unsigned long vendor_id)

As with previous patch, we'd expect the name to be

  virCPUarmVendorFindByID()

and apply same naming scheme for other methods in this patch, so
I won't repeat this.


> +static int
> +armVendorParse(xmlXPathContextPtr ctxt,
> +               const char *name,
> +               void *data)
> +{
> +    virCPUarmMapPtr map = (virCPUarmMapPtr)data;

No need for the (virCPUarmMapPtr)  cast, as 'void *' will happily
cast to any other pointer in C.  Only C++ needs the explicit casts.

> +    virCPUarmVendorPtr vendor = NULL;
> +    int ret = -1;
> +
> +    if (VIR_ALLOC(vendor) < 0)
> +        return ret;
> +
> +    vendor->name = g_strdup(name);
> +
> +    if (armVendorFindByName(map, vendor->name)) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("CPU vendor %s already defined"),
> +                       vendor->name);
> +        goto cleanup;
> +    }
> +
> +    if (virXPathULongHex("string(@value)", ctxt, &vendor->value) < 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       "%s", _("Missing CPU vendor value"));
> +        goto cleanup;
> +    }
> +
> +    if (armVendorFindByID(map, vendor->value)) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("CPU vendor value 0x%2lx already defined"),
> +                       vendor->value);
> +        goto cleanup;
> +    }
> +
> +    if (VIR_APPEND_ELEMENT(map->vendors, map->nvendors, vendor) < 0)
> +        goto cleanup;
> +
> +    ret = 0;
> +
> + cleanup:
> +    armVendorFree(vendor);
> +    return ret;
> +
> +}


> +static int
> +armModelParse(xmlXPathContextPtr ctxt,
> +              const char *name,
> +              void *data)
> +{
> +    virCPUarmMapPtr map = (virCPUarmMapPtr)data;

Again, no need for a cast

> +    virCPUarmModel *model;
> +    xmlNodePtr *nodes = NULL;

g_autofree xmlNodePtr *nodes = NULL;

> +    char *vendor = NULL;

g_autofree char *vendor = NULL;

> +    int ret = -1;
> +
> +    if (VIR_ALLOC(model) < 0)
> +        goto error;
> +
> +    model->name = g_strdup(name);
> +
> +    if (armModelFind(map, model->name)) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("CPU model %s already defined"),
> +                       model->name);
> +        goto error;
> +    }
> +
> +    if (virXPathBoolean("boolean(./vendor)", ctxt)) {
> +        vendor = virXPathString("string(./vendor/@name)", ctxt);
> +        if (!vendor) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("Invalid vendor element in CPU model %s"),
> +                           model->name);
> +            goto error;
> +        }
> +
> +        if (!(model->vendor = armVendorFindByName(map, vendor))) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("Unknown vendor %s referenced by CPU model %s"),
> +                           vendor, model->name);
> +            goto error;
> +        }
> +    }
> +
> +    if (!virXPathBoolean("boolean(./pvr)", ctxt)) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Missing PVR information for CPU model %s"),
> +                       model->name);
> +        goto error;
> +    }
> +
> +    if (virXPathULongHex("string(./pvr/@value)", ctxt, &model->data.pvr) < 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Missing or invalid PVR value in CPU model %s"),
> +                       model->name);
> +        goto error;
> +    }
> +
> +    if (VIR_APPEND_ELEMENT(map->models, map->nmodels, model) < 0)
> +        goto error;
> +
> +    ret = 0;
> +
> + cleanup:
> +    VIR_FREE(vendor);
> +    VIR_FREE(nodes);

...you can drop these two thanks to the g_autofree, as well
as the "cleanup:" label and "ret" variable.

> +    return ret;

"return 0";

> +
> + error:
> +    armModelFree(model);
> +    goto cleanup;

...and just "return -1" here instead of goto

> +}
> +
>  static virCPUarmMapPtr
>  virCPUarmLoadMap(void)
>  {
> @@ -213,8 +385,8 @@ virCPUarmLoadMap(void)
>  
>      map = virCPUarmMapNew();
>  
> -    if (cpuMapLoad("arm", NULL, virCPUarmMapFeatureParse, NULL, map) < 0)
> -        return NULL;
> +    if (cpuMapLoad("arm", armVendorParse, virCPUarmMapFeatureParse,
> +                   armModelParse, map) < 0)
>  
>      return g_steal_pointer(&map);
>  }
> -- 
> 2.26.0.windows.1
> 
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|