On 05/16/2018 04:39 AM, Jiri Denemark wrote:
> Both cpu-compare and cpu-baseline commands accept more that just CPU
> definition XML(s). For users' convenience they are able to extract the
> CPU definition(s) even from domain XML or capabilities XML. The main
> differences between the two commands is in the number of CPU definitions
> they expect: cpu-compare wants only one CPU definition while
> cpu-baseline expects one or more CPUs.
>
> The extracted code forms a new vshExtractCPUDefXML function.
>
> Signed-off-by: Jiri Denemark <jdenemar@xxxxxxxxxx>
> ---
> tools/virsh-host.c | 160 +++++++++++++++++++++------------------------
> 1 file changed, 75 insertions(+), 85 deletions(-)
>
> diff --git a/tools/virsh-host.c b/tools/virsh-host.c
> index 6d6e3cfc85..51497db385 100644
> --- a/tools/virsh-host.c
> +++ b/tools/virsh-host.c
> @@ -1106,6 +1106,72 @@ cmdURI(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED)
> return true;
> }
>
> +
> +/* Extracts the CPU definition XML strings from a file which may contain either
> + * - just the CPU definitions,
> + * - domain XMLs, or
> + * - capabilities XMLs.
> + *
> + * Returns NULL terminated string list.
> + */
> +static char **
> +vshExtractCPUDefXMLs(vshControl *ctl,
> + const char *xmlFile)
> +{
> + char **cpus = NULL;
> + char *buffer = NULL;
> + char *xmlStr = NULL;
> + xmlDocPtr xml = NULL;
> + xmlXPathContextPtr ctxt = NULL;
> + xmlNodePtr *nodes = NULL;
> + size_t i;
> + int n;
> +
> + if (virFileReadAll(xmlFile, VSH_MAX_XML_FILE, &buffer) < 0)
> + goto error;
> +
> + if (virAsprintf(&xmlStr, "<container>%s</container>", buffer) < 0)
> + goto error;
> +
Why wrap the xml in the <container> tags?
> + if (!(xml = virXMLParseStringCtxt(xmlStr, xmlFile, &ctxt)))
> + goto error;
> +
> + n = virXPathNodeSet("/container/cpu|"
> + "/container/domain/cpu|"
> + "/container/capabilities/host/cpu",
> + ctxt, &nodes);
> + if (n < 0)
> + goto error;
> +
> + if (n == 0) {
> + vshError(ctl, _("File '%s' does not contain any <cpu> element or "
> + "valid domain or capabilities XML"), xmlFile);
> + goto error;
> + }
> +
> + cpus = vshCalloc(ctl, n + 1, sizeof(const char *));
> +
> + for (i = 0; i < n; i++) {
> + if (!(cpus[i] = virXMLNodeToString(xml, nodes[i]))) {
> + vshSaveLibvirtError();
> + goto error;
> + }
> + }
> +
> + cleanup:
> + VIR_FREE(buffer);
> + VIR_FREE(xmlStr);
> + xmlFreeDoc(xml);
> + xmlXPathFreeContext(ctxt);
> + VIR_FREE(nodes);
> + return cpus;
> +
> + error:
> + virStringListFree(cpus);
> + goto cleanup;
> +}
> +
> +
> /*
> * "cpu-compare" command
> */
> @@ -1133,13 +1199,9 @@ cmdCPUCompare(vshControl *ctl, const vshCmd *cmd)
> {
> const char *from = NULL;
> bool ret = false;
> - char *buffer;
> int result;
> - char *snippet = NULL;
> + char **cpus = NULL;
> unsigned int flags = 0;
> - xmlDocPtr xml = NULL;
> - xmlXPathContextPtr ctxt = NULL;
> - xmlNodePtr node;
> virshControlPtr priv = ctl->privData;
>
> if (vshCommandOptBool(cmd, "error"))
> @@ -1148,27 +1210,10 @@ cmdCPUCompare(vshControl *ctl, const vshCmd *cmd)
> if (vshCommandOptStringReq(ctl, cmd, "file", &from) < 0)
> return false;
>
> - if (virFileReadAll(from, VSH_MAX_XML_FILE, &buffer) < 0)
> + if (!(cpus = vshExtractCPUDefXMLs(ctl, from)))
> return false;
>
> - /* try to extract the CPU element from as it would appear in a domain XML*/
> - if (!(xml = virXMLParseStringCtxt(buffer, from, &ctxt)))
> - goto cleanup;
> -
> - if ((node = virXPathNode("/cpu|"
> - "/domain/cpu|"
> - "/capabilities/host/cpu", ctxt))) {
> - if (!(snippet = virXMLNodeToString(xml, node))) {
> - vshSaveLibvirtError();
> - goto cleanup;
> - }
> - } else {
> - vshError(ctl, _("File '%s' does not contain a <cpu> element or is not "
> - "a valid domain or capabilities XML"), from);
> - goto cleanup;
> - }
> -
> - result = virConnectCompareCPU(priv->conn, snippet, flags);
> + result = virConnectCompareCPU(priv->conn, cpus[0], flags);
I wonder if it's worth commenting here or adding to the cpu compare docs that comparison only
compares the host CPU with the _first_ cpu found in the XML file?
>
> switch (result) {
> case VIR_CPU_COMPARE_INCOMPATIBLE:
> @@ -1196,10 +1241,7 @@ cmdCPUCompare(vshControl *ctl, const vshCmd *cmd)
> ret = true;
>
> cleanup:
> - VIR_FREE(buffer);
> - VIR_FREE(snippet);
> - xmlXPathFreeContext(ctxt);
> - xmlFreeDoc(xml);
> + virStringListFree(cpus);
>
> return ret;
> }
[...]
The rest of this patch looks good... I'd like to take another look tomorrow in case I missed
anything subtle.
--
Respectfully,
- Collin Walling
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
