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@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,
+ 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);
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;
}
@@ -196,15 +227,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;
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 809da94117..76f1d417c1 100644
--- a/src/cpu/cpu_x86.c
+++ b/src/cpu/cpu_x86.c
@@ -712,22 +712,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,
@@ -746,40 +745,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)
{
@@ -901,27 +881,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,
@@ -949,46 +929,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)
{
@@ -1184,47 +1146,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;
@@ -1279,62 +1240,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)
{
@@ -1364,30 +1306,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)
{
@@ -1396,7 +1314,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.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 08/01/2018 01:02 PM, 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@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,
> + 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);
> 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);
How about "Unexpected element %s..."
to be fair it's a callback function isn't provided for a specific arch,
but adding that level of detail would be a bit more painful for the low
level of benefit at least.
> goto cleanup;
> + }
> +
[...]
>
> 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;
> + }
To be fair, loadData does check "if (n > 0 && !callback)", so these
checks perhaps aren't necessary as if they were NULL we'd fail a bit
later on (if I'm reading things properly ;-)).
I suppose it doesn't matter if they stay or not until someone, some day
wonders why featureCB isn't checked.
> +
> + if (modelCB == NULL) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> - "%s", _("no callback provided"));
> + "%s", _("no model callback provided"));
> goto cleanup;
> }
>
[...]
> 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);
> }
>
[...]
> 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;
> +
Since VIR_APPEND_ELEMENT would clear @model on success, we don't
necessarily need the error and cleanup labels. More churn on the changes
though with the ppc64ModelFree moved to cleanup.
> + ret = 0;
> +
> cleanup:
> VIR_FREE(vendor);
> VIR_FREE(nodes);
> - return model;
> + return ret;
>
> error:
> ppc64ModelFree(model);
> - model = NULL;
> goto cleanup;
> }
>
>
[...]
> diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c
> index 809da94117..76f1d417c1 100644
> --- a/src/cpu/cpu_x86.c
> +++ b/src/cpu/cpu_x86.c
> @@ -712,22 +712,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,
> @@ -746,40 +745,21 @@ x86VendorParse(xmlXPathContextPtr ctxt,
> if (virCPUx86VendorToCPUID(string, &vendor->cpuid) < 0)
> goto error;
>
> + if (VIR_APPEND_ELEMENT(map->vendors, map->nvendors, vendor) < 0)
> + goto error;
Similar comment here regarding macro and cleanup/error labels
> +
> + ret = 0;
> +
> cleanup:
> VIR_FREE(string);
> - return vendor;
> + return ret;
>
> error:
> x86VendorFree(vendor);
> - vendor = NULL;
> goto cleanup;
> }
>
>
[...]
> -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,
> @@ -949,46 +929,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;
> +
Same here too.
> + ret = 0;
> +
> cleanup:
> VIR_FREE(nodes);
> VIR_FREE(str);
> - return feature;
> + return ret;
>
> error:
> x86FeatureFree(feature);
> - feature = NULL;
> goto cleanup;
> }
>
>
[...]
> +static int
> x86ModelParse(xmlXPathContextPtr ctxt,
> - virCPUx86MapPtr map)
> + const char *name,
> + void *data)
> {
[...]
>
> + if (VIR_APPEND_ELEMENT(map->models, map->nmodels, model) < 0)
> + goto error;
Similar regarding cleanup/error; however, ...
> +
> + ret = 0;
> +
> cleanup:
I ran the changes through Coverity and it complains here because one can
goto cleanup if the signature code fails, return -1, but the @model
wouldn't be free'd.
Prior to this change it seems failing in that code wasn't necessarily an
error, but it would generate the error, not go through the vendor and
feature processing, and return a somewhat empty @model. Perhaps a bug in
existing code, but uncovered in this refactoring.
> VIR_FREE(vendor);
> VIR_FREE(nodes);
> - return model;
> + return ret;
>
> error:
> x86ModelFree(model);
> - model = NULL;
> goto cleanup;
> }
>
>
[...]
Whether the VIR_APPEND_ELEMENT cleanup/error changes are made is your
call. The code is fine as is.
I think the signature return issue noted in x86ModelParse should
probably be it's own patch, but I'm not 100% clear what would happen if
you now error instead of just returning a mostly empty model. Since
it'd be developer error generating the file, perhaps we should just
error. Hopefully Jirka chimes in!
Reviewed-by: John Ferlan <jferlan@redhat.com>
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Aug 13, 2018 at 18:27:06 -0400, John Ferlan wrote:
>
>
> On 08/01/2018 01:02 PM, 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@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(-)
...
> > +static int
> > x86ModelParse(xmlXPathContextPtr ctxt,
> > - virCPUx86MapPtr map)
> > + const char *name,
> > + void *data)
> > {
>
> [...]
>
> >
> > + if (VIR_APPEND_ELEMENT(map->models, map->nmodels, model) < 0)
> > + goto error;
>
> Similar regarding cleanup/error; however, ...
>
> > +
> > + ret = 0;
> > +
> > cleanup:
>
> I ran the changes through Coverity and it complains here because one can
> goto cleanup if the signature code fails, return -1, but the @model
> wouldn't be free'd.
>
> Prior to this change it seems failing in that code wasn't necessarily an
> error, but it would generate the error, not go through the vendor and
> feature processing, and return a somewhat empty @model. Perhaps a bug in
> existing code, but uncovered in this refactoring.
Hmm, the gotos in the signature code should be jumping to the error
label. I guess the best fix would be a separate patch removing the error
label completely.
Jirka
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.