[libvirt] [PATCH 4/8] Add 'period' for Memballoon statistics gathering capability

[Michal Privoznik]

On 02.07.2013 15:39, John Ferlan wrote:
> Add a period in seconds to allow/enable statistics gathering from the
> Balloon driver for 'virsh dommemstat <domain>'.
> ---
>  docs/schemas/domaincommon.rng |  7 +++++++
>  src/conf/domain_conf.c        | 27 +++++++++++++++++++++++----
>  src/conf/domain_conf.h        |  1 +
>  3 files changed, 31 insertions(+), 4 deletions(-)
> 
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index cf82878..53e707c 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -2915,6 +2915,13 @@
>        <optional>
>          <ref name="address"/>
>        </optional>
> +      <optional>
> +        <element name="stats">
> +          <attribute name="period">
> +            <ref name="positiveInteger"/>
> +          </attribute>
> +        </element>
> +      </optional>
>      </element>
>    </define>
>    <define name="parallel">
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 011de71..cd7dbee 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -8571,10 +8571,12 @@ error:
>  
>  static virDomainMemballoonDefPtr
>  virDomainMemballoonDefParseXML(const xmlNodePtr node,
> +                               xmlXPathContextPtr ctxt,
>                                 unsigned int flags)
>  {
>      char *model;
>      virDomainMemballoonDefPtr def;
> +    xmlNodePtr save = ctxt->node;
>  
>      if (VIR_ALLOC(def) < 0) {
>          virReportOOMError();
> @@ -8587,18 +8589,24 @@ virDomainMemballoonDefParseXML(const xmlNodePtr node,
>                         _("balloon memory must contain model name"));
>          goto error;
>      }
> +
>      if ((def->model = virDomainMemballoonModelTypeFromString(model)) < 0) {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
>                         _("unknown memory balloon model '%s'"), model);
>          goto error;
>      }
>  
> +    ctxt->node = node;
> +    if (virXPathInt("string(./stats/@period)", ctxt, &def->period) < 0)
> +        def->period = 0;

This is silently ignoring invalid integer value in @period. Moreover,
it's merging two different cases into one: invalid integer and @period
not present. After all, setting def->period to zero is done by
VIR_ALLOC. So I think we must distinguish if virXPathInt returned -1
(@period not present in XML) or -2(@period is not valid integer). And we
must not forget to check if user hasn't entered period < 0.

> +
>      if (virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0)
>          goto error;
>  
>  cleanup:
>      VIR_FREE(model);
>  
> +    ctxt->node = save;
>      return def;
>  
>  error:
> @@ -11911,7 +11919,7 @@ virDomainDefParseXML(xmlDocPtr xml,
>      }
>      if (n > 0) {
>          virDomainMemballoonDefPtr memballoon =
> -            virDomainMemballoonDefParseXML(nodes[0], flags);
> +            virDomainMemballoonDefParseXML(nodes[0], ctxt, flags);
>          if (!memballoon)
>              goto error;
>  
> @@ -15080,6 +15088,7 @@ virDomainMemballoonDefFormat(virBufferPtr buf,
>                               unsigned int flags)
>  {
>      const char *model = virDomainMemballoonModelTypeToString(def->model);
> +    bool noopts = true;
>  
>      if (!model) {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -15093,11 +15102,21 @@ virDomainMemballoonDefFormat(virBufferPtr buf,
>          virBufferAddLit(buf, ">\n");
>          if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0)
>              return -1;
> -        virBufferAddLit(buf, "    </memballoon>\n");
> -    } else {
> -        virBufferAddLit(buf, "/>\n");
> +        noopts = false;
>      }
>  
> +    if (def->period) {
> +        if (noopts)
> +            virBufferAddLit(buf, ">\n");
> +        virBufferAsprintf(buf, "      <stats period='%d'/>\n", def->period);
> +        noopts = false;
> +    }
> +
> +    if (noopts)
> +        virBufferAddLit(buf, "/>\n");
> +    else
> +        virBufferAddLit(buf, "    </memballoon>\n");
> +
>      return 0;
>  }
>  
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 3817e37..405903f 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1527,6 +1527,7 @@ enum {
>  struct _virDomainMemballoonDef {
>      int model;
>      virDomainDeviceInfo info;
> +    int period; /* seconds between collections */
>  };
>  
>  struct _virDomainNVRAMDef {
> 

Michal