On 09/08/2015 02:27 AM, Joao Martins wrote:

Introduce support for domainGetCPUStats API call and consequently allow us to use `virsh cpu-stats`. The latter returns a more brief output than the one provided by`virsh vcpuinfo`.

Signed-off-by: Joao Martins <joao.m.martins@oracle.com> --- src/libxl/libxl_driver.c | 111 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 111 insertions(+)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 5f69b49..101e4c7 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -73,6 +73,8 @@ VIR_LOG_INIT("libxl.libxl_driver"); #define LIBXL_CONFIG_FORMAT_XM "xen-xm" #define LIBXL_CONFIG_FORMAT_SEXPR "xen-sxpr"

+#define LIBXL_NB_TOTAL_CPU_STAT_PARAM 1 + #define HYPERVISOR_CAPABILITIES "/proc/xen/capabilities" #define HYPERVISOR_XENSTORED "/dev/xen/xenstored"

@@ -4638,6 +4640,114 @@ libxlDomainIsUpdated(virDomainPtr dom) }

static int +libxlDomainGetTotalCPUStats(libxlDriverPrivatePtr driver, + virDomainObjPtr vm, + virTypedParameterPtr params, + unsigned int nparams) +{ + libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver); + libxl_dominfo d_info; + int ret = -1; + + if (nparams == 0) + return LIBXL_NB_TOTAL_CPU_STAT_PARAM; + + if (libxl_domain_info(cfg->ctx, &d_info, vm->def->id) != 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("libxl_domain_info failed for domain '%d'"), + vm->def->id); + return ret; + } +

I see that xl calls libxl_dominfo_dispose() after successful libxl_domain_info(). We might be missing that elsewhere in the libxl driver.

+ if (virTypedParameterAssign(¶ms[0], VIR_DOMAIN_CPU_STATS_CPUTIME, + VIR_TYPED_PARAM_ULLONG, d_info.cpu_time) < 0) + return ret; + + return nparams; +} + +static int +libxlDomainGetPerCPUStats(libxlDriverPrivatePtr driver, + virDomainObjPtr vm, + virTypedParameterPtr params, + unsigned int nparams, + int start_cpu, + unsigned int ncpus)

Indentation of parameters is off.

+{ + libxl_vcpuinfo *vcpuinfo; + int maxcpu, hostcpus; + size_t i; + libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver); + int ret = -1; + + if (nparams == 0 && ncpus != 0) + return LIBXL_NB_TOTAL_CPU_STAT_PARAM; + else if (nparams == 0) + return vm->def->maxvcpus; + + if ((vcpuinfo = libxl_list_vcpu(cfg->ctx, vm->def->id, &maxcpu, + &hostcpus)) == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to list vcpus for domain '%d' with libxenlight"), + vm->def->id); + goto cleanup; + } + + for (i = start_cpu; i < maxcpu && i < ncpus; ++i) { + if (virTypedParameterAssign(¶ms[(i-start_cpu)], + VIR_DOMAIN_CPU_STATS_CPUTIME, + VIR_TYPED_PARAM_ULLONG, + vcpuinfo[i].vcpu_time) < 0) + goto cleanup; + + libxl_vcpuinfo_dispose(&vcpuinfo[i]);

The call to libxl_vcpuinfo_dispose() can be removed

+ } + ret = nparams; + + cleanup: + VIR_FREE(vcpuinfo);

if the VIR_FREE() is replaced with libxl_vcpuinfo_list_free(). Other than the few nits, looks good. BTW, thanks for the patches! But be warned: I may make slow progress reviewing them. Regards, Jim

+ return ret; +} + +static int +libxlDomainGetCPUStats(virDomainPtr dom, + virTypedParameterPtr params, + unsigned int nparams, + int start_cpu, + unsigned int ncpus, + unsigned int flags) +{ + libxlDriverPrivatePtr driver = dom->conn->privateData; + virDomainObjPtr vm = NULL; + int ret = -1; + + virCheckFlags(VIR_TYPED_PARAM_STRING_OKAY, -1); + + if (!(vm = libxlDomObjFromDomain(dom))) + goto cleanup; + + if (virDomainGetCPUStatsEnsureACL(dom->conn, vm->def) < 0) + goto cleanup; + + if (!virDomainObjIsActive(vm)) { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("domain is not running")); + goto cleanup; + } + + if (start_cpu == -1) + ret = libxlDomainGetTotalCPUStats(driver, vm, params, nparams); + else + ret = libxlDomainGetPerCPUStats(driver, vm, params, nparams, + start_cpu, ncpus); + + cleanup: + if (vm) + virObjectUnlock(vm); + return ret; +} + +static int libxlConnectDomainEventRegisterAny(virConnectPtr conn, virDomainPtr dom, int eventID, virConnectDomainEventGenericCallback callback, void *opaque, virFreeCallback freecb) @@ -5230,6 +5340,7 @@ static virHypervisorDriver libxlHypervisorDriver = { #endif .nodeGetFreeMemory = libxlNodeGetFreeMemory, /* 0.9.0 */ .nodeGetCellsFreeMemory = libxlNodeGetCellsFreeMemory, /* 1.1.1 */ + .domainGetCPUStats = libxlDomainGetCPUStats, /* 1.2.20 */ .connectDomainEventRegister = libxlConnectDomainEventRegister, /* 0.9.0 */ .connectDomainEventDeregister = libxlConnectDomainEventDeregister, /* 0.9.0 */ .domainManagedSave = libxlDomainManagedSave, /* 0.9.2 */

On 09/17/2015 11:16 PM, Jim Fehlig wrote:

On 09/08/2015 02:27 AM, Joao Martins wrote:

...

Introduce support for domainMemoryStats API call, which consequently enables the use of `virsh dommemstat` command to query for memory statistics of a domain. We support the following statistics: balloon info, available and currently in use. swap-in, swap-out, major-faults, minor-faults require cooperation of the guest and thus currently not supported.

We build on the data returned from libxl_domain_info and deliver it in the virDomainMemoryStat format.

Signed-off-by: Joao Martins <joao.m.martins@oracle.com> --- src/libxl/libxl_driver.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 68 insertions(+)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 101e4c7..43e9e47 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -4747,6 +4747,73 @@ libxlDomainGetCPUStats(virDomainPtr dom, return ret; }

+#define LIBXL_SET_MEMSTAT(TAG, VAL) \ + if (i < nr_stats) { \ + stats[i].tag = TAG; \ + stats[i].val = VAL; \ + i++; \ + } + +static int +libxlDomainMemoryStats(virDomainPtr dom, + virDomainMemoryStatPtr stats, + unsigned int nr_stats, + unsigned int flags) +{ + libxlDriverPrivatePtr driver = dom->conn->privateData; + libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver); + virDomainObjPtr vm; + libxl_dominfo d_info; + unsigned mem, maxmem; + size_t i = 0; + int ret = -1; + + virCheckFlags(0, -1); + + if (!(vm = libxlDomObjFromDomain(dom))) + goto cleanup; + + if (virDomainMemoryStatsEnsureACL(dom->conn, vm->def) < 0) + goto cleanup; + + if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_QUERY) < 0) + goto cleanup; + + if (!virDomainObjIsActive(vm)) { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("domain is not running")); + goto endjob; + } + + if (libxl_domain_info(cfg->ctx, &d_info, vm->def->id) != 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("libxl_domain_info failed for domain '%d'"), + vm->def->id); + return -1;

goto endjob; ?

Yeah. Will fix that for v2.

Also, as you've already noted, another case of missing libxl_dominfo_dispose(). But I'll stop mentioning those now :-).

OK. Thanks, Joao

Regards, Jim

...

+ } + mem = d_info.current_memkb; + maxmem = d_info.max_memkb; + + LIBXL_SET_MEMSTAT(VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON, maxmem - mem); + LIBXL_SET_MEMSTAT(VIR_DOMAIN_MEMORY_STAT_AVAILABLE, maxmem); + LIBXL_SET_MEMSTAT(VIR_DOMAIN_MEMORY_STAT_RSS, mem); + + ret = i; + + endjob: + if (!libxlDomainObjEndJob(driver, vm)) { + virObjectUnlock(vm); + vm = NULL; + } + + cleanup: + if (vm) + virObjectUnlock(vm); + return ret; +} + +#undef LIBXL_SET_MEMSTAT + static int libxlConnectDomainEventRegisterAny(virConnectPtr conn, virDomainPtr dom, int eventID, virConnectDomainEventGenericCallback callback, @@ -5340,6 +5407,7 @@ static virHypervisorDriver libxlHypervisorDriver = { #endif .nodeGetFreeMemory = libxlNodeGetFreeMemory, /* 0.9.0 */ .nodeGetCellsFreeMemory = libxlNodeGetCellsFreeMemory, /* 1.1.1 */ + .domainMemoryStats = libxlDomainMemoryStats, /* 1.2.20 */ .domainGetCPUStats = libxlDomainGetCPUStats, /* 1.2.20 */ .connectDomainEventRegister = libxlConnectDomainEventRegister, /* 0.9.0 */ .connectDomainEventDeregister = libxlConnectDomainEventDeregister, /* 0.9.0 */

Joao Martins wrote:

Introduce support for domainInterfaceStats API call for querying network interface statistics. Consequently it also enables the use of `virsh domifstat <dom> <interface name>` command.

For getting statistics we resort to virNetInterfaceStats and let libvirt handle the platform specific nits. Note that the latter is not yet supported in FreeBSD.

Signed-off-by: Joao Martins <joao.m.martins@oracle.com> --- src/libxl/libxl_driver.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 43e9e47..dc83083 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -58,6 +58,7 @@ #include "virhostdev.h" #include "network/bridge_driver.h" #include "locking/domain_lock.h" +#include "virstats.h"

#define VIR_FROM_THIS VIR_FROM_LIBXL

@@ -4640,6 +4641,57 @@ libxlDomainIsUpdated(virDomainPtr dom) }

static int +libxlDomainInterfaceStats(virDomainPtr dom, + const char *path, + virDomainInterfaceStatsPtr stats) +{ + libxlDriverPrivatePtr driver = dom->conn->privateData; + virDomainObjPtr vm; + int ret = -1; + int domid, devid; + + if (!(vm = libxlDomObjFromDomain(dom))) + goto cleanup; + + if (virDomainInterfaceStatsEnsureACL(dom->conn, vm->def) < 0) + goto cleanup; + + if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_QUERY) < 0) + goto cleanup; + + if (!virDomainObjIsActive(vm)) { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("domain is not running")); + goto endjob; + } + + if (sscanf(path, "vif%d.%d", &domid, &devid) != 2) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("invalid path, unknown device")); + goto endjob; + } + + if (domid != vm->def->id) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("invalid path, domid doesn't match")); + goto endjob; + }

Should we also ensure the domain has an interface matching devid before calling virNetInterfaceStats()? I see the qemu driver has such a check, but virNetInterfaceStats() also reports "Interface not found". Regards, Jim

+ + ret = virNetInterfaceStats(path, stats); + + endjob: + if (!libxlDomainObjEndJob(driver, vm)) { + virObjectUnlock(vm); + vm = NULL; + } + + cleanup: + if (vm) + virObjectUnlock(vm); + return ret; +} + +static int libxlDomainGetTotalCPUStats(libxlDriverPrivatePtr driver, virDomainObjPtr vm, virTypedParameterPtr params, @@ -5407,6 +5459,7 @@ static virHypervisorDriver libxlHypervisorDriver = { #endif .nodeGetFreeMemory = libxlNodeGetFreeMemory, /* 0.9.0 */ .nodeGetCellsFreeMemory = libxlNodeGetCellsFreeMemory, /* 1.1.1 */ + .domainInterfaceStats = libxlDomainInterfaceStats, /* 1.2.20 */ .domainMemoryStats = libxlDomainMemoryStats, /* 1.2.20 */ .domainGetCPUStats = libxlDomainGetCPUStats, /* 1.2.20 */ .connectDomainEventRegister = libxlConnectDomainEventRegister, /* 0.9.0 */

Joao Martins wrote:

On 09/23/2015 08:18 PM, Jim Fehlig wrote:

...

Joao Martins wrote:

...

Introduce support for domainInterfaceStats API call for querying network interface statistics. Consequently it also enables the use of `virsh domifstat <dom> <interface name>` command.

For getting statistics we resort to virNetInterfaceStats and let libvirt handle the platform specific nits. Note that the latter is not yet supported in FreeBSD.

Signed-off-by: Joao Martins <joao.m.martins@oracle.com> --- src/libxl/libxl_driver.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 43e9e47..dc83083 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -58,6 +58,7 @@ #include "virhostdev.h" #include "network/bridge_driver.h" #include "locking/domain_lock.h" +#include "virstats.h"

#define VIR_FROM_THIS VIR_FROM_LIBXL

@@ -4640,6 +4641,57 @@ libxlDomainIsUpdated(virDomainPtr dom) }

static int +libxlDomainInterfaceStats(virDomainPtr dom, + const char *path, + virDomainInterfaceStatsPtr stats) +{ + libxlDriverPrivatePtr driver = dom->conn->privateData; + virDomainObjPtr vm; + int ret = -1; + int domid, devid; + + if (!(vm = libxlDomObjFromDomain(dom))) + goto cleanup; + + if (virDomainInterfaceStatsEnsureACL(dom->conn, vm->def) < 0) + goto cleanup; + + if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_QUERY) < 0) + goto cleanup; + + if (!virDomainObjIsActive(vm)) { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("domain is not running")); + goto endjob; + } + + if (sscanf(path, "vif%d.%d", &domid, &devid) != 2) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("invalid path, unknown device")); + goto endjob; + } + + if (domid != vm->def->id) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("invalid path, domid doesn't match")); + goto endjob; + }

Should we also ensure the domain has an interface matching devid before calling virNetInterfaceStats()? I see the qemu driver has such a check, but virNetInterfaceStats() also reports "Interface not found".

Ultimately I rely on that error and check if the path name is the path correspondent to a xen interface prefixed by "vif%d.%d". Problem with doing that check is that net->ifname is not set unless explicitly. And looking at the qemu driver: they create the device and fetch the ifname and then add it to the bridge.

Perhaps it is reasonable is if we set the net->ifname after domain create and then make that check, as you suggest, instead of trying to "guess" the path. The way this patch proposes, the user wouldn't be able to get statistics from anything other than "vif%d.%d[-emu]" which is incorrect.

Yeah, I think setting net->ifname after start is a good idea. libxlConsoleCallback() could be re-purposed to a more generic libxlDomainStartCallback() or similar, where info provided by libxl is copied to the virDomainObj. Regards, Jim

Joao Martins wrote:

Introduce initial support for domainBlockStats API call that allow us to query block device statistics. openstack nova uses this API call to query block statistics, alongside virDomainMemoryStats and virDomainInterfaceStats. Note that this patch only introduces it for VBD for starters. QDisk will come in a separate patch series.

A new statistics data structure is introduced to fit common statistics among others specific to the underlying block backends. For the VBD statistics on linux these are exported via sysfs on the path:

"/sys/bus/xen-backend/devices/vbd-<domid>-<devid>/statistics"

To calculate the block devid two libxl functions were ported (libxlDiskPathMatches and libxlDiskPathParse) to libvirt to make sure the devid is calculate in the same way as libxl. Each backend implements its own function to extract statistics and let there be handled the different platforms. An alternative would be to reuse libvirt xen driver function.

VBD stats are exposed in reqs and number of sectors from blkback, and it's up to us to convert it to sector sizes. The sector size is gathered through xenstore in the device backend entry "physical-sector-size". This adds up an extra dependency namely of xenstore for doing the xs_read.

BlockStatsFlags variant is also implemented which has the added benefit of getting the number of flush requests.

Signed-off-by: Joao Martins <joao.m.martins@oracle.com> --- configure.ac | 2 +- src/libxl/libxl_driver.c | 420 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 421 insertions(+), 1 deletion(-)

diff --git a/configure.ac b/configure.ac index ef7fbdb..56fb266 100644 --- a/configure.ac +++ b/configure.ac @@ -896,7 +896,7 @@ if test "$with_libxl" != "no" ; then LIBS="$LIBS $LIBXL_LIBS" AC_CHECK_LIB([xenlight], [libxl_ctx_alloc], [ with_libxl=yes - LIBXL_LIBS="$LIBXL_LIBS -lxenlight -lxenctrl" + LIBXL_LIBS="$LIBXL_LIBS -lxenlight -lxenctrl -lxenstore" ],[ if test "$with_libxl" = "yes"; then fail=1 diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index dc83083..fd952a3 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -59,6 +59,7 @@ #include "network/bridge_driver.h" #include "locking/domain_lock.h" #include "virstats.h" +#include <xenstore.h>

#define VIR_FROM_THIS VIR_FROM_LIBXL

@@ -75,6 +76,7 @@ VIR_LOG_INIT("libxl.libxl_driver"); #define LIBXL_CONFIG_FORMAT_SEXPR "xen-sxpr"

#define LIBXL_NB_TOTAL_CPU_STAT_PARAM 1 +#define LIBXL_NB_TOTAL_BLK_STAT_PARAM 6

#define HYPERVISOR_CAPABILITIES "/proc/xen/capabilities" #define HYPERVISOR_XENSTORED "/dev/xen/xenstored" @@ -103,6 +105,25 @@ struct _libxlOSEventHookInfo { int id; };

+/* Object used to store disk statistics across multiple xen backends */ +typedef struct _libxlBlockStats libxlBlockStats; +typedef libxlBlockStats *libxlBlockStatsPtr; +struct _libxlBlockStats { + long long rd_req; + long long rd_bytes; + long long wr_req; + long long wr_bytes; + long long f_req; + + char *backend; + union { + struct { + long long ds_req; + long long oo_req; + } vbd; + } u; +}; + /* Function declarations */ static int libxlDomainManagedSaveLoad(virDomainObjPtr vm, @@ -4641,6 +4662,403 @@ libxlDomainIsUpdated(virDomainPtr dom) }

static int +libxlDiskPathMatches(const char *virtpath, const char *devtype, + int *index_r, int max_index, + int *partition_r, int max_partition) +{ + const char *p; + char *ep; + int tl, c; + long pl; + + tl = strlen(devtype); + if (memcmp(virtpath, devtype, tl)) + return 0; + + /* We decode the drive letter as if it were in base 52 + * with digits a-zA-Z, more or less */ + *index_r = -1; + p = virtpath + tl; + for (;;) { + c = *p++; + if (c >= 'a' && c <= 'z') { + c -= 'a'; + } else { + --p; + break; + } + (*index_r)++; + (*index_r) *= 26; + (*index_r) += c; + + if (*index_r > max_index) + return 0; + } + + if (!*p) { + *partition_r = 0; + return 1; + } + + if (*p == '0') + return 0; /* leading zeroes not permitted in partition number */ + + if (virStrToLong_l(p, &ep, 10, &pl) < 0) + return 0; + + if (pl > max_partition || *ep) + return 0; + + *partition_r = pl; + return 1; +} + +static int +libxlDiskPathParse(const char *virtpath, int *pdisk, int *ppartition) +{ + int disk, partition; + char *ep; + unsigned long ul; + int chrused; + + chrused = -1; + if ((sscanf(virtpath, "d%ip%i%n", &disk, &partition, &chrused) >= 2 + && chrused == strlen(virtpath) && disk < (1<<20) && partition < 256) + || + libxlDiskPathMatches(virtpath, "xvd", + &disk, (1<<20)-1, + &partition, 255)) { + if (pdisk) *pdisk = disk; + if (ppartition) *ppartition = partition; + if (disk <= 15 && partition <= 15) + return (202 << 8) | (disk << 4) | partition; + else + return (1 << 28) | (disk << 8) | partition; + } + + errno = virStrToLong_ul(virtpath, &ep, 0, &ul); + if (!errno && !*ep && ul <= INT_MAX) { + /* FIXME: should parse ul to determine these. */ + if (pdisk || ppartition) + return -1; + return ul; + } + + if (libxlDiskPathMatches(virtpath, "hd", + &disk, 3, + &partition, 63)) { + if (pdisk) *pdisk = disk; + if (ppartition) *ppartition = partition; + return ((disk<2 ? 3 : 22) << 8) | ((disk & 1) << 6) | partition; + } + if (libxlDiskPathMatches(virtpath, "sd", + &disk, 15, + &partition, 15)) { + if (pdisk) *pdisk = disk; + if (ppartition) *ppartition = partition; + return (8 << 8) | (disk << 4) | partition; + } + return -1; +} + +#define LIBXL_VBD_SECTOR_SIZE 512 + +static int +libxlDiskSectorSize(int domid, int devno) +{ + char *path, *val; + struct xs_handle *handle; + int ret = LIBXL_VBD_SECTOR_SIZE; + unsigned int len; + + handle = xs_daemon_open_readonly(); + if (!handle) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("cannot read sector size")); + return ret; + } + + if (virAsprintf(&path, "/local/domain/%d/device/vbd/%d/backend", + domid, devno) < 0)

Indentation looks off.

+ goto close; + + if ((val = xs_read(handle, XBT_NULL, path, &len)) == NULL) + goto cleanup; + + VIR_FREE(path); + if (virAsprintf(&path, "%s/physical-sector-size", val) < 0) { + VIR_FREE(val); + goto close; + } + + VIR_FREE(val); + if ((val = xs_read(handle, XBT_NULL, path, &len)) == NULL) + goto cleanup; + + if (sscanf(val, "%d", &ret) != 1) + ret = -1;

Should ret be set back to LIBXL_VBD_SECTOR_SIZE? '-1' wouldn't be a good value for the calling function.

+ + VIR_FREE(val); + + cleanup: + VIR_FREE(path); + close: + xs_daemon_close(handle); + return ret; +} + +static int +libxlDomainBlockStatsVBD(virDomainObjPtr vm, + const char *dev, + libxlBlockStatsPtr stats) +{ + int ret = -1; + int devno = libxlDiskPathParse(dev, NULL, NULL); + int size = libxlDiskSectorSize(vm->def->id, devno); +#ifdef __linux__ + char *path, *name, *val; + unsigned long long stat; + + if (VIR_STRDUP(stats->backend, "vbd") < 0) { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("cannot set backend"));

VIR_STRDUP() already reports OOM error, which is about the only one it can encounter.

+ return -1; + } + + ret = virAsprintf(&path, "/sys/bus/xen-backend/devices/vbd-%d-%d/statistics", + vm->def->id, devno);

The usual pattern is 'if (virAsprintf(...) < 0)'.

+ + if (!virFileExists(path)) { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("cannot open bus path"));

I think the error should be VIR_ERR_OPERATION_FAILED.

+ goto cleanup; + } + +#define LIBXL_SET_VBDSTAT(FIELD, VAR, MUL) \

Indentation is off here. Fails 'make syntax-check' with cppi installed.

+ if ((virAsprintf(&name, "%s/"FIELD, path) < 0) || \ + (virFileReadAll(name, 256, &val) < 0) || \ + (sscanf(val, "%llu", &stat) != 1)) { \ + virReportError(VIR_ERR_OPERATION_INVALID, \ + _("cannot read %s"), name); \

VIR_ERR_OPERATION_FAILED?

+ goto cleanup; \ + } \ + VAR += (stat * MUL); \ + VIR_FREE(name); \ + VIR_FREE(val); + + LIBXL_SET_VBDSTAT("f_req", stats->f_req, 1) + LIBXL_SET_VBDSTAT("wr_req", stats->wr_req, 1) + LIBXL_SET_VBDSTAT("rd_req", stats->rd_req, 1) + LIBXL_SET_VBDSTAT("wr_sect", stats->wr_bytes, size) + LIBXL_SET_VBDSTAT("rd_sect", stats->rd_bytes, size) + + LIBXL_SET_VBDSTAT("ds_req", stats->u.vbd.ds_req, size) + LIBXL_SET_VBDSTAT("oo_req", stats->u.vbd.oo_req, 1) + ret = 0; + + cleanup: + VIR_FREE(name); + VIR_FREE(path); + VIR_FREE(val);

This will only cleanup 'name' and 'val' from the last invocation of the LIBXL_SET_VBDSTAT macro. 'name' and 'val' from the prior invocations will be leaked.

+ +#undef LIBXL_SET_VBDSTAT

Another case of failing 'make syntax-check' with cppi installed. Regards, Jim

+ +#else + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + "%s", _("platform unsupported")); +#endif + return ret; +} + +static int +libxlDomainBlockStatsGatherSingle(virDomainObjPtr vm, + const char *path, + libxlBlockStatsPtr stats) +{ + virDomainDiskDefPtr disk; + const char *disk_drv; + int ret = -1, disk_fmt; + + if (!(disk = virDomainDiskByName(vm->def, path, false))) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("invalid path: %s"), path); + return ret; + } + + disk_fmt = virDomainDiskGetFormat(disk); + if (!(disk_drv = virDomainDiskGetDriver(disk))) + disk_drv = "qemu"; + + if (STREQ(disk_drv, "phy")) { + if (disk_fmt != VIR_STORAGE_FILE_RAW && + disk_fmt != VIR_STORAGE_FILE_NONE) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("unsupported format %s"), + virStorageFileFormatTypeToString(disk_fmt)); + return ret; + } + + ret = libxlDomainBlockStatsVBD(vm, path, stats); + } else { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("unsupported disk driver %s"), + disk_drv); + return ret; + } + return ret; +} + +static int +libxlDomainBlockStatsGather(virDomainObjPtr vm, + const char *path, + libxlBlockStatsPtr stats) +{ + int ret = -1; + if (*path) { + if (libxlDomainBlockStatsGatherSingle(vm, path, stats) < 0) + return ret; + } else { + size_t i; + for (i = 0; i < vm->def->ndisks; ++i) { + if (libxlDomainBlockStatsGatherSingle(vm, vm->def->disks[i]->dst, + stats) < 0) + return ret; + } + } + return 0; +} + +static int +libxlDomainBlockStats(virDomainPtr dom, + const char *path, + virDomainBlockStatsPtr stats) +{ + libxlDriverPrivatePtr driver = dom->conn->privateData; + virDomainObjPtr vm; + libxlBlockStats blkstats; + int ret = -1; + + if (!(vm = libxlDomObjFromDomain(dom))) + goto cleanup; + + if (virDomainBlockStatsEnsureACL(dom->conn, vm->def) < 0) + goto cleanup; + + if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_QUERY) < 0) + goto cleanup; + + if (!virDomainObjIsActive(vm)) { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("domain is not running")); + goto endjob; + } + + memset(&blkstats, 0, sizeof(libxlBlockStats)); + if ((ret = libxlDomainBlockStatsGather(vm, path, &blkstats)) < 0) + goto endjob; + + stats->rd_req = blkstats.rd_req; + stats->rd_bytes = blkstats.rd_bytes; + stats->wr_req = blkstats.wr_req; + stats->wr_bytes = blkstats.wr_bytes; + if (STREQ_NULLABLE(blkstats.backend, "vbd")) + stats->errs = blkstats.u.vbd.oo_req; + else + stats->errs = -1; + + endjob: + if (!libxlDomainObjEndJob(driver, vm)) { + virObjectUnlock(vm); + vm = NULL; + } + + cleanup: + if (vm) + virObjectUnlock(vm); + return ret; +} + +static int +libxlDomainBlockStatsFlags(virDomainPtr dom, + const char *path, + virTypedParameterPtr params, + int *nparams, + unsigned int flags) +{ + libxlDriverPrivatePtr driver = dom->conn->privateData; + virDomainObjPtr vm; + libxlBlockStats blkstats; + int nstats; + int ret = -1; + + virCheckFlags(VIR_TYPED_PARAM_STRING_OKAY, -1); + + flags &= ~VIR_TYPED_PARAM_STRING_OKAY; + + if (!(vm = libxlDomObjFromDomain(dom))) + goto cleanup; + + if (virDomainBlockStatsFlagsEnsureACL(dom->conn, vm->def) < 0) + goto cleanup; + + if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_QUERY) < 0) + goto cleanup; + + if (!virDomainObjIsActive(vm)) { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("domain is not running")); + goto endjob; + } + + /* return count of supported stats */ + if (*nparams == 0) { + *nparams = LIBXL_NB_TOTAL_BLK_STAT_PARAM; + ret = 0; + goto endjob; + } + + memset(&blkstats, 0, sizeof(libxlBlockStats)); + if ((ret = libxlDomainBlockStatsGather(vm, path, &blkstats)) < 0) + goto endjob; + + nstats = 0; + +#define LIBXL_BLKSTAT_ASSIGN_PARAM(VAR, NAME) \ + if (nstats < *nparams && (blkstats.VAR) != -1) { \ + if (virTypedParameterAssign(params + nstats, NAME, \ + VIR_TYPED_PARAM_LLONG, (blkstats.VAR)) < 0) \ + goto endjob; \ + nstats++; \ + } + + LIBXL_BLKSTAT_ASSIGN_PARAM(wr_bytes, VIR_DOMAIN_BLOCK_STATS_WRITE_BYTES); + LIBXL_BLKSTAT_ASSIGN_PARAM(wr_req, VIR_DOMAIN_BLOCK_STATS_WRITE_REQ); + + LIBXL_BLKSTAT_ASSIGN_PARAM(rd_bytes, VIR_DOMAIN_BLOCK_STATS_READ_BYTES); + LIBXL_BLKSTAT_ASSIGN_PARAM(rd_req, VIR_DOMAIN_BLOCK_STATS_READ_REQ); + + LIBXL_BLKSTAT_ASSIGN_PARAM(f_req, VIR_DOMAIN_BLOCK_STATS_FLUSH_REQ); + + if (STREQ_NULLABLE(blkstats.backend, "vbd")) + LIBXL_BLKSTAT_ASSIGN_PARAM(u.vbd.oo_req, VIR_DOMAIN_BLOCK_STATS_ERRS); + + *nparams = nstats; + +#undef LIBXL_BLKSTAT_ASSIGN_PARAM + + endjob: + if (!libxlDomainObjEndJob(driver, vm)) { + virObjectUnlock(vm); + vm = NULL; + } + + cleanup: + if (vm) + virObjectUnlock(vm); + return ret; +} + +static int libxlDomainInterfaceStats(virDomainPtr dom, const char *path, virDomainInterfaceStatsPtr stats) @@ -5459,6 +5877,8 @@ static virHypervisorDriver libxlHypervisorDriver = { #endif .nodeGetFreeMemory = libxlNodeGetFreeMemory, /* 0.9.0 */ .nodeGetCellsFreeMemory = libxlNodeGetCellsFreeMemory, /* 1.1.1 */ + .domainBlockStats = libxlDomainBlockStats, /* 1.2.20 */ + .domainBlockStatsFlags = libxlDomainBlockStatsFlags, /* 1.2.20 */ .domainInterfaceStats = libxlDomainInterfaceStats, /* 1.2.20 */ .domainMemoryStats = libxlDomainMemoryStats, /* 1.2.20 */ .domainGetCPUStats = libxlDomainGetCPUStats, /* 1.2.20 */

Joao Martins wrote:

Introduce support for connectGetAllDomainStats call that allow us to _all_ domain(s) statistics including network, block, 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@oracle.com> --- src/libxl/libxl_driver.c | 267 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 267 insertions(+)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index fd952a3..01e97fd 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -5284,6 +5284,272 @@ 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); + virDomainStatsRecordPtr tmp; + libxl_dominfo d_info; + libxl_vcpuinfo *vcpuinfo = NULL; + char *path; + 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); + unsigned int vcpuflags = stats & VIR_DOMAIN_STATS_VCPU; + unsigned int netflags = stats & VIR_DOMAIN_STATS_INTERFACE; + unsigned int blkflags = stats & VIR_DOMAIN_STATS_BLOCK; + + if (VIR_ALLOC(tmp) < 0) + return ret; + + 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 (vcpuflags) + vcpuinfo = libxl_list_vcpu(cfg->ctx, dom->def->id, &maxcpu, &hostcpus); + + for (i = 0; i < dom->def->vcpus && vcpuflags; 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);

I found this a bit difficult to read. IMO, something like 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) libxl_vcpuinfo_list_free(vcpuinfo, maxcpu); } is a little clearer. What do you think?

+ + for (i = 0; i < dom->def->nnets && netflags; i++) { + struct _virDomainInterfaceStats netstats; + + if (!virDomainObjIsActive(dom)) + continue; + + if (virAsprintf(&path, "vif%d.%ld", dom->def->id, i) < 0) + goto cleanup; + + ret = virNetInterfaceStats(path, &netstats); + + 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", path, i); + } + + if (netflags) + LIBXL_RECORD_UINT(cleanup, "net.count", dom->def->nnets); + + for (i = 0; i < dom->def->ndisks && blkflags; 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); + } + + if (blkflags) + LIBXL_RECORD_UINT(cleanup, "block.count", dom->def->ndisks);

Same comment for VIR_DOMAIN_STATS_INTERFACE and VIR_DOMAIN_STATS_BLOCK.

+ + if (!(tmp->dom = virGetDomain(conn, dom->def->name, dom->def->uuid))) + goto cleanup; + + *record = tmp; + return 0; + + cleanup_vcpu: + if (vcpuinfo) + libxl_vcpuinfo_list_free(vcpuinfo, maxcpu); + cleanup: + if (path) + VIR_FREE(path);

'make syntax-check' complained here src/libxl/libxl_driver.c: if (path) VIR_FREE(path); maint.mk: found useless "if" before "free" above Also, it looks like you could declare path in a local block handling the interface stats. E.g. if (stats & VIR_DOMAIN_STATS_INTERFACE { char *path = NULL; ... VIR_FREE(path); }

+ 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, @@ -5882,6 +6148,7 @@ static virHypervisorDriver libxlHypervisorDriver = { .domainInterfaceStats = libxlDomainInterfaceStats, /* 1.2.20 */ .domainMemoryStats = libxlDomainMemoryStats, /* 1.2.20 */ .domainGetCPUStats = libxlDomainGetCPUStats, /* 1.2.20 */ + .connectGetAllDomainStats = libxlConnectGetAllDomainStats, /* 1.2.20 */

Now 1.2.21 due to my review delay :-/. Regards, Jim

On 10/13/2015 11:02 PM, Jim Fehlig wrote:

Joao Martins wrote:

...

Introduce support for domainGetJobInfo to get info about the ongoing job. If the job is active it will update the timeElapsed which is computed with the "started" field added to struct libxlDomainJobObj. For now we support just the very basic info and all jobs have VIR_DOMAIN_JOB_UNBOUNDED (i.e. no completion time estimation) plus timeElapsed computed.

Openstack Kilo uses the Job API to monitor live-migration progress which is currently inexistent in libxl driver and

s/inexistent/nonexistent/ ? I think inexistent is a rarely used form of nonexistent, but then again I'm no English major :-).

:)

...

therefore leads to a crash in the nova compute node. Right now, migration doesn't use jobs in the source node and will return VIR_DOMAIN_JOB_NONE. Though nova handles this case and will migrate it properly instead of crashing.

Signed-off-by: Joao Martins <joao.m.martins@oracle.com> --- src/libxl/libxl_domain.c | 29 +++++++++++++++++++++++++++++ src/libxl/libxl_domain.h | 6 ++++++ src/libxl/libxl_driver.c | 38 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 73 insertions(+)

diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 40dcea1..d665615 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -74,6 +74,10 @@ libxlDomainObjInitJob(libxlDomainObjPrivatePtr priv) if (virCondInit(&priv->job.cond) < 0) return -1;

+ if (VIR_ALLOC(priv->job.current) < 0) + return -1; + + memset(priv->job.current, 0, sizeof(*(priv->job.current))); return 0; }

@@ -84,12 +88,14 @@ libxlDomainObjResetJob(libxlDomainObjPrivatePtr priv)

job->active = LIBXL_JOB_NONE; job->owner = 0; +

Spurious whitespace change.

Will remove it.

...

}

static void libxlDomainObjFreeJob(libxlDomainObjPrivatePtr priv) { ignore_value(virCondDestroy(&priv->job.cond)); + VIR_FREE(priv->job.current); }

/* Give up waiting for mutex after 30 seconds */ @@ -131,6 +137,8 @@ libxlDomainObjBeginJob(libxlDriverPrivatePtr driver ATTRIBUTE_UNUSED, VIR_DEBUG("Starting job: %s", libxlDomainJobTypeToString(job)); priv->job.active = job; priv->job.owner = virThreadSelfID(); + priv->job.started = now; + priv->job.current->type = VIR_DOMAIN_JOB_UNBOUNDED;

return 0;

@@ -179,6 +187,27 @@ libxlDomainObjEndJob(libxlDriverPrivatePtr driver ATTRIBUTE_UNUSED, return virObjectUnref(obj); }

+int +libxlDomainJobUpdateTime(struct libxlDomainJobObj *job) +{ + virDomainJobInfoPtr jobInfo = job->current; + unsigned long long now; + + if (!job->started) + return 0; + + if (virTimeMillisNow(&now) < 0) + return -1; + + if (now < job->started) { + job->started = 0; + return 0; + } + + jobInfo->timeElapsed = now - job->started; + return 0; +} + static void * libxlDomainObjPrivateAlloc(void) { diff --git a/src/libxl/libxl_domain.h b/src/libxl/libxl_domain.h index 44b3e0b..1c1eba3 100644 --- a/src/libxl/libxl_domain.h +++ b/src/libxl/libxl_domain.h @@ -53,6 +53,8 @@ struct libxlDomainJobObj { virCond cond; /* Use to coordinate jobs */ enum libxlDomainJob active; /* Currently running job */ int owner; /* Thread which set current job */ + unsigned long long started; /* When the job started */ + virDomainJobInfoPtr current; /* Statistics for the current job */ };

typedef struct _libxlDomainObjPrivate libxlDomainObjPrivate; @@ -88,6 +90,10 @@ libxlDomainObjEndJob(libxlDriverPrivatePtr driver, virDomainObjPtr obj) ATTRIBUTE_RETURN_CHECK;

+int +libxlDomainJobUpdateTime(struct libxlDomainJobObj *job) + ATTRIBUTE_RETURN_CHECK; + void libxlDomainEventQueue(libxlDriverPrivatePtr driver, virObjectEventPtr event); diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 01e97fd..eaf6a67 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -5282,6 +5282,43 @@ libxlDomainMemoryStats(virDomainPtr dom, return ret; }

+static int +libxlDomainGetJobInfo(virDomainPtr dom, + virDomainJobInfoPtr info) +{ + libxlDomainObjPrivatePtr priv; + virDomainObjPtr vm; + int ret = -1; + + if (!(vm = libxlDomObjFromDomain(dom))) + goto cleanup; + + if (virDomainGetJobInfoEnsureACL(dom->conn, vm->def) < 0) + goto cleanup; + + priv = vm->privateData; + if (!priv->job.active) { + memset(info, 0, sizeof(*info)); + info->type = VIR_DOMAIN_JOB_NONE; + ret = 0; + goto cleanup; + } + + /* In libxl we don't have an estimed completion time

s/estimed/estimated/

...

+ * thus we always set to unbounded and update time + * for the active job. */ + if (libxlDomainJobUpdateTime(&priv->job) < 0) + goto cleanup; + + memcpy(info, priv->job.current, sizeof(virDomainJobInfo)); + ret = 0; + + cleanup: + if (vm) + virObjectUnlock(vm); + return ret; +} + #undef LIBXL_SET_MEMSTAT

#define LIBXL_RECORD_UINT(error, key, value, ...) \ @@ -6143,6 +6180,7 @@ static virHypervisorDriver libxlHypervisorDriver = { #endif .nodeGetFreeMemory = libxlNodeGetFreeMemory, /* 0.9.0 */ .nodeGetCellsFreeMemory = libxlNodeGetCellsFreeMemory, /* 1.1.1 */ + .domainGetJobInfo = libxlDomainGetJobInfo, /* 1.2.20 */

1.2.21.

Other than the nits, looks good!

Glad to hear that!

Regards, Jim

Joao Martins wrote:

Introduces support for domainGetJobStats which has the same info as domainGetJobInfo but in a slightly different format. Another difference is that virDomainGetJobStats can also retrieve info on the most recently completed job. Though so far this is only used in the source node to know if the migration has been completed. But because we don't support completed jobs we will deliver an error.

Signed-off-by: Joao Martins <joao.m.martins@oracle.com> --- src/libxl/libxl_driver.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index eaf6a67..31f0b39 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -5319,6 +5319,58 @@ libxlDomainGetJobInfo(virDomainPtr dom, return ret; }

+static int +libxlDomainGetJobStats(virDomainPtr dom, + int *type, + virTypedParameterPtr *params, + int *nparams, + unsigned int flags)

Indentation is off a bit.

+{ + libxlDomainObjPrivatePtr priv; + virDomainObjPtr vm; + virDomainJobInfoPtr jobInfo; + int ret = -1; + int maxparams = 0; + + /* VIR_DOMAIN_JOB_STATS_COMPLETED not supported yet */ + virCheckFlags(0, -1); + + if (!(vm = libxlDomObjFromDomain(dom))) + goto cleanup; + + if (virDomainGetJobStatsEnsureACL(dom->conn, vm->def) < 0) + goto cleanup; + + priv = vm->privateData; + jobInfo = priv->job.current; + if (!priv->job.active) { + *type = VIR_DOMAIN_JOB_NONE; + *params = NULL; + *nparams = 0; + ret = 0; + goto cleanup; + } + + /* In libxl we don't have an estimed completion time

s/estimed/estimated/ ?

+ * thus we always set to unbounded and update time + * for the active job. */ + if (libxlDomainJobUpdateTime(&priv->job) < 0) + goto cleanup; + + if (virTypedParamsAddULLong(params, nparams, &maxparams, + VIR_DOMAIN_JOB_TIME_ELAPSED, + jobInfo->timeElapsed) < 0) + goto cleanup; + + *type = jobInfo->type; + ret = 0; + + cleanup: + if (vm) + virObjectUnlock(vm); + return ret; +} + #undef LIBXL_SET_MEMSTAT

#define LIBXL_RECORD_UINT(error, key, value, ...) \ @@ -6181,6 +6233,7 @@ static virHypervisorDriver libxlHypervisorDriver = { .nodeGetFreeMemory = libxlNodeGetFreeMemory, /* 0.9.0 */ .nodeGetCellsFreeMemory = libxlNodeGetCellsFreeMemory, /* 1.1.1 */ .domainGetJobInfo = libxlDomainGetJobInfo, /* 1.2.20 */ + .domainGetJobStats = libxlDomainGetJobStats, /* 1.2.20 */

1.2.21. Other than the nits, this one looks good too. Looking forward to a V2 of this series, and your series supporting migration V3 and p2p migration. Perhaps the latter will need some rebasing now that Nikolay's series [1] has been committed. Regards, Jim [1] https://www.redhat.com/archives/libvir-list/2015-October/msg00049.html