[libvirt] [PATCH v2 4/4] conf: Use correct type for balloon stats period

[Erik Skultety]

On 03/13/2015 05:17 PM, Martin Kletzander wrote:
> We're parsing memballoon status period as unsigned int, but when we're
> trying to set it, both we and qemu use signed int.  That means large
> values will get wrapped around to negative one resulting in error.
> Basically the same problem as commit e3a7b874 was dealing with when
> updating live domain.
> 
> QEMU changed the accepted value to int64 in commit 1f9296b5, but even
> values as INT_MAX don't make sense since the value passed means seconds.
> Hence adding capability flag for this change isn't worth it.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1140958
> 
> Signed-off-by: Luyao Huang <lhuang at redhat.com>
> Signed-off-by: Martin Kletzander <mkletzan at redhat.com>
> ---
>  docs/formatdomain.html.in | 2 ++
>  src/conf/domain_conf.c    | 9 +++++++--
>  src/conf/domain_conf.h    | 2 +-
>  src/qemu/qemu_process.c   | 2 +-
>  4 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 40e2b29..7a11cc7 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -5630,6 +5630,8 @@ qemu-kvm -net nic,model=? /dev/null
>            only be made to the active guest.
>            If the QEMU driver is not at the right
>            revision, the attempt to set the period will fail.
> +          Large values might be ignored, but this only affects
> +          non-sensical numbers (i.e. many years).
>            <span class='since'>Since 1.1.1, requires QEMU 1.5</span>
>          </p>
>        </dd>

Just a nitpick, I'd probably avoid word construction non-sensical in our
docs (it's not even correct --> nonsensical) and simplify this to "Large
values (i.e. many years) might be ignored."

> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index e010040..b3d9999 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -10432,6 +10432,7 @@ virDomainMemballoonDefParseXML(xmlNodePtr node,
>      char *model;
>      virDomainMemballoonDefPtr def;
>      xmlNodePtr save = ctxt->node;
> +    unsigned int period = 0;
> 
>      if (VIR_ALLOC(def) < 0)
>          return NULL;
> @@ -10450,12 +10451,16 @@ virDomainMemballoonDefParseXML(xmlNodePtr node,
>      }
> 
>      ctxt->node = node;
> -    if (virXPathUInt("string(./stats/@period)", ctxt, &def->period) < -1) {
> +    if (virXPathUInt("string(./stats/@period)", ctxt, &period) < -1) {
>          virReportError(VIR_ERR_XML_ERROR, "%s",
>                         _("invalid statistics collection period"));
>          goto error;
>      }
> 
> +    def->period = period;
> +    if (def->period < 0)
> +        def->period = 0;
> +
>      if (def->model == VIR_DOMAIN_MEMBALLOON_MODEL_NONE)
>          VIR_DEBUG("Ignoring device address for none model Memballoon");
>      else if (virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0)
> @@ -18823,7 +18828,7 @@ virDomainMemballoonDefFormat(virBufferPtr buf,
>      virBufferAdjustIndent(&childrenBuf, indent + 2);
> 
>      if (def->period)
> -        virBufferAsprintf(&childrenBuf, "<stats period='%u'/>\n", def->period);
> +        virBufferAsprintf(&childrenBuf, "<stats period='%i'/>\n", def->period);
> 
>      if (virDomainDeviceInfoNeedsFormat(&def->info, flags) &&
>          virDomainDeviceInfoFormat(&childrenBuf, &def->info, flags) < 0) {
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index ea463cb..ee0f5fd 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1556,7 +1556,7 @@ enum {
>  struct _virDomainMemballoonDef {
>      int model;
>      virDomainDeviceInfo info;
> -    unsigned int period; /* seconds between collections */
> +    int period; /* seconds between collections */
>  };
> 
>  struct _virDomainNVRAMDef {
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index d1f089d..0f357d5 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -4390,7 +4390,7 @@ int qemuProcessStart(virConnectPtr conn,
>      virCommandPtr cmd = NULL;
>      struct qemuProcessHookData hookData;
>      unsigned long cur_balloon;
> -    unsigned int period = 0;
> +    int period = 0;
>      size_t i;
>      bool rawio_set = false;
>      char *nodeset = NULL;
> --
> 2.3.2
> 
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
> 

ACK with the nitpick fixed.
Erik