[libvirt] [PATCH 02/22] virsh: Extract common code from cmdCPU{Compare, Baseline}

[Collin Walling]

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 at redhat.com>
> ---
>  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