From 728231dcbbf2a4ab8a4bfe03fdf43d78bdeccbb0 Mon Sep 17 00:00:00 2001

Message-Id: <728231dcbbf2a4ab8a4bfe03fdf43d78bdeccbb0@dist-git>

From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= <berrange@redhat.com>

Date: Fri, 21 Jun 2019 09:24:46 +0200

Subject: [PATCH] cpu: push more parsing logic into common code

MIME-Version: 1.0

Content-Type: text/plain; charset=UTF-8

Content-Transfer-Encoding: 8bit

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.

Reviewed-by: Jiri Denemark <jdenemar@redhat.com>

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

(cherry picked from commit 0815f519784a7c565c543498777dc25f129620f9)

https://bugzilla.redhat.com/show_bug.cgi?id=1686895

Signed-off-by: Jiri Denemark <jdenemar@redhat.com>

Message-Id: <c770b678131dc5e207cbe84d276a2774e0c807c7.1561068591.git.jdenemar@redhat.com>

Reviewed-by: Ján Tomko <jtomko@redhat.com>

---

 src/cpu/cpu_map.c   |  98 +++++++++++++---------

 src/cpu/cpu_map.h   |  22 ++---

 src/cpu/cpu_ppc64.c | 112 ++++++-------------------

 src/cpu/cpu_x86.c   | 196 +++++++++++++-------------------------------

 4 files changed, 143 insertions(+), 285 deletions(-)

diff --git a/src/cpu/cpu_map.c b/src/cpu/cpu_map.c

index 333c7ef24f..ac7e58037a 100644

--- a/src/cpu/cpu_map.c

+++ b/src/cpu/cpu_map.c

@@ -35,31 +35,47 @@

 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 *element,

+         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)

+    if ((n = virXPathNodeSet(element, ctxt, &nodes)) < 0)

         goto cleanup;

-    if (n > 0 &&

-        callback(element, ctxt, nodes, n, data) < 0)

+    if (n > 0 && !callback) {

+        virReportError(VIR_ERR_INTERNAL_ERROR,

+                       _("Unexpected element '%s' in CPU map '%s'"), element, 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'"), element, mapfile);

+            goto cleanup;

+        }

+        VIR_DEBUG("Load %s name %s", element, 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 +88,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 +110,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;

@@ -114,7 +132,9 @@ cpuMapLoadInclude(const char *filename,

 static int

 loadIncludes(xmlXPathContextPtr ctxt,

-             cpuMapLoadCallback callback,

+             cpuMapLoadCallback vendorCB,

+             cpuMapLoadCallback featureCB,

+             cpuMapLoadCallback modelCB,

              void *data)

 {

     int ret = -1;

@@ -137,7 +157,7 @@ loadIncludes(xmlXPathContextPtr ctxt,

             goto cleanup;

         }

         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;

         }

@@ -155,7 +175,9 @@ loadIncludes(xmlXPathContextPtr ctxt,

 int cpuMapLoad(const char *arch,

-               cpuMapLoadCallback cb,

+               cpuMapLoadCallback vendorCB,

+               cpuMapLoadCallback featureCB,

+               cpuMapLoadCallback modelCB,

                void *data)

 {

     xmlDocPtr xml = NULL;

@@ -163,7 +185,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",

@@ -179,12 +200,6 @@ int cpuMapLoad(const char *arch,

         goto cleanup;

     }

-    if (cb == NULL) {

-        virReportError(VIR_ERR_INTERNAL_ERROR,

-                       "%s", _("no callback provided"));

-        goto cleanup;

-    }

-

     if (!(xml = virXMLParseFileCtxt(mapfile, &ctxt)))

         goto cleanup;

@@ -202,15 +217,16 @@ int cpuMapLoad(const char *arch,

         goto cleanup;

     }

-    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 (loadIncludes(ctxt, cb, data) < 0)

+    if (loadData(mapfile, ctxt, "feature", featureCB, data) < 0)

+        goto cleanup;

+

+    if (loadData(mapfile, ctxt, "model", modelCB, data) < 0)

+        goto cleanup;

+

+    if (loadIncludes(ctxt, vendorCB, featureCB, modelCB, data) < 0)

         goto cleanup;

     ret = 0;

diff --git a/src/cpu/cpu_map.h b/src/cpu/cpu_map.h

index 0c7507e98f..4596987150 100644

--- a/src/cpu/cpu_map.h

+++ b/src/cpu/cpu_map.h

@@ -26,28 +26,16 @@

 # include "virxml.h"

-

-typedef enum {

-    CPU_MAP_ELEMENT_VENDOR,

-    CPU_MAP_ELEMENT_FEATURE,

-    CPU_MAP_ELEMENT_MODEL,

-

-    CPU_MAP_ELEMENT_LAST

-} cpuMapElement;

-

-VIR_ENUM_DECL(cpuMapElement)

-

-

 typedef int

-(*cpuMapLoadCallback)  (cpuMapElement element,

-                        xmlXPathContextPtr ctxt,

-                        xmlNodePtr *nodes,

-                        int n,

+(*cpuMapLoadCallback)  (xmlXPathContextPtr ctxt,

+                        const char *name,

                         void *data);

 int

 cpuMapLoad(const char *arch,

-           cpuMapLoadCallback cb,

+           cpuMapLoadCallback vendorCB,

+           cpuMapLoadCallback featureCB,

+           cpuMapLoadCallback modelCB,

            void *data);

 #endif /* __VIR_CPU_MAP_H__ */

diff --git a/src/cpu/cpu_ppc64.c b/src/cpu/cpu_ppc64.c

index d562677fa3..75da5b77d8 100644

--- a/src/cpu/cpu_ppc64.c

+++ b/src/cpu/cpu_ppc64.c

@@ -281,21 +281,19 @@ ppc64MapFree(struct ppc64_map *map)

     VIR_FREE(map);

 }

-static struct ppc64_vendor *

-ppc64VendorParse(xmlXPathContextPtr ctxt,

-                 struct ppc64_map *map)

+static int

+ppc64VendorParse(xmlXPathContextPtr ctxt ATTRIBUTE_UNUSED,

+                 const char *name,

+                 void *data)

 {

+    struct ppc64_map *map = data;

     struct ppc64_vendor *vendor;

     if (VIR_ALLOC(vendor) < 0)

-        return NULL;

+        return -1;

-    vendor->name = virXPathString("string(@name)", ctxt);

-    if (!vendor->name) {

-        virReportError(VIR_ERR_INTERNAL_ERROR,

-                       "%s", _("Missing CPU vendor name"));

+    if (VIR_STRDUP(vendor->name, name) < 0)

         goto error;

-    }

     if (ppc64VendorFind(map, vendor->name)) {

         virReportError(VIR_ERR_INTERNAL_ERROR,

@@ -303,57 +301,36 @@ ppc64VendorParse(xmlXPathContextPtr ctxt,

         goto error;

     }

-    return vendor;

+    if (VIR_APPEND_ELEMENT(map->vendors, map->nvendors, vendor) < 0)

+        goto error;

+

+    return 0;

  error:

     ppc64VendorFree(vendor);

-    return NULL;

+    return -1;

 }

 static int

-ppc64VendorsLoad(struct ppc64_map *map,

-                 xmlXPathContextPtr ctxt,

-                 xmlNodePtr *nodes,

-                 int n)

-{

-    struct ppc64_vendor *vendor;

-    size_t i;

-

-    if (VIR_ALLOC_N(map->vendors, n) < 0)

-        return -1;

-

-    for (i = 0; i < n; i++) {

-        ctxt->node = nodes[i];

-        if (!(vendor = ppc64VendorParse(ctxt, map)))

-            return -1;

-        map->vendors[map->nvendors++] = vendor;

-    }

-

-    return 0;

-}

-

-

-static struct ppc64_model *

 ppc64ModelParse(xmlXPathContextPtr ctxt,

-                struct ppc64_map *map)

+                const char *name,

+                void *data)

 {

+    struct ppc64_map *map = data;

     struct ppc64_model *model;

     xmlNodePtr *nodes = NULL;

     char *vendor = NULL;

     unsigned long pvr;

     size_t i;

     int n;

+    int ret = -1;

     if (VIR_ALLOC(model) < 0)

         goto error;

-    model->name = virXPathString("string(@name)", ctxt);

-    if (!model->name) {

-        virReportError(VIR_ERR_INTERNAL_ERROR,

-                       "%s", _("Missing CPU model name"));

+    if (VIR_STRDUP(model->name, name) < 0)

         goto error;

-    }

     if (ppc64ModelFind(map, model->name)) {

         virReportError(VIR_ERR_INTERNAL_ERROR,

@@ -410,63 +387,22 @@ ppc64ModelParse(xmlXPathContextPtr ctxt,

         model->data.pvr[i].mask = pvr;

     }

+    if (VIR_APPEND_ELEMENT(map->models, map->nmodels, model) < 0)

+        goto error;

+

+    ret = 0;

+

  cleanup:

     VIR_FREE(vendor);

     VIR_FREE(nodes);

-    return model;

+    return ret;

  error:

     ppc64ModelFree(model);

-    model = NULL;

     goto cleanup;

 }

-static int

-ppc64ModelsLoad(struct ppc64_map *map,

-                xmlXPathContextPtr ctxt,

-                xmlNodePtr *nodes,

-                int n)

-{

-    struct ppc64_model *model;

-    size_t i;

-

-    if (VIR_ALLOC_N(map->models, n) < 0)

-        return -1;

-

-    for (i = 0; i < n; i++) {

-        ctxt->node = nodes[i];

-        if (!(model = ppc64ModelParse(ctxt, map)))

-            return -1;

-        map->models[map->nmodels++] = model;

-    }

-

-    return 0;

-}

-

-

-static int

-ppc64MapLoadCallback(cpuMapElement element,

-                     xmlXPathContextPtr ctxt,

-                     xmlNodePtr *nodes,

-                     int n,

-                     void *data)

-{

-    struct ppc64_map *map = data;

-

-    switch (element) {

-    case CPU_MAP_ELEMENT_VENDOR:

-        return ppc64VendorsLoad(map, ctxt, nodes, n);

-    case CPU_MAP_ELEMENT_MODEL:

-        return ppc64ModelsLoad(map, ctxt, nodes, n);

-    case CPU_MAP_ELEMENT_FEATURE:

-    case CPU_MAP_ELEMENT_LAST:

-        break;

-    }

-

-    return 0;

-}

-

 static struct ppc64_map *

 ppc64LoadMap(void)

 {

@@ -475,7 +411,7 @@ ppc64LoadMap(void)

     if (VIR_ALLOC(map) < 0)

         goto error;

-    if (cpuMapLoad("ppc64", ppc64MapLoadCallback, map) < 0)

+    if (cpuMapLoad("ppc64", ppc64VendorParse, NULL, ppc64ModelParse, map) < 0)

         goto error;

     return map;

diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c

index 124aa5fd5e..11dbd1e757 100644

--- a/src/cpu/cpu_x86.c

+++ b/src/cpu/cpu_x86.c

@@ -720,22 +720,21 @@ x86VendorFind(virCPUx86MapPtr map,

 }

-static virCPUx86VendorPtr

+static int

 x86VendorParse(xmlXPathContextPtr ctxt,

-               virCPUx86MapPtr map)

+               const char *name,

+               void *data)

 {

+    virCPUx86MapPtr map = data;

     virCPUx86VendorPtr vendor = NULL;

     char *string = NULL;

+    int ret = -1;

     if (VIR_ALLOC(vendor) < 0)

         goto error;

-    vendor->name = virXPathString("string(@name)", ctxt);

-    if (!vendor->name) {

-        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",

-                       _("Missing CPU vendor name"));

+    if (VIR_STRDUP(vendor->name, name) < 0)

         goto error;

-    }

     if (x86VendorFind(map, vendor->name)) {

         virReportError(VIR_ERR_INTERNAL_ERROR,

@@ -754,40 +753,21 @@ x86VendorParse(xmlXPathContextPtr ctxt,

     if (virCPUx86VendorToCPUID(string, &vendor->cpuid) < 0)

         goto error;

+    if (VIR_APPEND_ELEMENT(map->vendors, map->nvendors, vendor) < 0)

+        goto error;

+

+    ret = 0;

+

  cleanup:

     VIR_FREE(string);

-    return vendor;

+    return ret;

  error:

     x86VendorFree(vendor);

-    vendor = NULL;

     goto cleanup;

 }

-static int

-x86VendorsLoad(virCPUx86MapPtr map,

-               xmlXPathContextPtr ctxt,

-               xmlNodePtr *nodes,

-               int n)

-{

-    virCPUx86VendorPtr vendor;

-    size_t i;

-

-    if (VIR_ALLOC_N(map->vendors, n) < 0)

-        return -1;

-

-    for (i = 0; i < n; i++) {

-        ctxt->node = nodes[i];

-        if (!(vendor = x86VendorParse(ctxt, map)))

-            return -1;

-        map->vendors[map->nvendors++] = vendor;

-    }

-

-    return 0;

-}

-

-

 static virCPUx86FeaturePtr

 x86FeatureNew(void)

 {

@@ -909,27 +889,27 @@ x86ParseCPUID(xmlXPathContextPtr ctxt,

 }

-static virCPUx86FeaturePtr

+static int

 x86FeatureParse(xmlXPathContextPtr ctxt,

-                virCPUx86MapPtr map)

+                const char *name,

+                void *data)

 {

+    virCPUx86MapPtr map = data;

     xmlNodePtr *nodes = NULL;

     virCPUx86FeaturePtr feature;

     virCPUx86CPUID cpuid;

     size_t i;

     int n;

     char *str = NULL;

+    int ret = -1;

     if (!(feature = x86FeatureNew()))

         goto error;

     feature->migratable = true;

-    feature->name = virXPathString("string(@name)", ctxt);

-    if (!feature->name) {

-        virReportError(VIR_ERR_INTERNAL_ERROR,

-                       "%s", _("Missing CPU feature name"));

+

+    if (VIR_STRDUP(feature->name, name) < 0)

         goto error;

-    }

     if (x86FeatureFind(map, feature->name)) {

         virReportError(VIR_ERR_INTERNAL_ERROR,

@@ -957,46 +937,28 @@ x86FeatureParse(xmlXPathContextPtr ctxt,

             goto error;

     }

+    if (!feature->migratable &&

+        VIR_APPEND_ELEMENT_COPY(map->migrate_blockers,

+                                map->nblockers,

+                                feature) < 0)

+        goto error;

+

+    if (VIR_APPEND_ELEMENT(map->features, map->nfeatures, feature) < 0)

+        goto error;

+

+    ret = 0;

+

  cleanup:

     VIR_FREE(nodes);

     VIR_FREE(str);

-    return feature;

+    return ret;

  error:

     x86FeatureFree(feature);

-    feature = NULL;

     goto cleanup;

 }

-static int

-x86FeaturesLoad(virCPUx86MapPtr map,

-                xmlXPathContextPtr ctxt,

-                xmlNodePtr *nodes,

-                int n)

-{

-    virCPUx86FeaturePtr feature;

-    size_t i;

-

-    if (VIR_ALLOC_N(map->features, n) < 0)

-        return -1;

-

-    for (i = 0; i < n; i++) {

-        ctxt->node = nodes[i];

-        if (!(feature = x86FeatureParse(ctxt, map)))

-            return -1;

-        map->features[map->nfeatures++] = feature;

-        if (!feature->migratable &&

-            VIR_APPEND_ELEMENT(map->migrate_blockers,

-                               map->nblockers,

-                               feature) < 0)

-            return -1;

-    }

-

-    return 0;

-}

-

-

 static virCPUx86ModelPtr

 x86ModelNew(void)

 {

@@ -1192,47 +1154,46 @@ x86ModelCompare(virCPUx86ModelPtr model1,

 }

-static virCPUx86ModelPtr

+static int

 x86ModelParse(xmlXPathContextPtr ctxt,

-              virCPUx86MapPtr map)

+              const char *name,

+              void *data)

 {

+    virCPUx86MapPtr map = data;

     xmlNodePtr *nodes = NULL;

     virCPUx86ModelPtr model;

     char *vendor = NULL;

     size_t i;

     int n;

+    int ret = -1;

     if (!(model = x86ModelNew()))

         goto error;

-    model->name = virXPathString("string(@name)", ctxt);

-    if (!model->name) {

-        virReportError(VIR_ERR_INTERNAL_ERROR,

-                       "%s", _("Missing CPU model name"));

+    if (VIR_STRDUP(model->name, name) < 0)

         goto error;

-    }

     if (virXPathNode("./model", ctxt)) {

         virCPUx86ModelPtr ancestor;

-        char *name;

+        char *anname;

-        name = virXPathString("string(./model/@name)", ctxt);

-        if (!name) {

+        anname = virXPathString("string(./model/@name)", ctxt);

+        if (!anname) {

             virReportError(VIR_ERR_INTERNAL_ERROR,

                            _("Missing ancestor's name in CPU model %s"),

                            model->name);

             goto error;

         }

-        if (!(ancestor = x86ModelFind(map, name))) {

+        if (!(ancestor = x86ModelFind(map, anname))) {

             virReportError(VIR_ERR_INTERNAL_ERROR,

                            _("Ancestor model %s not found for CPU model %s"),

-                           name, model->name);

-            VIR_FREE(name);

+                           anname, model->name);

+            VIR_FREE(anname);

             goto error;

         }

-        VIR_FREE(name);

+        VIR_FREE(anname);

         model->vendor = ancestor->vendor;

         model->signature = ancestor->signature;

@@ -1287,62 +1248,43 @@ x86ModelParse(xmlXPathContextPtr ctxt,

     for (i = 0; i < n; i++) {

         virCPUx86FeaturePtr feature;

-        char *name;

+        char *ftname;

-        if (!(name = virXMLPropString(nodes[i], "name"))) {

+        if (!(ftname = virXMLPropString(nodes[i], "name"))) {

             virReportError(VIR_ERR_INTERNAL_ERROR,

                            _("Missing feature name for CPU model %s"), model->name);

             goto error;

         }

-        if (!(feature = x86FeatureFind(map, name))) {

+        if (!(feature = x86FeatureFind(map, ftname))) {

             virReportError(VIR_ERR_INTERNAL_ERROR,

                            _("Feature %s required by CPU model %s not found"),

-                           name, model->name);

-            VIR_FREE(name);

+                           ftname, model->name);

+            VIR_FREE(ftname);

             goto error;

         }

-        VIR_FREE(name);

+        VIR_FREE(ftname);

         if (x86DataAdd(&model->data, &feature->data))

             goto error;

     }

+    if (VIR_APPEND_ELEMENT(map->models, map->nmodels, model) < 0)

+        goto error;

+

+    ret = 0;

+

  cleanup:

     VIR_FREE(vendor);

     VIR_FREE(nodes);

-    return model;

+    return ret;

  error:

     x86ModelFree(model);

-    model = NULL;

     goto cleanup;

 }

-static int

-x86ModelsLoad(virCPUx86MapPtr map,

-              xmlXPathContextPtr ctxt,

-              xmlNodePtr *nodes,

-              int n)