[libvirt] [PATCH 1/3] Extract stats functions from the qemu driver
John Ferlan
jferlan at redhat.com
Wed Mar 7 14:41:31 UTC 2018
$SUBJ, prefix with "conf:" would be nice.
On 03/05/2018 12:21 PM, Cédric Bosdonnat wrote:
> Some of the qemu functions getting statistics can easily be reused in
> other drivers. Create a conf/domain_stats.[ch] pair to host some of
> them.
> ---
> src/Makefile.am | 1 +
> src/conf/domain_stats.c | 139 +++++++++++++++++++++++++++++++++++++++++
> src/conf/domain_stats.h | 64 +++++++++++++++++++
> src/libvirt_private.syms | 4 ++
> src/qemu/qemu_driver.c | 158 +++--------------------------------------------
> src/util/vircgroup.c | 46 ++++++++++++++
> src/util/vircgroup.h | 4 ++
> 7 files changed, 265 insertions(+), 151 deletions(-)
> create mode 100644 src/conf/domain_stats.c
> create mode 100644 src/conf/domain_stats.h
>
May I suggest "git send-email --cover-letter..." when you have more than
one patch...
You will also need a S-O-B as we recently started to require all
contributors to indicate their compliance with the DCO
https://developercertificate.org/
you can do this by simply by adding 'Signed-off-by: Your Name
<your at email>' in commit message for patches you submit
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 3bf2da543..3952a6d2c 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
With commit 'ed30a13c4' this moves to src/conf/Makefile.inc.am
> @@ -441,6 +441,7 @@ DOMAIN_CONF_SOURCES = \
> conf/domain_conf.c conf/domain_conf.h \
> conf/domain_audit.c conf/domain_audit.h \
> conf/domain_nwfilter.c conf/domain_nwfilter.h \
> + conf/domain_stats.c conf/domain_stats.h \
> conf/virsavecookie.c conf/virsavecookie.h \
> conf/snapshot_conf.c conf/snapshot_conf.h \
> conf/numa_conf.c conf/numa_conf.h \
> diff --git a/src/conf/domain_stats.c b/src/conf/domain_stats.c
> new file mode 100644
> index 000000000..beb3c09d5
> --- /dev/null
> +++ b/src/conf/domain_stats.c
> @@ -0,0 +1,139 @@
> +/*
> + * domain_stats.c: domain stats extraction helpers
> + *
> + * Copyright (C) 2006-2016 Red Hat, Inc.
> + * Copyright (C) 2006-2008 Daniel P. Berrange
> + * Copyright (c) 2018 SUSE LINUX Products GmbH, Nuernberg, Germany.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library. If not, see
> + * <http://www.gnu.org/licenses/>.
> + *
> + * Author: Daniel P. Berrange <berrange at redhat.com>
> + */
> +
> +#include <config.h>
> +
> +#include <stdio.h>
> +
> +#include "virlog.h"
> +#include "domain_stats.h"
> +#include "virtypedparam.h"
> +#include "virnetdevtap.h"
> +#include "virnetdevopenvswitch.h"
> +
> +#define VIR_FROM_THIS VIR_FROM_DOMAIN
> +
> +VIR_LOG_INIT("conf.domain_stats");
> +
> +int
> +virDomainStatsGetState(virDomainObjPtr dom,
> + virDomainStatsRecordPtr record,
> + int *maxparams)
> +{
> + if (virTypedParamsAddInt(&record->params,
> + &record->nparams,
> + maxparams,
> + "state.state",
> + dom->state.state) < 0)
> + return -1;
> +
> + if (virTypedParamsAddInt(&record->params,
> + &record->nparams,
> + maxparams,
> + "state.reason",
> + dom->state.reason) < 0)
> + return -1;
> +
> + return 0;
> +}
For newer or moved code using 2 blank lines between functions has been
the "norm".
Also to make it easier to review - could the v2 add each "moved"
function one at a time here. Thus the above change would be separatable
and the next hunk in the "next" patch.
> +
> +#define STATS_ADD_NET_PARAM(record, maxparams, num, name, value) \
> +do { \
> + char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; \
> + snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, \
> + "net.%zu.%s", num, name); \
> + if (value >= 0 && virTypedParamsAddULLong(&(record)->params, \
> + &(record)->nparams, \
> + maxparams, \
> + param_name, \
> + value) < 0) \
> + return -1; \
> +} while (0)
> +
> +int
> +virDomainStatsGetInterface(virDomainObjPtr dom,
> + virDomainStatsRecordPtr record,
> + int *maxparams)
> +{
> + size_t i;
> + struct _virDomainInterfaceStats tmp;
> + int ret = -1;
> +
> + if (!virDomainObjIsActive(dom))
> + return 0;
> +
> + VIR_DOMAIN_STATS_ADD_COUNT_PARAM(record, maxparams, "net", dom->def->nnets);
> +
> + /* Check the path is one of the domain's network interfaces. */
> + for (i = 0; i < dom->def->nnets; i++) {
> + virDomainNetDefPtr net = dom->def->nets[i];
> + virDomainNetType actualType;
> +
> + if (!net->ifname)
> + continue;
> +
> + memset(&tmp, 0, sizeof(tmp));
> +
> + actualType = virDomainNetGetActualType(net);
> +
> + VIR_DOMAIN_STATS_ADD_NAME_PARAM(record, maxparams,
> + "net", "name", i, net->ifname);
^^^ ->
Indention is off here
> +
> + if (actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER) {
> + if (virNetDevOpenvswitchInterfaceStats(net->ifname, &tmp) < 0) {
> + virResetLastError();
> + continue;
> + }
> + } else {
> + if (virNetDevTapInterfaceStats(net->ifname, &tmp,
> + !virDomainNetTypeSharesHostView(net)) < 0) {
> + virResetLastError();
> + continue;
> + }
> + }
> +
> + STATS_ADD_NET_PARAM(record, maxparams, i,
> + "rx.bytes", tmp.rx_bytes);
> + STATS_ADD_NET_PARAM(record, maxparams, i,
> + "rx.pkts", tmp.rx_packets);
> + STATS_ADD_NET_PARAM(record, maxparams, i,
> + "rx.errs", tmp.rx_errs);
> + STATS_ADD_NET_PARAM(record, maxparams, i,
> + "rx.drop", tmp.rx_drop);
> + STATS_ADD_NET_PARAM(record, maxparams, i,
> + "tx.bytes", tmp.tx_bytes);
> + STATS_ADD_NET_PARAM(record, maxparams, i,
> + "tx.pkts", tmp.tx_packets);
> + STATS_ADD_NET_PARAM(record, maxparams, i,
> + "tx.errs", tmp.tx_errs);
> + STATS_ADD_NET_PARAM(record, maxparams, i,
> + "tx.drop", tmp.tx_drop);
> + }
> +
> + ret = 0;
> + cleanup:
> + return ret;
> +}
> +
> +#undef STATS_ADD_NET_PARAM
> diff --git a/src/conf/domain_stats.h b/src/conf/domain_stats.h
> new file mode 100644
> index 000000000..42f8cb6d3
> --- /dev/null
> +++ b/src/conf/domain_stats.h
> @@ -0,0 +1,64 @@
> +/*
> + * domain_stats.h: domain stats extraction helpers
> + *
> + * Copyright (C) 2006-2016 Red Hat, Inc.
> + * Copyright (C) 2006-2008 Daniel P. Berrange
> + * Copyright (c) 2018 SUSE LINUX Products GmbH, Nuernberg, Germany.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library. If not, see
> + * <http://www.gnu.org/licenses/>.
> + *
> + * Author: Daniel P. Berrange <berrange at redhat.com>
> + */
> +#ifndef __DOMAIN_STATS_H
> +# define __DOMAIN_STATS_H
> +
> +# include "internal.h"
> +# include "domain_conf.h"
> +
> +
> +# define VIR_DOMAIN_STATS_ADD_COUNT_PARAM(record, maxparams, type, count) \
> +do { \
> + char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; \
> + snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, "%s.count", type); \
> + if (virTypedParamsAddUInt(&(record)->params, \
> + &(record)->nparams, \
> + maxparams, \
> + param_name, \
> + count) < 0) \
> + goto cleanup; \
> +} while (0)
> +
> +# define VIR_DOMAIN_STATS_ADD_NAME_PARAM(record, maxparams, type, subtype, num, name) \
> +do { \
> + char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; \
> + snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, \
> + "%s.%zu.%s", type, num, subtype); \
> + if (virTypedParamsAddString(&(record)->params, \
> + &(record)->nparams, \
> + maxparams, \
> + param_name, \
> + name) < 0) \
> + goto cleanup; \
> +} while (0)
I have "mixed" feelings about having a goto within the macro, but the
compiler will complain rather vehmently if a cleanup: label doesn't
exist when the macro is used.
When I've seen it used before in *.h files, the goto target is a
parameter to the macro (using cscope search on "goto.*\\" and various .h
files).
> +
> +int virDomainStatsGetState(virDomainObjPtr dom,
> + virDomainStatsRecordPtr record,
> + int *maxparams);
> +
> +int virDomainStatsGetInterface(virDomainObjPtr dom,
> + virDomainStatsRecordPtr record,
> + int *maxparams);
> +
> +#endif /* __DOMAIN_STATS_H */
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 8a62ea159..6fb5b6945 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -644,6 +644,9 @@ virDomainConfNWFilterRegister;
> virDomainConfNWFilterTeardown;
> virDomainConfVMNWFilterTeardown;
>
> +# conf/domain_stats.h
> +virDomainStatsGetInterface;
> +virDomainStatsGetState;
>
> # conf/interface_conf.h
> virInterfaceDefFormat;
> @@ -1500,6 +1503,7 @@ virCgroupGetMemoryUsage;
> virCgroupGetMemSwapHardLimit;
> virCgroupGetMemSwapUsage;
> virCgroupGetPercpuStats;
> +virCgroupGetStatsCpu;
> virCgroupHasController;
> virCgroupHasEmptyTasks;
> virCgroupKill;
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 96454c17c..d2e6c0ebe 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -72,6 +72,7 @@
> #include "viralloc.h"
> #include "viruuid.h"
> #include "domain_conf.h"
> +#include "domain_stats.h"
> #include "domain_audit.h"
> #include "node_device_conf.h"
> #include "virpci.h"
> @@ -19510,21 +19511,7 @@ qemuDomainGetStatsState(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
> int *maxparams,
> unsigned int privflags ATTRIBUTE_UNUSED)
> {
> - if (virTypedParamsAddInt(&record->params,
> - &record->nparams,
> - maxparams,
> - "state.state",
> - dom->state.state) < 0)
> - return -1;
> -
> - if (virTypedParamsAddInt(&record->params,
> - &record->nparams,
> - maxparams,
> - "state.reason",
> - dom->state.reason) < 0)
> - return -1;
> -
> - return 0;
> + return virDomainStatsGetState(dom, record, maxparams);
> }
>
>
> @@ -19547,37 +19534,7 @@ qemuDomainGetStatsCpu(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
> unsigned int privflags ATTRIBUTE_UNUSED)
> {
> qemuDomainObjPrivatePtr priv = dom->privateData;
> - unsigned long long cpu_time = 0;
> - unsigned long long user_time = 0;
> - unsigned long long sys_time = 0;
> - int err = 0;
> -
> - if (!priv->cgroup)
> - return 0;
> -
> - err = virCgroupGetCpuacctUsage(priv->cgroup, &cpu_time);
> - if (!err && virTypedParamsAddULLong(&record->params,
> - &record->nparams,
> - maxparams,
> - "cpu.time",
> - cpu_time) < 0)
> - return -1;
> -
> - err = virCgroupGetCpuacctStat(priv->cgroup, &user_time, &sys_time);
> - if (!err && virTypedParamsAddULLong(&record->params,
> - &record->nparams,
> - maxparams,
> - "cpu.user",
> - user_time) < 0)
> - return -1;
> - if (!err && virTypedParamsAddULLong(&record->params,
> - &record->nparams,
> - maxparams,
> - "cpu.system",
> - sys_time) < 0)
> - return -1;
> -
> - return 0;
> + return virCgroupGetStatsCpu(priv->cgroup, record, maxparams);
This appears to be separable into it's own patch along the last hunk at
the bottom.
> }
>
> static int
> @@ -19756,44 +19713,6 @@ qemuDomainGetStatsVcpu(virQEMUDriverPtr driver,
> return ret;
> }
>
> -#define QEMU_ADD_COUNT_PARAM(record, maxparams, type, count) \
> -do { \
> - char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; \
> - snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, "%s.count", type); \
> - if (virTypedParamsAddUInt(&(record)->params, \
> - &(record)->nparams, \
> - maxparams, \
> - param_name, \
> - count) < 0) \
> - goto cleanup; \
> -} while (0)
> -
> -#define QEMU_ADD_NAME_PARAM(record, maxparams, type, subtype, num, name) \
> -do { \
> - char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; \
> - snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, \
> - "%s.%zu.%s", type, num, subtype); \
> - if (virTypedParamsAddString(&(record)->params, \
> - &(record)->nparams, \
> - maxparams, \
> - param_name, \
> - name) < 0) \
> - goto cleanup; \
> -} while (0)
> -
> -#define QEMU_ADD_NET_PARAM(record, maxparams, num, name, value) \
> -do { \
> - char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; \
> - snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, \
> - "net.%zu.%s", num, name); \
> - if (value >= 0 && virTypedParamsAddULLong(&(record)->params, \
> - &(record)->nparams, \
> - maxparams, \
> - param_name, \
> - value) < 0) \
> - return -1; \
> -} while (0)
> -
> static int
> qemuDomainGetStatsInterface(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
> virDomainObjPtr dom,
> @@ -19801,68 +19720,9 @@ qemuDomainGetStatsInterface(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
> int *maxparams,
> unsigned int privflags ATTRIBUTE_UNUSED)
> {
> - size_t i;
> - struct _virDomainInterfaceStats tmp;
> - int ret = -1;
> -
> - if (!virDomainObjIsActive(dom))
> - return 0;
> -
> - QEMU_ADD_COUNT_PARAM(record, maxparams, "net", dom->def->nnets);
> -
> - /* Check the path is one of the domain's network interfaces. */
> - for (i = 0; i < dom->def->nnets; i++) {
> - virDomainNetDefPtr net = dom->def->nets[i];
> - virDomainNetType actualType;
> -
> - if (!net->ifname)
> - continue;
> -
> - memset(&tmp, 0, sizeof(tmp));
> -
> - actualType = virDomainNetGetActualType(net);
> -
> - QEMU_ADD_NAME_PARAM(record, maxparams,
> - "net", "name", i, net->ifname);
> -
> - if (actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER) {
> - if (virNetDevOpenvswitchInterfaceStats(net->ifname, &tmp) < 0) {
> - virResetLastError();
> - continue;
> - }
> - } else {
> - if (virNetDevTapInterfaceStats(net->ifname, &tmp,
> - !virDomainNetTypeSharesHostView(net)) < 0) {
> - virResetLastError();
> - continue;
> - }
> - }
> -
> - QEMU_ADD_NET_PARAM(record, maxparams, i,
> - "rx.bytes", tmp.rx_bytes);
> - QEMU_ADD_NET_PARAM(record, maxparams, i,
> - "rx.pkts", tmp.rx_packets);
> - QEMU_ADD_NET_PARAM(record, maxparams, i,
> - "rx.errs", tmp.rx_errs);
> - QEMU_ADD_NET_PARAM(record, maxparams, i,
> - "rx.drop", tmp.rx_drop);
> - QEMU_ADD_NET_PARAM(record, maxparams, i,
> - "tx.bytes", tmp.tx_bytes);
> - QEMU_ADD_NET_PARAM(record, maxparams, i,
> - "tx.pkts", tmp.tx_packets);
> - QEMU_ADD_NET_PARAM(record, maxparams, i,
> - "tx.errs", tmp.tx_errs);
> - QEMU_ADD_NET_PARAM(record, maxparams, i,
> - "tx.drop", tmp.tx_drop);
> - }
> -
> - ret = 0;
> - cleanup:
> - return ret;
> + return virDomainStatsGetInterface(dom, record, maxparams);
> }
>
> -#undef QEMU_ADD_NET_PARAM
> -
> #define QEMU_ADD_BLOCK_PARAM_UI(record, maxparams, num, name, value) \
> do { \
> char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; \
> @@ -19984,10 +19844,10 @@ qemuDomainGetStatsOneBlock(virQEMUDriverPtr driver,
> if (disk->info.alias)
> alias = qemuDomainStorageAlias(disk->info.alias, backing_idx);
>
> - QEMU_ADD_NAME_PARAM(record, maxparams, "block", "name", block_idx,
> + VIR_DOMAIN_STATS_ADD_NAME_PARAM(record, maxparams, "block", "name", block_idx,
> disk->dst);
^^^ ->
The indention ends up being off here...
> if (virStorageSourceIsLocalStorage(src) && src->path)
> - QEMU_ADD_NAME_PARAM(record, maxparams, "block", "path",
> + VIR_DOMAIN_STATS_ADD_NAME_PARAM(record, maxparams, "block", "path",
> block_idx, src->path);
^^^ ->
... and here.
> if (backing_idx)
> QEMU_ADD_BLOCK_PARAM_UI(record, maxparams, block_idx, "backingIndex",
> @@ -20103,7 +19963,7 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver,
> * after the iteration than it is to iterate twice; but we still
> * want count listed first. */
> count_index = record->nparams;
> - QEMU_ADD_COUNT_PARAM(record, maxparams, "block", 0);
> + VIR_DOMAIN_STATS_ADD_COUNT_PARAM(record, maxparams, "block", 0);
>
> for (i = 0; i < dom->def->ndisks; i++) {
> virDomainDiskDefPtr disk = dom->def->disks[i];
> @@ -20137,10 +19997,6 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver,
>
> #undef QEMU_ADD_BLOCK_PARAM_ULL
>
> -#undef QEMU_ADD_NAME_PARAM
> -
> -#undef QEMU_ADD_COUNT_PARAM
> -
> static int
> qemuDomainGetStatsPerfOneEvent(virPerfPtr perf,
> virPerfEventType type,
The hunk below here could be its own patch.
John
> diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
> index 0a31947b0..04ef4c1a7 100644
> --- a/src/util/vircgroup.c
> +++ b/src/util/vircgroup.c
> @@ -4122,6 +4122,44 @@ virCgroupControllerAvailable(int controller)
> return ret;
> }
>
> +int
> +virCgroupGetStatsCpu(virCgroupPtr cgroup,
> + virDomainStatsRecordPtr record,
> + int *maxparams)
> +{
> + unsigned long long cpu_time = 0;
> + unsigned long long user_time = 0;
> + unsigned long long sys_time = 0;
> + int err = 0;
> +
> + if (!cgroup)
> + return 0;
> +
> + err = virCgroupGetCpuacctUsage(cgroup, &cpu_time);
> + if (!err && virTypedParamsAddULLong(&record->params,
> + &record->nparams,
> + maxparams,
> + "cpu.time",
> + cpu_time) < 0)
> + return -1;
> +
> + err = virCgroupGetCpuacctStat(cgroup, &user_time, &sys_time);
> + if (!err && virTypedParamsAddULLong(&record->params,
> + &record->nparams,
> + maxparams,
> + "cpu.user",
> + user_time) < 0)
> + return -1;
> + if (!err && virTypedParamsAddULLong(&record->params,
> + &record->nparams,
> + maxparams,
> + "cpu.system",
> + sys_time) < 0)
> + return -1;
> +
> + return 0;
> +}
> +
> #else /* !VIR_CGROUP_SUPPORTED */
>
> bool
> @@ -4899,6 +4937,14 @@ virCgroupControllerAvailable(int controller ATTRIBUTE_UNUSED)
> {
> return false;
> }
> +
> +int
> +virCgroupGetStatsCpu(virCgroupPtr cgroup,
> + virDomainStatsRecordPtr record,
> + int *maxparams)
> +{
> + return 0;
> +}
> #endif /* !VIR_CGROUP_SUPPORTED */
>
>
> diff --git a/src/util/vircgroup.h b/src/util/vircgroup.h
> index d83392767..2ebdf9505 100644
> --- a/src/util/vircgroup.h
> +++ b/src/util/vircgroup.h
> @@ -297,4 +297,8 @@ int virCgroupSetOwner(virCgroupPtr cgroup,
> int virCgroupHasEmptyTasks(virCgroupPtr cgroup, int controller);
>
> bool virCgroupControllerAvailable(int controller);
> +
> +int virCgroupGetStatsCpu(virCgroupPtr cgroup,
> + virDomainStatsRecordPtr record,
> + int *maxparams);
> #endif /* __VIR_CGROUP_H__ */
>
More information about the libvir-list
mailing list