From 728231dcbbf2a4ab8a4bfe03fdf43d78bdeccbb0 Mon Sep 17 00:00:00 2001 Message-Id: <728231dcbbf2a4ab8a4bfe03fdf43d78bdeccbb0@dist-git> From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= 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 Signed-off-by: Daniel P. Berrangé (cherry picked from commit 0815f519784a7c565c543498777dc25f129620f9) https://bugzilla.redhat.com/show_bug.cgi?id=1686895 Signed-off-by: Jiri Denemark Message-Id: Reviewed-by: Ján Tomko --- 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