[libvirt] [PATCH 0/2] virsh: freecell --all getting wrong NUMA nodes count
This is fix for virsh freecell command. I was using virNodeGetInfo() instead of virConnectGetCapabilities() which returns XML with the right number. Therefore we need XML/XPath function to pick node values from XML kept in buffer. Moreover, this function might be used to replace bad practise in cmdVcpucount(). Michal Privoznik (2): virsh.c: new xPathULong() to parse XML returning ulong value of selected node. virsh: fix wrong NUMA nodes count getting tools/virsh.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++-------- 1 files changed, 65 insertions(+), 11 deletions(-) -- 1.7.3.5
Parse XML from given buffer, select appropiate XPath node and return its value as unsigned long. --- tools/virsh.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 49 insertions(+), 0 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 50d5e33..8804fc3 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -2270,6 +2270,55 @@ static const vshCmdInfo info_freecell[] = { {NULL, NULL} }; +static int +xPathULong(char *xml, char *xpath, unsigned long *value) { + + xmlDocPtr doc = NULL; + xmlXPathContextPtr ctxt = NULL; + xmlXPathObjectPtr obj = NULL; + int ret = -1; + + doc = xmlReadDoc((const xmlChar *) xml, "domain.xml", NULL, + XML_PARSE_NOENT | XML_PARSE_NONET | XML_PARSE_NOWARNING); + if (!doc) + goto cleanup; + + ctxt = xmlXPathNewContext(doc); + if (!ctxt) + goto cleanup; + + obj = xmlXPathEval(BAD_CAST xpath, ctxt); + if (!obj) + goto cleanup; + + if (!value) + goto cleanup; + + *value = 0; + if ((obj->type == XPATH_STRING) && + (obj->stringval != NULL) && (obj->stringval[0] != 0)) { + char *conv = NULL; + unsigned long val; + + val = strtoul((const char *) obj->stringval, &conv, 10); + if (*conv =='\0') { + *value = val; + ret = 0; + } + } else if ((obj != NULL) && (obj->type == XPATH_NUMBER)) { + *value = (unsigned long) obj->floatval; + if (*value == obj->floatval) { + ret = 0; + } + } + +cleanup: + xmlXPathFreeObject(obj); + xmlXPathFreeContext(ctxt); + xmlFreeDoc(doc); + return ret; +} + static const vshCmdOptDef opts_freecell[] = { {"cellno", VSH_OT_INT, 0, N_("NUMA cell number")}, {"all", VSH_OT_BOOL, 0, N_("show free memory for all NUMA cells")}, -- 1.7.3.5
2011/2/17 Michal Privoznik <mprivozn@redhat.com>:
Parse XML from given buffer, select appropiate XPath node and return its value as unsigned long. --- tools/virsh.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 49 insertions(+), 0 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index 50d5e33..8804fc3 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -2270,6 +2270,55 @@ static const vshCmdInfo info_freecell[] = { {NULL, NULL} };
+static int +xPathULong(char *xml, char *xpath, unsigned long *value) { + + xmlDocPtr doc = NULL; + xmlXPathContextPtr ctxt = NULL; + xmlXPathObjectPtr obj = NULL; + int ret = -1; + + doc = xmlReadDoc((const xmlChar *) xml, "domain.xml", NULL, + XML_PARSE_NOENT | XML_PARSE_NONET | XML_PARSE_NOWARNING); + if (!doc) + goto cleanup; + + ctxt = xmlXPathNewContext(doc); + if (!ctxt) + goto cleanup; + + obj = xmlXPathEval(BAD_CAST xpath, ctxt); + if (!obj) + goto cleanup; + + if (!value) + goto cleanup; + + *value = 0; + if ((obj->type == XPATH_STRING) && + (obj->stringval != NULL) && (obj->stringval[0] != 0)) { + char *conv = NULL; + unsigned long val; + + val = strtoul((const char *) obj->stringval, &conv, 10); + if (*conv =='\0') { + *value = val; + ret = 0; + } + } else if ((obj != NULL) && (obj->type == XPATH_NUMBER)) { + *value = (unsigned long) obj->floatval; + if (*value == obj->floatval) { + ret = 0; + } + } + +cleanup: + xmlXPathFreeObject(obj); + xmlXPathFreeContext(ctxt); + xmlFreeDoc(doc); + return ret; +} + static const vshCmdOptDef opts_freecell[] = { {"cellno", VSH_OT_INT, 0, N_("NUMA cell number")}, {"all", VSH_OT_BOOL, 0, N_("show free memory for all NUMA cells")}, -- 1.7.3.5
NACK, there already is a virXPathULong function. Look at how other virXPath* functions are used in virsh. Matthias
as written in docs, one must parse capabilities XML to get the right count of NUMA nodes. --- tools/virsh.c | 27 ++++++++++++++++----------- 1 files changed, 16 insertions(+), 11 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 8804fc3..1086555 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -2333,8 +2333,9 @@ cmdFreecell(vshControl *ctl, const vshCmd *cmd) int cell, cell_given; unsigned long long memory; unsigned long long *nodes = NULL; + unsigned long nodes_cnt; int all_given; - virNodeInfo info; + char *node_xml; if (!vshConnectionUsability(ctl, ctl->conn)) @@ -2350,30 +2351,33 @@ cmdFreecell(vshControl *ctl, const vshCmd *cmd) } if (all_given) { - if (virNodeGetInfo(ctl->conn, &info) < 0) { - vshError(ctl, "%s", _("failed to get NUMA nodes count")); + node_xml = virConnectGetCapabilities(ctl->conn); + if (!node_xml) { + vshError(ctl, "%s", _("unable to get node capabilities")); goto cleanup; } - if (!info.nodes) { - vshError(ctl, "%s", _("no NUMA nodes present")); - goto cleanup; - } + ret = xPathULong(node_xml, + (char*)"string(/capabilities/host/topology/cells/@num)", + &nodes_cnt); + + if (ret) + nodes_cnt = 1; - if (VIR_ALLOC_N(nodes, info.nodes) < 0) { + if (VIR_ALLOC_N(nodes, nodes_cnt) < 0) { vshError(ctl, "%s", _("could not allocate memory")); goto cleanup; } - ret = virNodeGetCellsFreeMemory(ctl->conn, nodes, 0, info.nodes); - if (ret != info.nodes) { + ret = virNodeGetCellsFreeMemory(ctl->conn, nodes, 0, nodes_cnt); + if (ret != nodes_cnt) { vshError(ctl, "%s", _("could not get information about " "all NUMA nodes")); goto cleanup; } memory = 0; - for (cell = 0; cell < info.nodes; cell++) { + for (cell = 0; cell < nodes_cnt; cell++) { vshPrint(ctl, "%5d: %10llu kB\n", cell, (nodes[cell]/1024)); memory += nodes[cell]; } @@ -2401,6 +2405,7 @@ cmdFreecell(vshControl *ctl, const vshCmd *cmd) cleanup: VIR_FREE(nodes); + VIR_FREE(node_xml); return func_ret; } -- 1.7.3.5
On 02/17/2011 06:39 AM, Michal Privoznik wrote:
as written in docs, one must parse capabilities XML to get the right count of NUMA nodes. --- tools/virsh.c | 27 ++++++++++++++++----------- 1 files changed, 16 insertions(+), 11 deletions(-)
- if (!info.nodes) { - vshError(ctl, "%s", _("no NUMA nodes present")); - goto cleanup; - } + ret = xPathULong(node_xml, + (char*)"string(/capabilities/host/topology/cells/@num)", + &nodes_cnt); + + if (ret) + nodes_cnt = 1;
As Matthias pointed out, we already have a parse function. If you ditch patch 1/2, then squash this into 2/2, does it all work for you (only compile-tested here)? diff --git i/tools/virsh.c w/tools/virsh.c index dd844ea..a2bff60 100644 --- i/tools/virsh.c +++ w/tools/virsh.c @@ -2287,7 +2287,8 @@ cmdFreecell(vshControl *ctl, const vshCmd *cmd) unsigned long nodes_cnt; int all_given; char *node_xml; - + xmlDocPtr xml = NULL; + xmlXPathContextPtr ctxt = NULL; if (!vshConnectionUsability(ctl, ctl->conn)) return FALSE; @@ -2308,12 +2309,24 @@ cmdFreecell(vshControl *ctl, const vshCmd *cmd) goto cleanup; } - ret = xPathULong(node_xml, - (char*)"string(/capabilities/host/topology/cells/@num)", - &nodes_cnt); + xml = xmlReadDoc((const xmlChar *) node_xml, "capabilities.xml", NULL, + XML_PARSE_NOENT | XML_PARSE_NONET | + XML_PARSE_NOWARNING); + if (!xml) + goto cleanup; + + ctxt = xmlXPathNewContext(xml); + if (!ctxt) + goto cleanup; - if (ret) + ret = virXPathULong("string(/capabilities/host/topology/cells/@num)", + ctxt, &nodes_cnt); + if (ret == -1) { nodes_cnt = 1; + } else if (ret < 0) { + vshError(ctl, "%s", _("unable to determine number of nodes")); + goto cleanup; + } if (VIR_ALLOC_N(nodes, nodes_cnt) < 0) { vshError(ctl, "%s", _("could not allocate memory")); @@ -2357,6 +2370,7 @@ cmdFreecell(vshControl *ctl, const vshCmd *cmd) cleanup: VIR_FREE(nodes); VIR_FREE(node_xml); + xmlXPathFreeContext(ctxt); return func_ret; } -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 02/17/2011 11:11 PM, Eric Blake wrote:
On 02/17/2011 06:39 AM, Michal Privoznik wrote:
...
As Matthias pointed out, we already have a parse function. If you ditch patch 1/2, then squash this into 2/2, does it all work for you (only compile-tested here)?
I've sent the second version, which uses the libvirt xml parsing functions. it was tested on some exotic architeture where NUMA nodes IDs are not continuous (hp-magnycours-02.rhts.eng.bos.redhat.com to be specific), but are something like 0,2,4,7. Michal
participants (4)
-
Eric Blake -
Matthias Bolte -
Michal Privoznik -
Michal Prívozník