9:35 a.m.
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(a)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