[libvirt] [PATCH 2/4] cpu: push more parsing logic into common code

[Jiri Denemark]

On Wed, Aug 01, 2018 at 18:02:30 +0100, Daniel P. Berrangé wrote:
> The x86 and ppc impls both duplicate some logic when parsing CPU
> features. Change the callback signature so that this duplication can be
> pushed up a level to common code.
> 
> Signed-off-by: Daniel P. Berrangé <berrange at redhat.com>
> ---
>  src/cpu/cpu_map.c   | 106 +++++++++++++++---------
>  src/cpu/cpu_map.h   |  22 ++---
>  src/cpu/cpu_ppc64.c | 112 ++++++-------------------
>  src/cpu/cpu_x86.c   | 196 +++++++++++++-------------------------------
>  4 files changed, 155 insertions(+), 281 deletions(-)
> 
> diff --git a/src/cpu/cpu_map.c b/src/cpu/cpu_map.c
> index 9e090919ed..17ed53fda6 100644
> --- a/src/cpu/cpu_map.c
> +++ b/src/cpu/cpu_map.c
> @@ -35,31 +35,51 @@
>  
>  VIR_LOG_INIT("cpu.cpu_map");
>  
> -VIR_ENUM_IMPL(cpuMapElement, CPU_MAP_ELEMENT_LAST,
> -    "vendor",
> -    "feature",
> -    "model")
> -
> -
> -static int load(xmlXPathContextPtr ctxt,
> -                cpuMapElement element,
> -                cpuMapLoadCallback callback,
> -                void *data)
> +static int
> +loadData(const char *mapfile,
> +         xmlXPathContextPtr ctxt,
> +         const char *xpath,

Do you have any further plans with @xpath and actually pass something
more fancy than just an element name in it? If not, I'd suggest
renaming this parameter as "element" or something similar. Initially I
was confused what XPath expression you'd be passing to loadData and
whether including the expression in the error messages is a good idea.

> +         cpuMapLoadCallback callback,
> +         void *data)
>  {
>      int ret = -1;
>      xmlNodePtr ctxt_node;
>      xmlNodePtr *nodes = NULL;
>      int n;
> +    size_t i;
> +    int rv;
>  
>      ctxt_node = ctxt->node;
>  
> -    n = virXPathNodeSet(cpuMapElementTypeToString(element), ctxt, &nodes);
> -    if (n < 0)
> +    n = virXPathNodeSet(xpath, ctxt, &nodes);
> +    if (n < 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("cannot find '%s' in CPU map '%s'"), xpath, mapfile);

virXPathNodeSet already reports an appropriate error message before
returning -1. Not to mention that the function would return 0 if the
element was not found (in which case you don't want to report error
anyway). Simply removing the call to virReportError will fix both
issues.

>          goto cleanup;
> +    }
>  
> -    if (n > 0 &&
> -        callback(element, ctxt, nodes, n, data) < 0)
> +    if (n > 0 && !callback) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Unexpected %s in CPU map '%s'"), xpath, mapfile);
>          goto cleanup;
> +    }
> +
> +    for (i = 0; i < n; i++) {
> +        xmlNodePtr old = ctxt->node;
> +        char *name = virXMLPropString(nodes[i], "name");
> +        if (!name) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("cannot find %s name in CPU map '%s'"), xpath, mapfile);
> +            goto cleanup;
> +        }
> +        VIR_DEBUG("Load %s name %s", xpath, name);
> +        ctxt->node = nodes[i];
> +        rv = callback(ctxt, name, data);
> +        ctxt->node = old;
> +        VIR_FREE(name);
> +        if (rv < 0)
> +            goto cleanup;
> +    }
>  
>      ret = 0;
>  
> @@ -72,13 +92,14 @@ static int load(xmlXPathContextPtr ctxt,
>  
>  static int
>  cpuMapLoadInclude(const char *filename,
> -                  cpuMapLoadCallback cb,
> +                  cpuMapLoadCallback vendorCB,
> +                  cpuMapLoadCallback featureCB,
> +                  cpuMapLoadCallback modelCB,
>                    void *data)
>  {
>      xmlDocPtr xml = NULL;
>      xmlXPathContextPtr ctxt = NULL;
>      int ret = -1;
> -    int element;
>      char *mapfile;
>  
>      if (!(mapfile = virFileFindResource(filename,
> @@ -93,13 +114,14 @@ cpuMapLoadInclude(const char *filename,
>  
>      ctxt->node = xmlDocGetRootElement(xml);
>  
> -    for (element = 0; element < CPU_MAP_ELEMENT_LAST; element++) {
> -        if (load(ctxt, element, cb, data) < 0) {
> -            virReportError(VIR_ERR_INTERNAL_ERROR,
> -                           _("cannot parse CPU map '%s'"), mapfile);
> -            goto cleanup;
> -        }
> -    }
> +    if (loadData(mapfile, ctxt, "vendor", vendorCB, data) < 0)
> +        goto cleanup;
> +
> +    if (loadData(mapfile, ctxt, "feature", featureCB, data) < 0)
> +        goto cleanup;
> +
> +    if (loadData(mapfile, ctxt, "model", modelCB, data) < 0)
> +        goto cleanup;
>  
>      ret = 0;
>  
> @@ -113,7 +135,9 @@ cpuMapLoadInclude(const char *filename,
>  
>  
>  static int loadIncludes(xmlXPathContextPtr ctxt,
> -                        cpuMapLoadCallback callback,
> +                        cpuMapLoadCallback vendorCB,
> +                        cpuMapLoadCallback featureCB,
> +                        cpuMapLoadCallback modelCB,
>                          void *data)
>  {
>      int ret = -1;
> @@ -131,7 +155,7 @@ static int loadIncludes(xmlXPathContextPtr ctxt,
>      for (i = 0; i < n; i++) {
>          char *filename = virXMLPropString(nodes[i], "filename");
>          VIR_DEBUG("Finding CPU map include '%s'", filename);
> -        if (cpuMapLoadInclude(filename, callback, data) < 0) {
> +        if (cpuMapLoadInclude(filename, vendorCB, featureCB, modelCB, data) < 0) {
>              VIR_FREE(filename);
>              goto cleanup;
>          }
> @@ -149,7 +173,9 @@ static int loadIncludes(xmlXPathContextPtr ctxt,
>  
>  
>  int cpuMapLoad(const char *arch,
> -               cpuMapLoadCallback cb,
> +               cpuMapLoadCallback vendorCB,
> +               cpuMapLoadCallback featureCB,
> +               cpuMapLoadCallback modelCB,
>                 void *data)
>  {
>      xmlDocPtr xml = NULL;
> @@ -157,7 +183,6 @@ int cpuMapLoad(const char *arch,
>      virBuffer buf = VIR_BUFFER_INITIALIZER;
>      char *xpath = NULL;
>      int ret = -1;
> -    int element;
>      char *mapfile;
>  
>      if (!(mapfile = virFileFindResource("cpu_map.xml",
> @@ -173,9 +198,15 @@ int cpuMapLoad(const char *arch,
>          goto cleanup;
>      }
>  
> -    if (cb == NULL) {
> +    if (vendorCB == NULL) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       "%s", _("no vendor callback provided"));
> +        goto cleanup;
> +    }
> +
> +    if (modelCB == NULL) {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
> -                       "%s", _("no callback provided"));
> +                       "%s", _("no model callback provided"));
>          goto cleanup;
>      }

I'd remove both checks as suggested by John.

...

Jirka