On 06/16/2016 02:38 PM, Maxim Nestratov wrote:
> From: Derbyshev Dmitry <dderbyshev@xxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxx>
> Signed-off-by: Maxim Nestratov <mnestratov@xxxxxxxxxxxxx>
> ---
> 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
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list