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

[Jiri Denemark]

On Tue, May 22, 2018 at 17:33:14 -0400, Collin Walling wrote:
> 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?

Because you can only have one root element in an XML document. Thus to
be able to parse a file with several root elements, we need to
encapsulate the content into a container.

> 
> > +    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?

The cpu-compare command is already documented this way. It accepts a CPU
definition and compares it to host CPU, while cpu-baseline accepts a set
of CPU definitions.

Jirka