[libvirt] [PATCH v5 3/3] qemu: return balloon statistics when all domain statistics reported

[John Ferlan]

On 06/16/2016 02:38 PM, Maxim Nestratov wrote:
> From: Derbyshev Dmitry <dderbyshev at virtuozzo.com>
> 
> To collect all balloon statistics for all guests it was necessary to make
> several libvirt requests. Now it's possible to get all balloon statiscs via
> single connectGetAllDomainStats call.
> 
> Signed-off-by: Derbyshev Dmitry <dderbyshev at virtuozzo.com>
> Signed-off-by: Maxim Nestratov <mnestratov at virtuozzo.com>
> ---
>  src/qemu/qemu_driver.c | 95 +++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 74 insertions(+), 21 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 2885936..675ac5a 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -11073,32 +11073,23 @@ qemuDomainGetInterfaceParameters(virDomainPtr dom,
>      return ret;
>  }
>  
> +/* This functions assumes that job  QEMU_JOB_QUERY is started by a caller */
                                     ^
Extra space
> +
>  static int
> -qemuDomainMemoryStats(virDomainPtr dom,
> -                      virDomainMemoryStatPtr stats,
> -                      unsigned int nr_stats,
> -                      unsigned int flags)
> +qemuDomainMemoryStatsInternal(virQEMUDriverPtr driver,
> +                              virDomainObjPtr vm,
> +                              virDomainMemoryStatPtr stats,
> +                              unsigned int nr_stats)
> +
>  {
> -    virQEMUDriverPtr driver = dom->conn->privateData;
> -    virDomainObjPtr vm;
>      int ret = -1;
> +    qemuDomainObjPrivatePtr priv;

This fails to compile as 'priv' isn't used.

>      long rss;
>  
> -    virCheckFlags(0, -1);
> -
> -    if (!(vm = qemuDomObjFromDomain(dom)))
> -        goto cleanup;
> -
> -    if (virDomainMemoryStatsEnsureACL(dom->conn, vm->def) < 0)
> -        goto cleanup;
> -
> -    if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
> -        goto cleanup;
> -
>      if (!virDomainObjIsActive(vm)) {
>          virReportError(VIR_ERR_OPERATION_INVALID,
>                         "%s", _("domain is not running"));
> -        goto endjob;
> +        goto cleanup;

No need to cleanup, just return -1

>      }
>  
>      if (vm->def->memballoon &&
> @@ -11109,7 +11100,7 @@ qemuDomainMemoryStats(virDomainPtr dom,
>              ret = -1;
>  
>          if (ret < 0 || ret >= nr_stats)
> -            goto endjob;
> +            goto cleanup;

Likewise, return ret

>      } else {
>          ret = 0;
>      }
> @@ -11123,7 +11114,33 @@ qemuDomainMemoryStats(virDomainPtr dom,
>          ret++;
>      }
>  
> - endjob:
> + cleanup:
> +    return ret;

Rendering cleanup: unnecessary

> +}
> +
> +static int
> +qemuDomainMemoryStats(virDomainPtr dom,
> +                      virDomainMemoryStatPtr stats,
> +                      unsigned int nr_stats,
> +                      unsigned int flags)
> +{
> +    virQEMUDriverPtr driver = dom->conn->privateData;
> +    virDomainObjPtr vm;
> +    int ret = -1;
> +
> +    virCheckFlags(0, -1);
> +
> +    if (!(vm = qemuDomObjFromDomain(dom)))
> +        goto cleanup;
> +
> +    if (virDomainMemoryStatsEnsureACL(dom->conn, vm->def) < 0)
> +        goto cleanup;
> +
> +    if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
> +        goto cleanup;
> +
> +    ret = qemuDomainMemoryStatsInternal(driver, vm, stats, nr_stats);
> +
>      qemuDomainObjEndJob(driver, vm);
>  
>   cleanup:
> @@ -18608,15 +18625,29 @@ qemuDomainGetStatsCpu(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
>      return 0;
>  }
>  
> +
> +#define STORE_MEM_RECORD(TAG, NAME) {                  \
> +    if (stats[i].tag == VIR_DOMAIN_MEMORY_STAT_ ##TAG) \
> +        if (virTypedParamsAddULLong(&record->params,   \
> +                                    &record->nparams,  \
> +                                    maxparams,         \
> +                                    "balloon." NAME,   \
> +                                    stats[i].val) < 0) \
> +            return -1;                                 \
> +}
> +

All of the rest feels like a separate patch...

If I understand correctly this is for 'domstats' to display, correct?
If so, then virsh.pod could use an update.

>  static int
>  qemuDomainGetStatsBalloon(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,

This is now used too.

>                            virDomainObjPtr dom,
>                            virDomainStatsRecordPtr record,
>                            int *maxparams,
> -                          unsigned int privflags ATTRIBUTE_UNUSED)
> +                          unsigned int privflags)
>  {
>      qemuDomainObjPrivatePtr priv = dom->privateData;
> +    virDomainMemoryStatStruct stats[VIR_DOMAIN_MEMORY_STAT_NR];
> +    int nr_stats;
>      unsigned long long cur_balloon = 0;
> +    size_t i;
>      int err = 0;
>  
>      if (!virDomainDefHasMemballoon(dom->def)) {
> @@ -18641,8 +18672,30 @@ qemuDomainGetStatsBalloon(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
>                                  virDomainDefGetMemoryActual(dom->def)) < 0)
>          return -1;
>  

In order to get "balloon.current" 'err' must be 0, so what would allow
any of the following to be collected if err == -1?

> +    if (!HAVE_JOB(privflags))
> +        return 0;
> +
> +    nr_stats = qemuDomainMemoryStatsInternal(driver, dom, stats,
> +                                             VIR_DOMAIN_MEMORY_STAT_NR);

'driver' is now no longer unused it seems...

> +    if (nr_stats < 0)
> +        return 0;
> +
> +    for (i = 0; i < nr_stats; i++) {
> +        STORE_MEM_RECORD(SWAP_IN, "swap_in")
> +        STORE_MEM_RECORD(SWAP_OUT, "swap_out")
> +        STORE_MEM_RECORD(MAJOR_FAULT, "major_fault")
> +        STORE_MEM_RECORD(MINOR_FAULT, "minor_fault")
> +        STORE_MEM_RECORD(UNUSED, "unused")
> +        STORE_MEM_RECORD(AVAILABLE, "available")
> +        STORE_MEM_RECORD(ACTUAL_BALLOON, "actual")

Doesn't this ^ replicate "balloon.current"


John

> +        STORE_MEM_RECORD(RSS, "rss")
> +        STORE_MEM_RECORD(LAST_UPDATE, "last-update")
> +        STORE_MEM_RECORD(USABLE, "usable")
> +    }
> +
>      return 0;
>  }
> +#undef STORE_MEM_RECORD
>  
>  
>  static int
>