On 11/18/2015 10:03 PM, Jim Fehlig wrote:
> On 11/13/2015 06:14 AM, Joao Martins wrote:
>> Introduce support for connectGetAllDomainStats call that
>> allow us to _all_ domain(s) statistics including network, block,
>
> allows us to get
>
>> cpus and memory. Changes are rather mechanical and mostly
>> take care of the format to export the data.
>>
>> Signed-off-by: Joao Martins <joao.m.martins@xxxxxxxxxx>
>> ---
>> Changes since v1:
>> - Rework flags checking on libxlDomainGetStats
>> for VIR_DOMAIN_STATS_{VCPU,INTERFACE,BLOCK}
>> - Removed path since we are reusing <virDomainNetDef>.ifname
>> - Init dominfo and dispose it on cleanup.
>> - Fixed VIR_FREE issue that was reported with make syntax-check"
>> - Bump version to 1.2.22
>> ---
>> src/libxl/libxl_driver.c | 266 +++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 266 insertions(+)
>>
>> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
>> index ba1d67b..8db6536 100644
>> --- a/src/libxl/libxl_driver.c
>> +++ b/src/libxl/libxl_driver.c
>> @@ -5238,6 +5238,271 @@ libxlDomainMemoryStats(virDomainPtr dom,
>>
>> #undef LIBXL_SET_MEMSTAT
>>
>> +#define LIBXL_RECORD_UINT(error, key, value, ...) \
>> +do { \
>> + char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; \
>> + snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, \
>> + key, ##__VA_ARGS__); \
>> + if (virTypedParamsAddUInt(&tmp->params, \
>> + &tmp->nparams, \
>> + &maxparams, \
>> + param_name, \
>> + value) < 0) \
>> + goto error; \
>> +} while (0)
>> +
>> +#define LIBXL_RECORD_LL(error, key, value, ...) \
>> +do { \
>> + char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; \
>> + snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, \
>> + key, ##__VA_ARGS__); \
>> + if (virTypedParamsAddLLong(&tmp->params, \
>> + &tmp->nparams, \
>> + &maxparams, \
>> + param_name, \
>> + value) < 0) \
>> + goto error; \
>> +} while (0)
>> +
>> +#define LIBXL_RECORD_ULL(error, key, value, ...) \
>> +do { \
>> + char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; \
>> + snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, \
>> + key, ##__VA_ARGS__); \
>> + if (virTypedParamsAddULLong(&tmp->params, \
>> + &tmp->nparams, \
>> + &maxparams, \
>> + param_name, \
>> + value) < 0) \
>> + goto error; \
>> +} while (0)
>> +
>> +#define LIBXL_RECORD_STR(error, key, value, ...) \
>> +do { \
>> + char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; \
>> + snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, \
>> + key, ##__VA_ARGS__); \
>> + if (virTypedParamsAddString(&tmp->params, \
>> + &tmp->nparams, \
>> + &maxparams, \
>> + param_name, \
>> + value) < 0) \
>> + goto error; \
>> +} while (0)
>> +
>> +static int
>> +libxlDomainGetStats(virConnectPtr conn,
>> + virDomainObjPtr dom,
>> + unsigned int stats,
>> + virDomainStatsRecordPtr *record)
>> +{
>> + libxlDriverPrivatePtr driver = conn->privateData;
>> + libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver);
>
> cfg needs to be unref'ed when this function terminates. Sadly I noticed this
> only after pushing some of the other patches with a similar flaw.
>
OK, will fix that. I saw and tested your fix series, and it appears both of them
are Acked already.
>> + virDomainStatsRecordPtr tmp;
>> + libxl_dominfo d_info;
>> + libxl_vcpuinfo *vcpuinfo = NULL;
>> + int maxcpu, hostcpus;
>> + unsigned long long mem, maxmem;
>> + int maxparams = 0;
>> + int ret = -1;
>> + size_t i, state;
>> + unsigned int domflags = stats & (VIR_DOMAIN_STATS_BALLOON |
>> + VIR_DOMAIN_STATS_CPU_TOTAL);
>> +
>> + if (VIR_ALLOC(tmp) < 0)
>> + return ret;
>> +
>> + libxl_dominfo_init(&d_info);
>> +
>> + mem = virDomainDefGetMemoryInitial(dom->def);
>> + maxmem = virDomainDefGetMemoryActual(dom->def);
>> + d_info.cpu_time = 0;
>> +
>> + if (domflags && virDomainObjIsActive(dom) &&
>> + !libxl_domain_info(cfg->ctx, &d_info, dom->def->id)) {
>> + mem = d_info.current_memkb;
>> + maxmem = d_info.max_memkb;
>> + }
>> +
>> + if (stats & VIR_DOMAIN_STATS_STATE) {
>> + LIBXL_RECORD_UINT(cleanup, "state.reason", dom->state.reason);
>> + LIBXL_RECORD_UINT(cleanup, "state.state", dom->state.state);
>> + }
>> +
>> + if (stats & VIR_DOMAIN_STATS_BALLOON) {
>> + LIBXL_RECORD_ULL(cleanup, "balloon.current", mem);
>> + LIBXL_RECORD_ULL(cleanup, "balloon.maximum", maxmem);
>> + }
>> +
>> + if (stats & VIR_DOMAIN_STATS_CPU_TOTAL)
>> + LIBXL_RECORD_ULL(cleanup, "cpu.time", d_info.cpu_time);
>> +
>> + if (stats & VIR_DOMAIN_STATS_VCPU) {
>> + vcpuinfo = libxl_list_vcpu(cfg->ctx, dom->def->id, &maxcpu, &hostcpus);
>> +
>> + for (i = 0; i < dom->def->vcpus; i++) {
>> + if (!vcpuinfo)
>> + state = VIR_VCPU_OFFLINE;
>> + else if (vcpuinfo[i].running)
>> + state = VIR_VCPU_RUNNING;
>> + else if (vcpuinfo[i].blocked)
>> + state = VIR_VCPU_BLOCKED;
>> + else
>> + state = VIR_VCPU_OFFLINE;
>> +
>> + LIBXL_RECORD_UINT(cleanup_vcpu, "vcpu.%zu.state", state, i);
>> +
>> + /* vcputime is shown only if the VM is active */
>> + if (!virDomainObjIsActive(dom))
>> + continue;
>> +
>> + LIBXL_RECORD_ULL(cleanup_vcpu, "vcpu.%zu.time", vcpuinfo[i].vcpu_time, i);
>> + }
>> +
>> + if (vcpuinfo)
>> + libxl_vcpuinfo_list_free(vcpuinfo, maxcpu);
>> + }
>> +
>> + if (stats & VIR_DOMAIN_STATS_INTERFACE) {
>> + for (i = 0; i < dom->def->nnets; i++) {
>> + virDomainNetDefPtr net = dom->def->nets[i];
>> + struct _virDomainInterfaceStats netstats;
>> +
>> + if (!virDomainObjIsActive(dom) || !net->ifname)
>> + continue;
>> +
>> + if ((ret = virNetInterfaceStats(net->ifname, &netstats)) < 0)
>> + continue;
>> +
>> + LIBXL_RECORD_ULL(cleanup, "net.%zu.rx.bytes", netstats.rx_bytes, i);
>> + LIBXL_RECORD_ULL(cleanup, "net.%zu.rx.pkts", netstats.rx_packets, i);
>> + LIBXL_RECORD_ULL(cleanup, "net.%zu.rx.drop", netstats.rx_drop, i);
>> + LIBXL_RECORD_ULL(cleanup, "net.%zu.rx.errs", netstats.rx_errs, i);
>> + LIBXL_RECORD_ULL(cleanup, "net.%zu.tx.bytes", netstats.tx_bytes, i);
>> + LIBXL_RECORD_ULL(cleanup, "net.%zu.tx.pkts", netstats.tx_packets, i);
>> + LIBXL_RECORD_ULL(cleanup, "net.%zu.tx.drop", netstats.tx_drop, i);
>> + LIBXL_RECORD_ULL(cleanup, "net.%zu.tx.errs", netstats.tx_errs, i);
>> + LIBXL_RECORD_STR(cleanup, "net.%zu.name", net->ifname, i);
>> + }
>> +
>> + LIBXL_RECORD_UINT(cleanup, "net.count", dom->def->nnets);
>> + }
>> +
>> + if (stats & VIR_DOMAIN_STATS_BLOCK) {
>> + for (i = 0; i < dom->def->ndisks; i++) {
>> + virDomainDiskDefPtr disk = dom->def->disks[i];
>> + struct _libxlBlockStats blkstats;
>> +
>> + if (!virDomainObjIsActive(dom))
>> + continue;
>> +
>> + memset(&blkstats, 0, sizeof(libxlBlockStats));
>> + if ((ret = libxlDomainBlockStatsGather(dom, disk->dst, &blkstats)) < 0)
>> + continue;
>> +
>> + LIBXL_RECORD_LL(cleanup, "block.%zu.rd.reqs", blkstats.rd_req, i);
>> + LIBXL_RECORD_LL(cleanup, "block.%zu.rd.bytes", blkstats.rd_bytes, i);
>> + LIBXL_RECORD_LL(cleanup, "block.%zu.wr.reqs", blkstats.wr_req, i);
>> + LIBXL_RECORD_LL(cleanup, "block.%zu.wr.bytes", blkstats.wr_bytes, i);
>> + LIBXL_RECORD_LL(cleanup, "block.%zu.fl.reqs", blkstats.f_req, i);
>> +
>> + if (STREQ_NULLABLE(blkstats.backend, "vbd")) {
>> + LIBXL_RECORD_LL(cleanup, "block.%zu.discard.reqs", blkstats.u.vbd.ds_req, i);
>> + LIBXL_RECORD_LL(cleanup, "block.%zu.errs", blkstats.u.vbd.oo_req, i);
>> + }
>> + LIBXL_RECORD_STR(cleanup, "block.%zu.name", disk->dst, i);
>> + LIBXL_RECORD_STR(cleanup, "block.%zu.path", disk->src->path, i);
>> + }
>> +
>> + LIBXL_RECORD_UINT(cleanup, "block.count", dom->def->ndisks);
>> + }
>> +
>> + if (!(tmp->dom = virGetDomain(conn, dom->def->name, dom->def->uuid)))
>> + goto cleanup;
>> +
>> + *record = tmp;
>
> tmp = NULL;
> ret = 0;
>
> then fall-though to cleanup? Otherwise looks like there are some leaks.
>
Ah, yes. Will need to do this too:
if (tmp)
virTypedParamsFree(tmp->params, tmp->nparams);
And perhaps set vcpuinfo to NULL on (stats & VIR_DOMAIN_STATS_VCPU) after
libxl_vcpuinfo_list_free() gets called so that I can just remove the "return 0"
there. That way return path is clear and also with no leaks.
Thanks!
Joao
> Regards,
> Jim
>
>> + return 0;
>> +
>> + cleanup_vcpu:
>> + if (vcpuinfo)
>> + libxl_vcpuinfo_list_free(vcpuinfo, maxcpu);
>> + cleanup:
>> + libxl_dominfo_dispose(&d_info);
>> + virTypedParamsFree(tmp->params, tmp->nparams);
>> + VIR_FREE(tmp);
>> + return ret;
>> +}
>> +
>> +static int
>> +libxlConnectGetAllDomainStats(virConnectPtr conn,
>> + virDomainPtr *doms,
>> + unsigned int ndoms,
>> + unsigned int stats,
>> + virDomainStatsRecordPtr **retStats,
>> + unsigned int flags)
>> +{
>> + libxlDriverPrivatePtr driver = conn->privateData;
>> + virDomainObjPtr *vms = NULL;
>> + virDomainObjPtr vm;
>> + size_t nvms;
>> + virDomainStatsRecordPtr *tmpstats = NULL;
>> + int nstats = 0;
>> + size_t i;
>> + int ret = -1;
>> + unsigned int lflags = flags & (VIR_CONNECT_LIST_DOMAINS_FILTERS_ACTIVE |
>> + VIR_CONNECT_LIST_DOMAINS_FILTERS_PERSISTENT |
>> + VIR_CONNECT_LIST_DOMAINS_FILTERS_STATE);
>> +
>> + virCheckFlags(VIR_CONNECT_LIST_DOMAINS_FILTERS_ACTIVE |
>> + VIR_CONNECT_LIST_DOMAINS_FILTERS_PERSISTENT |
>> + VIR_CONNECT_LIST_DOMAINS_FILTERS_STATE, -1);
>> +
>> + if (virConnectGetAllDomainStatsEnsureACL(conn) < 0)
>> + return -1;
>> +
>> + if (ndoms) {
>> + if (virDomainObjListConvert(driver->domains, conn, doms, ndoms, &vms,
>> + &nvms, virConnectGetAllDomainStatsCheckACL,
>> + lflags, true) < 0)
>> + return -1;
>> + } else {
>> + if (virDomainObjListCollect(driver->domains, conn, &vms, &nvms,
>> + virConnectGetAllDomainStatsCheckACL,
>> + lflags) < 0)
>> + return -1;
>> + }
>> +
>> + if (VIR_ALLOC_N(tmpstats, nvms + 1) < 0)
>> + return -1;
>> +
>> + for (i = 0; i < nvms; i++) {
>> + virDomainStatsRecordPtr tmp = NULL;
>> + vm = vms[i];
>> +
>> + virObjectLock(vm);
>> +
>> + if (libxlDomainGetStats(conn, vm, stats, &tmp) < 0) {
>> + virObjectUnlock(vm);
>> + goto cleanup;
>> + }
>> +
>> + if (tmp)
>> + tmpstats[nstats++] = tmp;
>> +
>> + virObjectUnlock(vm);
>> + }
>> +
>> + *retStats = tmpstats;
>> + tmpstats = NULL;
>> +
>> + ret = nstats;
>> +
>> + cleanup:
>> + virDomainStatsRecordListFree(tmpstats);
>> + virObjectListFreeCount(vms, nvms);
>> + return ret;
>> +}
>> +
>> static int
>> libxlConnectDomainEventRegisterAny(virConnectPtr conn, virDomainPtr dom, int eventID,
>> virConnectDomainEventGenericCallback callback,
>> @@ -5836,6 +6101,7 @@ static virHypervisorDriver libxlHypervisorDriver = {
>> .domainInterfaceStats = libxlDomainInterfaceStats, /* 1.2.22 */
>> .domainMemoryStats = libxlDomainMemoryStats, /* 1.2.22 */
>> .domainGetCPUStats = libxlDomainGetCPUStats, /* 1.2.22 */
>> + .connectGetAllDomainStats = libxlConnectGetAllDomainStats, /* 1.2.22 */
>> .connectDomainEventRegister = libxlConnectDomainEventRegister, /* 0.9.0 */
>> .connectDomainEventDeregister = libxlConnectDomainEventDeregister, /* 0.9.0 */
>> .domainManagedSave = libxlDomainManagedSave, /* 0.9.2 */
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list