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)
-{
- 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)
{
@@ -1372,30 +1314,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)
{
@@ -1404,7 +1322,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;
--
2.22.0