$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@xxxxxxxxxx> > + */ > + > +#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@xxxxxxxxxx> > + */ > +#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__ */ > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
