$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@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@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@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__ */