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@xxxxxxxxx>
> ---
> 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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list