[libvirt] [PATCH v2 5/8] perf: add new xml element

[Daniel P. Berrange]

On Mon, Dec 07, 2015 at 03:53:56PM +0800, Qiaowei Ren wrote:
> This patch adds new xml element, and so we can have the option of
> also having perf events enabled immediately at startup.
> 
> Signed-off-by: Qiaowei Ren <qiaowei.ren at intel.com>
> ---
>  docs/schemas/domaincommon.rng | 27 +++++++++++++++++++++++++++
>  src/conf/domain_conf.c        | 37 +++++++++++++++++++++++++++++++++++++
>  src/conf/domain_conf.h        | 10 ++++++++++
>  src/qemu/qemu_driver.c        | 26 ++++++++++++++++++++++++++
>  src/qemu/qemu_process.c       |  6 ++++--
>  5 files changed, 104 insertions(+), 2 deletions(-)

Needs an addition to the tests to exercise the XML round-trip
parse+format

> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 4804c69..fb4bf2b 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -393,6 +393,33 @@
>    </define>
>  
>    <!--
> +      Enable or disable perf events for the domain. For each
> +      of the events the following rules apply:
> +      on: the event will be forcefully enabled
> +      off: the event will be forcefully disabled
> +      not specified: the event will be disabled by default
> +  -->
> +  <define name="perf">
> +    <element name="perf">
> +      <interleave>
> +        <optional>
> +          <element name="cmt">
> +            <ref name="enableChoices"/>
> +          </element>
> +        </optional>
> +      </interleave>
> +      <empty/>
> +    </element>
> +  </define>
> +  <define name="enableChoices">
> +    <optional>
> +      <attribute name="enabled">
> +        <ref name="virYesNo"/>
> +      </attribute>
> +    </optional>
> +  </define>
> +

You have not used the 'perf' define anywhere, so this is currently
orphaned. Presumably you're adding this at the top level of the
<domain> XML. If you add an example XML file to the test suite then
you'd see a test error about this.

> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 2f5c0ed..833e69f 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -12231,6 +12231,29 @@ virDomainPMStateParseXML(xmlXPathContextPtr ctxt,
>  
>  
>  static int
> +virDomainPerfParseXML(xmlXPathContextPtr ctxt,
> +                      const char *xpath,
> +                      int *val)
> +{
> +    int ret = -1;
> +    char *tmp = virXPathString(xpath, ctxt);
> +    if (tmp) {
> +        *val = virTristateBoolTypeFromString(tmp);
> +        if (*val < 0) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                           _("unknown perf event state %s"), tmp);
> +            goto cleanup;
> +        }
> +    }
> +
> +    ret = 0;
> + cleanup:
> +    VIR_FREE(tmp);
> +    return ret;
> +}
> +
> +
> +static int
>  virDomainMemorySourceDefParseXML(xmlNodePtr node,
>                                   xmlXPathContextPtr ctxt,
>                                   virDomainMemoryDefPtr def)
> @@ -15388,6 +15411,11 @@ virDomainDefParseXML(xmlDocPtr xml,
>                                   &def->pm.s4) < 0)
>          goto error;
>  
> +    if (virDomainPerfParseXML(ctxt,
> +                                 "string(./perf/cmt/@enabled)",
> +                                 &def->perf.cmt) < 0)
> +        goto error;
> +
>      if ((tmp = virXPathString("string(./clock/@offset)", ctxt)) &&
>          (def->clock.offset = virDomainClockOffsetTypeFromString(tmp)) < 0) {
>          virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> @@ -22011,6 +22039,15 @@ virDomainDefFormatInternal(virDomainDefPtr def,
>          virBufferAddLit(buf, "</pm>\n");
>      }
>  
> +    if (def->perf.cmt) {
> +        virBufferAddLit(buf, "<perf>\n");
> +        virBufferAdjustIndent(buf, 2);
> +        virBufferAsprintf(buf, "<cmt enabled='%s'/>\n",
> +                          virTristateBoolTypeToString(def->perf.cmt));

It might be slightly nicer to have

   <event name="cmt" enabled="%s"/>

Also, we should probably have a  VIR_ENUM defined for VIR_PERF_EVENT*
types, so you can just do  virPerfEventTypeToString() to get the
value for the 'name' attribute. That would avoid us needing to
extend the parsing code every time we add a new event

> +        virBufferAdjustIndent(buf, -2);
> +        virBufferAddLit(buf, "</perf>\n");
> +    }
> +
>      virBufferAddLit(buf, "<devices>\n");
>      virBufferAdjustIndent(buf, 2);
>  
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 90d8e13..7383faf 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -2192,6 +2192,14 @@ struct _virDomainPowerManagement {
>      int s4;
>  };
>  
> +typedef struct _virDomainPerf virDomainPerf;
> +typedef virDomainPerf *virDomainPerfPtr;
> +
> +struct _virDomainPerf {
> +    /* These options are of type enum virTristateBool */
> +    int cmt;
> +};

This should be an array with size VIR_PERF_EVENT_LAST really,
so we don't hardcode 'cmt' in the parser.


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|