cpu: push more parsing logic into common code

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: NJiri Denemark <jdenemar@redhat.com>
Signed-off-by: NDaniel P. Berrangé <berrange@redhat.com>

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 (loadData(mapfile, ctxt, "feature", featureCB, data) < 0)
+        goto cleanup;
+
+    if (loadData(mapfile, ctxt, "model", modelCB, data) < 0)
+        goto cleanup;
 
-    if (loadIncludes(ctxt, cb, data) < 0)
+    if (loadIncludes(ctxt, vendorCB, featureCB, modelCB, data) < 0)
         goto cleanup;
 
     ret = 0;

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__ */

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;

src/cpu/cpu_x86.c

@@ -721,22 +721,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,
@@ -755,40 +754,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)
 {
@@ -910,27 +890,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,
@@ -958,46 +938,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)
 {
@@ -1193,47 +1155,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;
@@ -1288,62 +1249,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)
-{
-    virCPUx86ModelPtr 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 = x86ModelParse(ctxt, map)))
-            return -1;
-        map->models[map->nmodels++] = model;
-    }
-
-    return 0;
-}
-
-
 static void
 x86MapFree(virCPUx86MapPtr map)
 {
@@ -1373,30 +1315,6 @@ x86MapFree(virCPUx86MapPtr map)
 }
 
 
-static int
-x86MapLoadCallback(cpuMapElement element,
-                   xmlXPathContextPtr ctxt,
-                   xmlNodePtr *nodes,
-                   int n,
-                   void *data)
-{
-    virCPUx86MapPtr map = data;
-
-    switch (element) {
-    case CPU_MAP_ELEMENT_VENDOR:
-        return x86VendorsLoad(map, ctxt, nodes, n);
-    case CPU_MAP_ELEMENT_FEATURE:
-        return x86FeaturesLoad(map, ctxt, nodes, n);
-    case CPU_MAP_ELEMENT_MODEL:
-        return x86ModelsLoad(map, ctxt, nodes, n);
-    case CPU_MAP_ELEMENT_LAST:
-        break;
-    }
-
-    return 0;
-}
-
-
 static virCPUx86MapPtr
 virCPUx86LoadMap(void)
 {
@@ -1405,7 +1323,7 @@ virCPUx86LoadMap(void)
     if (VIR_ALLOC(map) < 0)
         return NULL;
 
-    if (cpuMapLoad("x86", x86MapLoadCallback, map) < 0)
+    if (cpuMapLoad("x86", x86VendorParse, x86FeatureParse, x86ModelParse, map) < 0)
         goto error;
 
     return map;