[PATCH v3 1/8] libxl: implement virDomainGetCPUStats

[PATCH v3 1/8] libxl: implement virDomainGetCPUStats

[Joao Martins]

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>
---
Changes since v1:
 - Remove libxl_vcpuinfo_dispose() in favor or using
 libxl_vcpuinfo_list_free(), and also removing VIR_FREE call
 - Dispose libxl_dominfo after succesfull call to
 libxl_domain_info()
 - Fixed identation of parameters
 - Bump version to 1.2.22
---
 src/libxl/libxl_driver.c | 110 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 110 insertions(+)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index fcdcbdb..50f6e34 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"
 
@@ -4641,6 +4643,113 @@ 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;
+    }
+
+    if (virTypedParameterAssign(&params[0], VIR_DOMAIN_CPU_STATS_CPUTIME,
+                                VIR_TYPED_PARAM_ULLONG, d_info.cpu_time) < 0)
+        nparams = ret;
+
+    libxl_dominfo_dispose(&d_info);
+    return nparams;
+}
+
+static int
+libxlDomainGetPerCPUStats(libxlDriverPrivatePtr driver,
+                          virDomainObjPtr vm,
+                          virTypedParameterPtr params,
+                          unsigned int nparams,
+                          int start_cpu,
+                          unsigned int ncpus)
+{
+    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);
+        return ret;
+    }
+
+    for (i = start_cpu; i < maxcpu && i < ncpus; ++i) {
+        if (virTypedParameterAssign(&params[(i-start_cpu)],
+                                    VIR_DOMAIN_CPU_STATS_CPUTIME,
+                                    VIR_TYPED_PARAM_ULLONG,
+                                    vcpuinfo[i].vcpu_time) < 0)
+            goto cleanup;
+    }
+    ret = nparams;
+
+ cleanup:
+    libxl_vcpuinfo_list_free(vcpuinfo, maxcpu);
+    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)
@@ -5233,6 +5342,7 @@ static virHypervisorDriver libxlHypervisorDriver = {
 #endif
     .nodeGetFreeMemory = libxlNodeGetFreeMemory, /* 0.9.0 */
     .nodeGetCellsFreeMemory = libxlNodeGetCellsFreeMemory, /* 1.1.1 */
+    .domainGetCPUStats = libxlDomainGetCPUStats, /* 1.2.22 */
     .connectDomainEventRegister = libxlConnectDomainEventRegister, /* 0.9.0 */
     .connectDomainEventDeregister = libxlConnectDomainEventDeregister, /* 0.9.0 */
     .domainManagedSave = libxlDomainManagedSave, /* 0.9.2 */
-- 
2.1.4

[PATCH v3 2/8] libxl: implement virDomainMemorystats

[Joao Martins]

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>
---
Changes since v1:
 - Cleanup properly after error fetching domain stats
 - Dispose libxl_dominfo after succesfull call to
 libxl_domain_info()
 - Bump version to 1.2.22
---
 src/libxl/libxl_driver.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 70 insertions(+)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 50f6e34..f4fc7bc 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -4749,6 +4749,75 @@ 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);
+        goto endjob;
+    }
+    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;
+
+    libxl_dominfo_dispose(&d_info);
+
+ 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,
@@ -5342,6 +5411,7 @@ static virHypervisorDriver libxlHypervisorDriver = {
 #endif
     .nodeGetFreeMemory = libxlNodeGetFreeMemory, /* 0.9.0 */
     .nodeGetCellsFreeMemory = libxlNodeGetCellsFreeMemory, /* 1.1.1 */
+    .domainMemoryStats = libxlDomainMemoryStats, /* 1.2.22 */
     .domainGetCPUStats = libxlDomainGetCPUStats, /* 1.2.22 */
     .connectDomainEventRegister = libxlConnectDomainEventRegister, /* 0.9.0 */
     .connectDomainEventDeregister = libxlConnectDomainEventDeregister, /* 0.9.0 */
-- 
2.1.4

[PATCH v3 3/8] libxl: implement virDomainInterfaceStats

[Joao Martins]

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

After succesful guest creation we fill the network
interfaces names based on domain, device id and append suffix
if it's emulated in the following form: vif<domid>.<devid>[-emu]. Because
we need the devid from domain config (which is filled by libxl on domain
create)  we cannot do generate the names in console callback. On domain
cleanup we also clear ifname, in case it was set by libvirt (i.e.
being prefixed with "vif"). We also skip these two steps in case the name
of the interface was manually inserted by the adminstrator.

For getting the interface 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>
---
Changes since v2:
 - Clear ifname if it's autogenerated, since otherwise will persist
 on successive domain starts. Change commit message reflecting this
 change.

Changes since v1:
 - Fill <virDomainNetDef>.ifname after domain start with generated
 name from libxl  based on domain id and devid returned by libxl.
 After that path validation don interfaceStats is enterily based
 on ifname pretty much like the other drivers.
 - Modify commit message reflecting the changes mentioned in
 the previous item.
 - Bump version to 1.2.22
---
 src/libxl/libxl_domain.c | 26 ++++++++++++++++++++++++++
 src/libxl/libxl_driver.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 74 insertions(+)

diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index 40dcea1..adb4563 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -728,6 +728,17 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver,
         }
     }
 
+    if ((vm->def->nnets)) {
+        ssize_t i;
+
+        for (i = 0; i < vm->def->nnets; i++) {
+            virDomainNetDefPtr net = vm->def->nets[i];
+
+            if (STRPREFIX(net->ifname, "vif"))
+                VIR_FREE(net->ifname);
+        }
+    }
+
     if (virAsprintf(&file, "%s/%s.xml", cfg->stateDir, vm->def->name) > 0) {
         if (unlink(file) < 0 && errno != ENOENT && errno != ENOTDIR)
             VIR_DEBUG("Failed to remove domain XML for %s", vm->def->name);
@@ -901,6 +912,7 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
     virDomainDefPtr def = NULL;
     virObjectEventPtr event = NULL;
     libxlSavefileHeader hdr;
+    ssize_t i;
     int ret = -1;
     uint32_t domid = 0;
     char *dom_xml = NULL;
@@ -1023,6 +1035,20 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
      */
     vm->def->id = domid;
 
+    for (i = 0; i < vm->def->nnets; i++) {
+        virDomainNetDefPtr net = vm->def->nets[i];
+        libxl_device_nic *x_nic = &d_config.nics[i];
+        const char *suffix =
+            x_nic->nictype != LIBXL_NIC_TYPE_VIF ? "-emu" : "";
+
+        if (net->ifname)
+            continue;
+
+        if (virAsprintf(&net->ifname, "vif%d.%d%s",
+                        domid, x_nic->devid, suffix) < 0)
+            continue;
+    }
+
     /* Always enable domain death events */
     if (libxl_evenable_domain_death(cfg->ctx, vm->def->id, 0, &priv->deathW))
         goto cleanup_dom;
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index f4fc7bc..9a5a74c 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
 
@@ -4643,6 +4644,52 @@ libxlDomainIsUpdated(virDomainPtr dom)
 }
 
 static int
+libxlDomainInterfaceStats(virDomainPtr dom,
+                          const char *path,
+                          virDomainInterfaceStatsPtr stats)
+{
+    libxlDriverPrivatePtr driver = dom->conn->privateData;
+    virDomainObjPtr vm;
+    ssize_t i;
+    int ret = -1;
+
+    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;
+    }
+
+    /* Check the path is one of the domain's network interfaces. */
+    for (i = 0; i < vm->def->nnets; i++) {
+        if (vm->def->nets[i]->ifname &&
+            STREQ(vm->def->nets[i]->ifname, path)) {
+            ret = virNetInterfaceStats(path, stats);
+            break;
+        }
+    }
+
+ 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,
@@ -5411,6 +5458,7 @@ static virHypervisorDriver libxlHypervisorDriver = {
 #endif
     .nodeGetFreeMemory = libxlNodeGetFreeMemory, /* 0.9.0 */
     .nodeGetCellsFreeMemory = libxlNodeGetCellsFreeMemory, /* 1.1.1 */
+    .domainInterfaceStats = libxlDomainInterfaceStats, /* 1.2.22 */
     .domainMemoryStats = libxlDomainMemoryStats, /* 1.2.22 */
     .domainGetCPUStats = libxlDomainGetCPUStats, /* 1.2.22 */
     .connectDomainEventRegister = libxlConnectDomainEventRegister, /* 0.9.0 */
-- 
2.1.4

[PATCH v3 4/8] util: add virDiskNameParse to handle disk and partition idx

[Joao Martins]

Introduce a new helper function "virDiskNameParse" which extends
virDiskNameToIndex but handling both disk index and partition index.
Also rework virDiskNameToIndex to be based on virDiskNameParse.
A test is also added for this function testing both valid and
invalid disk names.

Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
---
Changes since v2:
 - Sort the newly added symbol in the list
---
 src/libvirt_private.syms |  1 +
 src/util/virutil.c       | 41 +++++++++++++++++++++++++++++++----
 src/util/virutil.h       |  1 +
 tests/utiltest.c         | 56 ++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 95 insertions(+), 4 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index a835f18..7e60d87 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -148,6 +148,7 @@ virDomainCapsNew;
 
 # conf/domain_conf.h
 virBlkioDeviceArrayClear;
+virDiskNameParse;
 virDiskNameToBusDeviceIndex;
 virDiskNameToIndex;
 virDomainActualNetDefFree;
diff --git a/src/util/virutil.c b/src/util/virutil.c
index cddc78a..76591be 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -537,14 +537,17 @@ const char *virEnumToString(const char *const*types,
 }
 
 /* Translates a device name of the form (regex) /^[fhv]d[a-z]+[0-9]*$/
- * into the corresponding index (e.g. sda => 0, hdz => 25, vdaa => 26)
- * Note that any trailing string of digits is simply ignored.
+ * into the corresponding index and partition number
+ * (e.g. sda0 => (0,0), hdz2 => (25,2), vdaa12 => (26,12))
  * @param name The name of the device
- * @return name's index, or -1 on failure
+ * @param disk The disk index to be returned
+ * @param partition The partition index to be returned
+ * @return 0 on success, or -1 on failure
  */
-int virDiskNameToIndex(const char *name)
+int virDiskNameParse(const char *name, int *disk, int *partition)
 {
     const char *ptr = NULL;
+    char *rem;
     int idx = 0;
     static char const* const drive_prefix[] = {"fd", "hd", "vd", "sd", "xvd", "ubd"};
     size_t i;
@@ -573,6 +576,36 @@ int virDiskNameToIndex(const char *name)
     if (ptr[n_digits] != '\0')
         return -1;
 
+    *disk = idx;
+
+    /* Convert trailing digits into our partition index */
+    if (partition) {
+        *partition = 0;
+
+        /* Shouldn't start by zero */
+        if (n_digits > 1 && *ptr == '0')
+            return -1;
+
+        if (n_digits && virStrToLong_i(ptr, &rem, 10, partition) < 0)
+            return -1;
+    }
+
+    return 0;
+}
+
+/* Translates a device name of the form (regex) /^[fhv]d[a-z]+[0-9]*$/
+ * into the corresponding index (e.g. sda => 0, hdz => 25, vdaa => 26)
+ * Note that any trailing string of digits is simply ignored.
+ * @param name The name of the device
+ * @return name's index, or -1 on failure
+ */
+int virDiskNameToIndex(const char *name)
+{
+    int idx;
+
+    if (virDiskNameParse(name, &idx, NULL))
+        idx = -1;
+
     return idx;
 }
 
diff --git a/src/util/virutil.h b/src/util/virutil.h
index 53025f7..02387e0 100644
--- a/src/util/virutil.h
+++ b/src/util/virutil.h
@@ -67,6 +67,7 @@ int virDoubleToStr(char **strp, double number)
 char *virFormatIntDecimal(char *buf, size_t buflen, int val)
     ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
 
+int virDiskNameParse(const char *name, int *disk, int *partition);
 int virDiskNameToIndex(const char* str);
 char *virIndexToDiskName(int idx, const char *prefix);
 
diff --git a/tests/utiltest.c b/tests/utiltest.c
index 98c689d..a8f9060 100644
--- a/tests/utiltest.c
+++ b/tests/utiltest.c
@@ -23,6 +23,23 @@ static const char* diskNames[] = {
     "sdia", "sdib", "sdic", "sdid", "sdie", "sdif", "sdig", "sdih", "sdii", "sdij", "sdik", "sdil", "sdim", "sdin", "sdio", "sdip", "sdiq", "sdir", "sdis", "sdit", "sdiu", "sdiv", "sdiw", "sdix", "sdiy", "sdiz"
 };
 
+struct testDiskName
+{
+    const char *name;
+    int idx;
+    int partition;
+};
+
+static struct testDiskName diskNamesPart[] = {
+    {"sda0",          0,           0},
+    {"sdb10",         1,          10},
+    {"sdc2147483647", 2,  2147483647},
+};
+
+static const char* diskNamesInvalid[] = {
+    "sda00", "sda01", "sdb-1"
+};
+
 static int
 testIndexToDiskName(const void *data ATTRIBUTE_UNUSED)
 {
@@ -79,6 +96,44 @@ testDiskNameToIndex(const void *data ATTRIBUTE_UNUSED)
 
 
 
+static int
+testDiskNameParse(const void *data ATTRIBUTE_UNUSED)
+{
+    size_t i;
+    int idx;
+    int partition;
+    struct testDiskName *disk = NULL;
+
+    for (i = 0; i < ARRAY_CARDINALITY(diskNamesPart); ++i) {
+        disk = &diskNamesPart[i];
+        if (virDiskNameParse(disk->name, &idx, &partition))
+            return -1;
+
+        if (disk->idx != idx) {
+            VIR_TEST_DEBUG("\nExpect [%d]\n", disk->idx);
+            VIR_TEST_DEBUG("Actual [%d]\n", idx);
+            return -1;
+        }
+
+        if (disk->partition != partition) {
+            VIR_TEST_DEBUG("\nExpect [%d]\n", disk->partition);
+            VIR_TEST_DEBUG("Actual [%d]\n", partition);
+            return -1;
+        }
+    }
+
+    for (i = 0; i < ARRAY_CARDINALITY(diskNamesInvalid); ++i) {
+        if (!virDiskNameParse(diskNamesInvalid[i], &idx, &partition)) {
+            VIR_TEST_DEBUG("Should Fail [%s]\n", diskNamesInvalid[i]);
+            return -1;
+        }
+    }
+
+    return 0;
+}
+
+
+
 struct testVersionString
 {
     const char *string;
@@ -220,6 +275,7 @@ mymain(void)
 
     DO_TEST(IndexToDiskName);
     DO_TEST(DiskNameToIndex);
+    DO_TEST(DiskNameParse);
     DO_TEST(ParseVersionString);
     DO_TEST(RoundValueToPowerOfTwo);
     DO_TEST(OverflowCheckMacro);
-- 
2.1.4

Re: [PATCH v3 4/8] util: add virDiskNameParse to handle disk and partition idx

[Jim Fehlig]

On 11/13/2015 06:14 AM, Joao Martins wrote:
> Introduce a new helper function "virDiskNameParse" which extends
> virDiskNameToIndex but handling both disk index and partition index.
> Also rework virDiskNameToIndex to be based on virDiskNameParse.
> A test is also added for this function testing both valid and
> invalid disk names.
>
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> ---
> Changes since v2:
>  - Sort the newly added symbol in the list
> ---
>  src/libvirt_private.syms |  1 +
>  src/util/virutil.c       | 41 +++++++++++++++++++++++++++++++----
>  src/util/virutil.h       |  1 +
>  tests/utiltest.c         | 56 ++++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 95 insertions(+), 4 deletions(-)

ACK; pushed. Thanks!

Regards,
Jim

[PATCH v3 5/8] libxl: implement virDomainBlockStats

[Joao Martins]

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 devno libxlDiskPathToID is introduced.
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>
---
Changes since v1:
 - Fix identation issues
 - Set ret to LIBXL_VBD_SECTOR_SIZE
 - Reuse VIR_STRDUP error instead of doing virReportError
 when we fail to set stats->backend
 - Change virAsprintf(...) error checking
 - Change error to VIR_ERR_OPERATION_FAILED when xenstore path
 does not exist and when failing to read stat.
 - Resolve issues with 'make syntax-check' with cppi installed.
 - Remove libxlDiskPathMatches in favor of using virutil
 virDiskNameParse to fetch disk and partition index.
 - Rename libxlDiskPathParse to libxlDiskPathToID and rework
 function to just convert disk and partition index to devno.
 - Bump version to 1.2.22
---
 configure.ac             |   2 +-
 src/libxl/libxl_driver.c | 375 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 376 insertions(+), 1 deletion(-)

diff --git a/configure.ac b/configure.ac
index f481c50..10c56e5 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 9a5a74c..ba1d67b 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,
@@ -4644,6 +4665,358 @@ libxlDomainIsUpdated(virDomainPtr dom)
 }
 
 static int
+libxlDiskPathToID(const char *virtpath)
+{
+    static char const* drive_prefix[] = {"xvd", "hd", "sd"};
+    int disk, partition, chrused;
+    int fmt, id;
+    char *r;
+    size_t i;
+
+    fmt = id = -1;
+
+    /* Find any disk prefixes we know about */
+    for (i = 0; i < ARRAY_CARDINALITY(drive_prefix); i++) {
+        if (STRPREFIX(virtpath, drive_prefix[i]) &&
+            !virDiskNameParse(virtpath, &disk, &partition)) {
+            fmt = i;
+            break;
+        }
+    }
+
+    /* Handle it same way as xvd */
+    if (fmt < 0 &&
+        (sscanf(virtpath, "d%ip%i%n", &disk, &partition, &chrused) >= 2
+         && chrused == strlen(virtpath)))
+        fmt = 0;
+
+    /* Test indexes ranges and calculate the device id */
+    switch (fmt) {
+    case 0: /* xvd */
+        if (disk <= 15 && partition <= 15)
+            id = (202 << 8) | (disk << 4) | partition;
+        else if ((disk <= ((1<<20)-1)) || partition <= 255)
+            id = (1 << 28) | (disk << 8) | partition;
+        break;
+    case 1: /* hd */
+        if (disk <= 3 && partition <= 63)
+            id = ((disk < 2 ? 3 : 22) << 8) | ((disk & 1) << 6) | partition;
+        break;
+    case 2: /* sd */
+        if (disk <= 15 && (partition <= 15))
+            id = (8 << 8) | (disk << 4) | partition;
+        break;
+    default:
+        errno = virStrToLong_i(virtpath, &r, 0, &id);
+        if (errno || *r || id > INT_MAX)
+            id = -1;
+        break;
+    }
+    return id;
+}
+
+#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)
+        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 = LIBXL_VBD_SECTOR_SIZE;
+
+    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 = libxlDiskPathToID(dev);
+    int size = libxlDiskSectorSize(vm->def->id, devno);
+#ifdef __linux__
+    char *path, *name, *val;
+    unsigned long long stat;
+
+    path = name = val = NULL;
+    if (devno < 0) {
+        virReportError(VIR_ERR_OPERATION_INVALID,
+                       "%s", _("cannot find device number"));
+        return ret;
+    }
+    if (VIR_STRDUP(stats->backend, "vbd") < 0)
+        return ret;
+
+    if (virAsprintf(&path, "/sys/bus/xen-backend/devices/vbd-%d-%d/statistics",
+                    vm->def->id, devno) < 0)
+        return ret;
+
+    if (!virFileExists(path)) {
+        virReportError(VIR_ERR_OPERATION_FAILED,
+                       "%s", _("cannot open bus path"));
+        goto cleanup;
+    }
+
+# define LIBXL_SET_VBDSTAT(FIELD, VAR, MUL)           \
+    if ((virAsprintf(&name, "%s/"FIELD, path) < 0) || \
+        (virFileReadAll(name, 256, &val) < 0) ||      \
+        (sscanf(val, "%llu", &stat) != 1)) {          \
+        virReportError(VIR_ERR_OPERATION_FAILED,     \
+                       _("cannot read %s"), name);    \
+        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);
+
+# undef LIBXL_SET_VBDSTAT
+
+#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)
@@ -5458,6 +5831,8 @@ static virHypervisorDriver libxlHypervisorDriver = {
 #endif
     .nodeGetFreeMemory = libxlNodeGetFreeMemory, /* 0.9.0 */
     .nodeGetCellsFreeMemory = libxlNodeGetCellsFreeMemory, /* 1.1.1 */
+    .domainBlockStats = libxlDomainBlockStats, /* 1.2.22 */
+    .domainBlockStatsFlags = libxlDomainBlockStatsFlags, /* 1.2.22 */
     .domainInterfaceStats = libxlDomainInterfaceStats, /* 1.2.22 */
     .domainMemoryStats = libxlDomainMemoryStats, /* 1.2.22 */
     .domainGetCPUStats = libxlDomainGetCPUStats, /* 1.2.22 */
-- 
2.1.4

[PATCH v3 6/8] libxl: implement virConnectGetAllDomainStats

[Joao Martins]

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>
---
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);
+    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;
+    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 */
-- 
2.1.4

[PATCH v3 7/8] libxl: implement virDomainGetJobInfo

[Joao Martins]

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 nonexistent in libxl driver and
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>
---
Changes since v1:
 - s/inexistent/nonexistent/g in commit message
 - s/estimed/estimated/g
 - Bump version to 1.2.22
---
 src/libxl/libxl_domain.c | 28 ++++++++++++++++++++++++++++
 src/libxl/libxl_domain.h |  6 ++++++
 src/libxl/libxl_driver.c | 38 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 72 insertions(+)

diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index adb4563..baa9617 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;
 }
 
@@ -90,6 +94,7 @@ 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 +136,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 +186,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 8db6536..b0b6ea7 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -5236,6 +5236,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 estimated completion time
+     * 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, ...) \
@@ -6096,6 +6133,7 @@ static virHypervisorDriver libxlHypervisorDriver = {
 #endif
     .nodeGetFreeMemory = libxlNodeGetFreeMemory, /* 0.9.0 */
     .nodeGetCellsFreeMemory = libxlNodeGetCellsFreeMemory, /* 1.1.1 */
+    .domainGetJobInfo = libxlDomainGetJobInfo, /* 1.2.22 */
     .domainBlockStats = libxlDomainBlockStats, /* 1.2.22 */
     .domainBlockStatsFlags = libxlDomainBlockStatsFlags, /* 1.2.22 */
     .domainInterfaceStats = libxlDomainInterfaceStats, /* 1.2.22 */
-- 
2.1.4

[PATCH v3 8/8] libxl: implement virDomainGetJobStats

[Joao Martins]

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>
---
Changes since v1:
 - Fixed indentation on libxlDomainGetJobStats()
 - s/estimed/estimated/g
 - Bump version to 1.2.22
---
 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 b0b6ea7..dda14c2 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -5273,6 +5273,58 @@ libxlDomainGetJobInfo(virDomainPtr dom,
     return ret;
 }
 
+static int
+libxlDomainGetJobStats(virDomainPtr dom,
+                       int *type,
+                       virTypedParameterPtr *params,
+                       int *nparams,
+                       unsigned int flags)
+{
+    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 estimated completion time
+     * 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, ...) \
@@ -6134,6 +6186,7 @@ static virHypervisorDriver libxlHypervisorDriver = {
     .nodeGetFreeMemory = libxlNodeGetFreeMemory, /* 0.9.0 */
     .nodeGetCellsFreeMemory = libxlNodeGetCellsFreeMemory, /* 1.1.1 */
     .domainGetJobInfo = libxlDomainGetJobInfo, /* 1.2.22 */
+    .domainGetJobStats = libxlDomainGetJobStats, /* 1.2.22 */
     .domainBlockStats = libxlDomainBlockStats, /* 1.2.22 */
     .domainBlockStatsFlags = libxlDomainBlockStatsFlags, /* 1.2.22 */
     .domainInterfaceStats = libxlDomainInterfaceStats, /* 1.2.22 */
-- 
2.1.4

Re: [PATCH v3 3/8] libxl: implement virDomainInterfaceStats

[Jim Fehlig]

On 11/13/2015 06:14 AM, 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.
>
> After succesful guest creation we fill the network
> interfaces names based on domain, device id and append suffix
> if it's emulated in the following form: vif<domid>.<devid>[-emu]. Because
> we need the devid from domain config (which is filled by libxl on domain
> create)  we cannot do generate the names in console callback.

Bummer, but see below.

>  On domain
> cleanup we also clear ifname, in case it was set by libvirt (i.e.
> being prefixed with "vif"). We also skip these two steps in case the name
> of the interface was manually inserted by the adminstrator.
>
> For getting the interface 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>
> ---
> Changes since v2:
>  - Clear ifname if it's autogenerated, since otherwise will persist
>  on successive domain starts. Change commit message reflecting this
>  change.
>
> Changes since v1:
>  - Fill <virDomainNetDef>.ifname after domain start with generated
>  name from libxl  based on domain id and devid returned by libxl.
>  After that path validation don interfaceStats is enterily based
>  on ifname pretty much like the other drivers.
>  - Modify commit message reflecting the changes mentioned in
>  the previous item.
>  - Bump version to 1.2.22
> ---
>  src/libxl/libxl_domain.c | 26 ++++++++++++++++++++++++++
>  src/libxl/libxl_driver.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 74 insertions(+)
>
> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
> index 40dcea1..adb4563 100644
> --- a/src/libxl/libxl_domain.c
> +++ b/src/libxl/libxl_domain.c
> @@ -728,6 +728,17 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver,
>          }
>      }
>  
> +    if ((vm->def->nnets)) {
> +        ssize_t i;
> +
> +        for (i = 0; i < vm->def->nnets; i++) {
> +            virDomainNetDefPtr net = vm->def->nets[i];
> +
> +            if (STRPREFIX(net->ifname, "vif"))
> +                VIR_FREE(net->ifname);

Would not be nice if user-specified ifname started with "vif" :-).

> +        }
> +    }
> +
>      if (virAsprintf(&file, "%s/%s.xml", cfg->stateDir, vm->def->name) > 0) {
>          if (unlink(file) < 0 && errno != ENOENT && errno != ENOTDIR)
>              VIR_DEBUG("Failed to remove domain XML for %s", vm->def->name);
> @@ -901,6 +912,7 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
>      virDomainDefPtr def = NULL;
>      virObjectEventPtr event = NULL;
>      libxlSavefileHeader hdr;
> +    ssize_t i;
>      int ret = -1;
>      uint32_t domid = 0;
>      char *dom_xml = NULL;
> @@ -1023,6 +1035,20 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
>       */
>      vm->def->id = domid;
>  
> +    for (i = 0; i < vm->def->nnets; i++) {
> +        virDomainNetDefPtr net = vm->def->nets[i];
> +        libxl_device_nic *x_nic = &d_config.nics[i];
> +        const char *suffix =
> +            x_nic->nictype != LIBXL_NIC_TYPE_VIF ? "-emu" : "";
> +
> +        if (net->ifname)
> +            continue;
> +
> +        if (virAsprintf(&net->ifname, "vif%d.%d%s",
> +                        domid, x_nic->devid, suffix) < 0)
> +            continue;
> +    }
> +

This could be done in the callback, if we added the libxl_domain_config object
to the libxlDomainObjPrivate struct. Currently we create, use, and dispose the
object in libxlDomainStart, but it would probably be useful keep the object
around while the domain is active. Actually, you could directly use the object
in libxlDomainInterfaceStats if it was included in the libxlDomainObjPrivate
struct. I realize it is a bit more work than this or the V1 approach, but what
do you think about it?

Regards,
Jim

Re: [PATCH v3 3/8] libxl: implement virDomainInterfaceStats

[Joao Martins]

On 11/17/2015 02:48 AM, Jim Fehlig wrote:
> On 11/13/2015 06:14 AM, 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.
>>
>> After succesful guest creation we fill the network
>> interfaces names based on domain, device id and append suffix
>> if it's emulated in the following form: vif<domid>.<devid>[-emu]. Because
>> we need the devid from domain config (which is filled by libxl on domain
>> create)  we cannot do generate the names in console callback.
> 
> Bummer, but see below.
> 
>>  On domain
>> cleanup we also clear ifname, in case it was set by libvirt (i.e.
>> being prefixed with "vif"). We also skip these two steps in case the name
>> of the interface was manually inserted by the adminstrator.
>>
>> For getting the interface 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>
>> ---
>> Changes since v2:
>>  - Clear ifname if it's autogenerated, since otherwise will persist
>>  on successive domain starts. Change commit message reflecting this
>>  change.
>>
>> Changes since v1:
>>  - Fill <virDomainNetDef>.ifname after domain start with generated
>>  name from libxl  based on domain id and devid returned by libxl.
>>  After that path validation don interfaceStats is enterily based
>>  on ifname pretty much like the other drivers.
>>  - Modify commit message reflecting the changes mentioned in
>>  the previous item.
>>  - Bump version to 1.2.22
>> ---
>>  src/libxl/libxl_domain.c | 26 ++++++++++++++++++++++++++
>>  src/libxl/libxl_driver.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 74 insertions(+)
>>
>> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
>> index 40dcea1..adb4563 100644
>> --- a/src/libxl/libxl_domain.c
>> +++ b/src/libxl/libxl_domain.c
>> @@ -728,6 +728,17 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver,
>>          }
>>      }
>>  
>> +    if ((vm->def->nnets)) {
>> +        ssize_t i;
>> +
>> +        for (i = 0; i < vm->def->nnets; i++) {
>> +            virDomainNetDefPtr net = vm->def->nets[i];
>> +
>> +            if (STRPREFIX(net->ifname, "vif"))
>> +                VIR_FREE(net->ifname);
> 
> Would not be nice if user-specified ifname started with "vif" :-).
> 
I think QEMU they do in a similar way, in case, specified interfaces started
with a prefix that is solely for autogenerated interface naming. Would you
prefer removing it? The problem with removing this is that ifname would persist
on the XML, and future domainStart would use the old value.

>> +        }
>> +    }
>> +
>>      if (virAsprintf(&file, "%s/%s.xml", cfg->stateDir, vm->def->name) > 0) {
>>          if (unlink(file) < 0 && errno != ENOENT && errno != ENOTDIR)
>>              VIR_DEBUG("Failed to remove domain XML for %s", vm->def->name);
>> @@ -901,6 +912,7 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
>>      virDomainDefPtr def = NULL;
>>      virObjectEventPtr event = NULL;
>>      libxlSavefileHeader hdr;
>> +    ssize_t i;
>>      int ret = -1;
>>      uint32_t domid = 0;
>>      char *dom_xml = NULL;
>> @@ -1023,6 +1035,20 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
>>       */
>>      vm->def->id = domid;
>>  
>> +    for (i = 0; i < vm->def->nnets; i++) {
>> +        virDomainNetDefPtr net = vm->def->nets[i];
>> +        libxl_device_nic *x_nic = &d_config.nics[i];
>> +        const char *suffix =
>> +            x_nic->nictype != LIBXL_NIC_TYPE_VIF ? "-emu" : "";
>> +
>> +        if (net->ifname)
>> +            continue;
>> +
>> +        if (virAsprintf(&net->ifname, "vif%d.%d%s",
>> +                        domid, x_nic->devid, suffix) < 0)
>> +            continue;
>> +    }
>> +
> 
> This could be done in the callback, if we added the libxl_domain_config object
> to the libxlDomainObjPrivate struct. Currently we create, use, and dispose the
> object in libxlDomainStart, but it would probably be useful keep the object
> around while the domain is active.
That's a good idea. This chunk would definitely be better placed in ConsoleCallback.

> Actually, you could directly use the object
> in libxlDomainInterfaceStats if it was included in the libxlDomainObjPrivate
> struct. I realize it is a bit more work than this or the V1 approach, but what
> do you think about it?
Huum, IIUC we would be doing similar to V1, with the difference that we would do
it more reliably by knowning devid in advance through usage of
libxl_domain_config. The good thing about this version though is that it
simplifies things simple for interfaceStats and getAllDomainStats, since all it
takes is just to compare against the interface name we calculated beforehand on
domain start. Moreover we can actually see what interfaces each domain has when
doing domiflist as a result of setting net->ifname (right now only "-" would
show up in the name). The only problem on the latter is the assumption on
domainCleanup in which all interfaces starting "vif" are assumed to be
autogenerated (since there is no flag/field to tell that on virDomainNetDef).

Cheers,
Joao

> 
> Regards,
> Jim
> 

Re: [PATCH v3 3/8] libxl: implement virDomainInterfaceStats

[Jim Fehlig]

Joao Martins wrote:
> 
> On 11/17/2015 02:48 AM, Jim Fehlig wrote:
>> On 11/13/2015 06:14 AM, 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.
>>>
>>> After succesful guest creation we fill the network
>>> interfaces names based on domain, device id and append suffix
>>> if it's emulated in the following form: vif<domid>.<devid>[-emu]. Because
>>> we need the devid from domain config (which is filled by libxl on domain
>>> create)  we cannot do generate the names in console callback.
>> Bummer, but see below.
>>
>>>  On domain
>>> cleanup we also clear ifname, in case it was set by libvirt (i.e.
>>> being prefixed with "vif"). We also skip these two steps in case the name
>>> of the interface was manually inserted by the adminstrator.
>>>
>>> For getting the interface 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>
>>> ---
>>> Changes since v2:
>>>  - Clear ifname if it's autogenerated, since otherwise will persist
>>>  on successive domain starts. Change commit message reflecting this
>>>  change.
>>>
>>> Changes since v1:
>>>  - Fill <virDomainNetDef>.ifname after domain start with generated
>>>  name from libxl  based on domain id and devid returned by libxl.
>>>  After that path validation don interfaceStats is enterily based
>>>  on ifname pretty much like the other drivers.
>>>  - Modify commit message reflecting the changes mentioned in
>>>  the previous item.
>>>  - Bump version to 1.2.22
>>> ---
>>>  src/libxl/libxl_domain.c | 26 ++++++++++++++++++++++++++
>>>  src/libxl/libxl_driver.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>>>  2 files changed, 74 insertions(+)
>>>
>>> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
>>> index 40dcea1..adb4563 100644
>>> --- a/src/libxl/libxl_domain.c
>>> +++ b/src/libxl/libxl_domain.c
>>> @@ -728,6 +728,17 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver,
>>>          }
>>>      }
>>>  
>>> +    if ((vm->def->nnets)) {
>>> +        ssize_t i;
>>> +
>>> +        for (i = 0; i < vm->def->nnets; i++) {
>>> +            virDomainNetDefPtr net = vm->def->nets[i];
>>> +
>>> +            if (STRPREFIX(net->ifname, "vif"))
>>> +                VIR_FREE(net->ifname);
>> Would not be nice if user-specified ifname started with "vif" :-).
>>
> I think QEMU they do in a similar way, in case, specified interfaces started
> with a prefix that is solely for autogenerated interface naming.

Ah, right. Same with bhyve and UML drivers.

> Would you
> prefer removing it? The problem with removing this is that ifname would persist
> on the XML, and future domainStart would use the old value.

Yep, understood. Fine to leave it.

> 
>>> +        }
>>> +    }
>>> +
>>>      if (virAsprintf(&file, "%s/%s.xml", cfg->stateDir, vm->def->name) > 0) {
>>>          if (unlink(file) < 0 && errno != ENOENT && errno != ENOTDIR)
>>>              VIR_DEBUG("Failed to remove domain XML for %s", vm->def->name);
>>> @@ -901,6 +912,7 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
>>>      virDomainDefPtr def = NULL;
>>>      virObjectEventPtr event = NULL;
>>>      libxlSavefileHeader hdr;
>>> +    ssize_t i;
>>>      int ret = -1;
>>>      uint32_t domid = 0;
>>>      char *dom_xml = NULL;
>>> @@ -1023,6 +1035,20 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
>>>       */
>>>      vm->def->id = domid;
>>>  
>>> +    for (i = 0; i < vm->def->nnets; i++) {
>>> +        virDomainNetDefPtr net = vm->def->nets[i];
>>> +        libxl_device_nic *x_nic = &d_config.nics[i];
>>> +        const char *suffix =
>>> +            x_nic->nictype != LIBXL_NIC_TYPE_VIF ? "-emu" : "";
>>> +
>>> +        if (net->ifname)
>>> +            continue;
>>> +
>>> +        if (virAsprintf(&net->ifname, "vif%d.%d%s",
>>> +                        domid, x_nic->devid, suffix) < 0)
>>> +            continue;
>>> +    }
>>> +
>> This could be done in the callback, if we added the libxl_domain_config object
>> to the libxlDomainObjPrivate struct. Currently we create, use, and dispose the
>> object in libxlDomainStart, but it would probably be useful keep the object
>> around while the domain is active.
> That's a good idea. This chunk would definitely be better placed in ConsoleCallback.

Agreed, with ConsoleCallback being renamed to something a bit more generic like
DomainStartCallback or similar.

> 
>> Actually, you could directly use the object
>> in libxlDomainInterfaceStats if it was included in the libxlDomainObjPrivate
>> struct. I realize it is a bit more work than this or the V1 approach, but what
>> do you think about it?
> Huum, IIUC we would be doing similar to V1, with the difference that we would do
> it more reliably by knowning devid in advance through usage of
> libxl_domain_config. The good thing about this version though is that it
> simplifies things simple for interfaceStats and getAllDomainStats, since all it
> takes is just to compare against the interface name we calculated beforehand on
> domain start. Moreover we can actually see what interfaces each domain has when
> doing domiflist as a result of setting net->ifname (right now only "-" would
> show up in the name).

Yes, good point. I think it is best to generate the name when starting the VM.
Other drivers take the same approach.

So it looks like this simple patch could become a series in itself: one patch
adding libxl_domain_config to the libxlDomainObjPrivate struct, another patch to
rename ConsoleCallback to a more generic name, and finally this patch
implementing virDomainInterfaceStats on top of the others.

Regards,
Jim

Re: [PATCH v3 3/8] libxl: implement virDomainInterfaceStats

[Joao Martins]

On 11/17/2015 11:38 PM, Jim Fehlig wrote:
> Joao Martins wrote:
>>
>> On 11/17/2015 02:48 AM, Jim Fehlig wrote:
>>> On 11/13/2015 06:14 AM, 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.
>>>>
>>>> After succesful guest creation we fill the network
>>>> interfaces names based on domain, device id and append suffix
>>>> if it's emulated in the following form: vif<domid>.<devid>[-emu]. Because
>>>> we need the devid from domain config (which is filled by libxl on domain
>>>> create)  we cannot do generate the names in console callback.
>>> Bummer, but see below.
>>>
>>>>  On domain
>>>> cleanup we also clear ifname, in case it was set by libvirt (i.e.
>>>> being prefixed with "vif"). We also skip these two steps in case the name
>>>> of the interface was manually inserted by the adminstrator.
>>>>
>>>> For getting the interface 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>
>>>> ---
>>>> Changes since v2:
>>>>  - Clear ifname if it's autogenerated, since otherwise will persist
>>>>  on successive domain starts. Change commit message reflecting this
>>>>  change.
>>>>
>>>> Changes since v1:
>>>>  - Fill <virDomainNetDef>.ifname after domain start with generated
>>>>  name from libxl  based on domain id and devid returned by libxl.
>>>>  After that path validation don interfaceStats is enterily based
>>>>  on ifname pretty much like the other drivers.
>>>>  - Modify commit message reflecting the changes mentioned in
>>>>  the previous item.
>>>>  - Bump version to 1.2.22
>>>> ---
>>>>  src/libxl/libxl_domain.c | 26 ++++++++++++++++++++++++++
>>>>  src/libxl/libxl_driver.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>>>>  2 files changed, 74 insertions(+)
>>>>
>>>> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
>>>> index 40dcea1..adb4563 100644
>>>> --- a/src/libxl/libxl_domain.c
>>>> +++ b/src/libxl/libxl_domain.c
>>>> @@ -728,6 +728,17 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver,
>>>>          }
>>>>      }
>>>>  
>>>> +    if ((vm->def->nnets)) {
>>>> +        ssize_t i;
>>>> +
>>>> +        for (i = 0; i < vm->def->nnets; i++) {
>>>> +            virDomainNetDefPtr net = vm->def->nets[i];
>>>> +
>>>> +            if (STRPREFIX(net->ifname, "vif"))
>>>> +                VIR_FREE(net->ifname);
>>> Would not be nice if user-specified ifname started with "vif" :-).
>>>
>> I think QEMU they do in a similar way, in case, specified interfaces started
>> with a prefix that is solely for autogenerated interface naming.
> 
> Ah, right. Same with bhyve and UML drivers.
> 
>> Would you
>> prefer removing it? The problem with removing this is that ifname would persist
>> on the XML, and future domainStart would use the old value.
> 
> Yep, understood. Fine to leave it.
> 
OK.

>>
>>>> +        }
>>>> +    }
>>>> +
>>>>      if (virAsprintf(&file, "%s/%s.xml", cfg->stateDir, vm->def->name) > 0) {
>>>>          if (unlink(file) < 0 && errno != ENOENT && errno != ENOTDIR)
>>>>              VIR_DEBUG("Failed to remove domain XML for %s", vm->def->name);
>>>> @@ -901,6 +912,7 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
>>>>      virDomainDefPtr def = NULL;
>>>>      virObjectEventPtr event = NULL;
>>>>      libxlSavefileHeader hdr;
>>>> +    ssize_t i;
>>>>      int ret = -1;
>>>>      uint32_t domid = 0;
>>>>      char *dom_xml = NULL;
>>>> @@ -1023,6 +1035,20 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
>>>>       */
>>>>      vm->def->id = domid;
>>>>  
>>>> +    for (i = 0; i < vm->def->nnets; i++) {
>>>> +        virDomainNetDefPtr net = vm->def->nets[i];
>>>> +        libxl_device_nic *x_nic = &d_config.nics[i];
>>>> +        const char *suffix =
>>>> +            x_nic->nictype != LIBXL_NIC_TYPE_VIF ? "-emu" : "";
>>>> +
>>>> +        if (net->ifname)
>>>> +            continue;
>>>> +
>>>> +        if (virAsprintf(&net->ifname, "vif%d.%d%s",
>>>> +                        domid, x_nic->devid, suffix) < 0)
>>>> +            continue;
>>>> +    }
>>>> +
>>> This could be done in the callback, if we added the libxl_domain_config object
>>> to the libxlDomainObjPrivate struct. Currently we create, use, and dispose the
>>> object in libxlDomainStart, but it would probably be useful keep the object
>>> around while the domain is active.
>> That's a good idea. This chunk would definitely be better placed in ConsoleCallback.
> 
> Agreed, with ConsoleCallback being renamed to something a bit more generic like
> DomainStartCallback or similar.
> 
OK.

>>
>>> Actually, you could directly use the object
>>> in libxlDomainInterfaceStats if it was included in the libxlDomainObjPrivate
>>> struct. I realize it is a bit more work than this or the V1 approach, but what
>>> do you think about it?
>> Huum, IIUC we would be doing similar to V1, with the difference that we would do
>> it more reliably by knowning devid in advance through usage of
>> libxl_domain_config. The good thing about this version though is that it
>> simplifies things simple for interfaceStats and getAllDomainStats, since all it
>> takes is just to compare against the interface name we calculated beforehand on
>> domain start. Moreover we can actually see what interfaces each domain has when
>> doing domiflist as a result of setting net->ifname (right now only "-" would
>> show up in the name).
> 
> Yes, good point. I think it is best to generate the name when starting the VM.
> Other drivers take the same approach.
> 
> So it looks like this simple patch could become a series in itself: one patch
> adding libxl_domain_config to the libxlDomainObjPrivate struct, another patch to
> rename ConsoleCallback to a more generic name, and finally this patch
> implementing virDomainInterfaceStats on top of the others.
Cool, Thanks for the guideline. Will send it over this new series!

Regards,
Joao

> 
> Regards,
> Jim
> 

Re: [PATCH v3 2/8] libxl: implement virDomainMemorystats

[Jim Fehlig]

On 11/13/2015 06:14 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>
> ---
> Changes since v1:
>  - Cleanup properly after error fetching domain stats
>  - Dispose libxl_dominfo after succesfull call to
>  libxl_domain_info()
>  - Bump version to 1.2.22
> ---
>  src/libxl/libxl_driver.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 70 insertions(+)
>
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index 50f6e34..f4fc7bc 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -4749,6 +4749,75 @@ 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);
> +        goto endjob;
> +    }
> +    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;
> +
> +    libxl_dominfo_dispose(&d_info);
> +
> + 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,
> @@ -5342,6 +5411,7 @@ static virHypervisorDriver libxlHypervisorDriver = {
>  #endif
>      .nodeGetFreeMemory = libxlNodeGetFreeMemory, /* 0.9.0 */
>      .nodeGetCellsFreeMemory = libxlNodeGetCellsFreeMemory, /* 1.1.1 */
> +    .domainMemoryStats = libxlDomainMemoryStats, /* 1.2.22 */

ACK to this patch. I'd push it but I'm unsure about the version.

libvirt devs: Will the next release be 1.2.22 or 1.3.0?

Regards,
Jim

Re: [PATCH v3 2/8] libxl: implement virDomainMemorystats

[Jim Fehlig]

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>
> ---
> Changes since v1:
>  - Cleanup properly after error fetching domain stats
>  - Dispose libxl_dominfo after succesfull call to
>  libxl_domain_info()
>  - Bump version to 1.2.22
> ---
>  src/libxl/libxl_driver.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 70 insertions(+)
> 
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index 50f6e34..f4fc7bc 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -4749,6 +4749,75 @@ 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);
> +        goto endjob;
> +    }
> +    mem = d_info.current_memkb;
> +    maxmem = d_info.max_memkb;
> +
> +    LIBXL_SET_MEMSTAT(VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON, maxmem - mem);

Should this just be 'mem'? On a domain with

  <memory unit='KiB'>1048576</memory>
  <currentMemory unit='KiB'>1048576</currentMemory>

I see

# virsh dommemstat test
actual 1024
available 1049600
rss 1048576

The documentation for VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON only says "Current
balloon value (in KB)". I (perhaps incorrectly) interpret that as the amount of
memory currently assigned to the domain.

Also, I wonder if we should provide rss for Xen domains? I'm not sure it makes
much sense since Xen domains are more than just a process running on the host.
AFAIK, rss would always be set to d_info.current_memkb even if a domain was not
actually using all the memory.

Regards,
Jim

> +    LIBXL_SET_MEMSTAT(VIR_DOMAIN_MEMORY_STAT_AVAILABLE, maxmem);
> +    LIBXL_SET_MEMSTAT(VIR_DOMAIN_MEMORY_STAT_RSS, mem);
> +
> +    ret = i;
> +
> +    libxl_dominfo_dispose(&d_info);
> +
> + 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,
> @@ -5342,6 +5411,7 @@ static virHypervisorDriver libxlHypervisorDriver = {
>  #endif
>      .nodeGetFreeMemory = libxlNodeGetFreeMemory, /* 0.9.0 */
>      .nodeGetCellsFreeMemory = libxlNodeGetCellsFreeMemory, /* 1.1.1 */
> +    .domainMemoryStats = libxlDomainMemoryStats, /* 1.2.22 */
>      .domainGetCPUStats = libxlDomainGetCPUStats, /* 1.2.22 */
>      .connectDomainEventRegister = libxlConnectDomainEventRegister, /* 0.9.0 */
>      .connectDomainEventDeregister = libxlConnectDomainEventDeregister, /* 0.9.0 */

Re: [PATCH v3 2/8] libxl: implement virDomainMemorystats

[Joao Martins]

On 11/17/2015 11:15 PM, Jim Fehlig wrote:
> 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>
>> ---
>> Changes since v1:
>>  - Cleanup properly after error fetching domain stats
>>  - Dispose libxl_dominfo after succesfull call to
>>  libxl_domain_info()
>>  - Bump version to 1.2.22
>> ---
>>  src/libxl/libxl_driver.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 70 insertions(+)
>>
>> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
>> index 50f6e34..f4fc7bc 100644
>> --- a/src/libxl/libxl_driver.c
>> +++ b/src/libxl/libxl_driver.c
>> @@ -4749,6 +4749,75 @@ 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);
>> +        goto endjob;
>> +    }
>> +    mem = d_info.current_memkb;
>> +    maxmem = d_info.max_memkb;
>> +
>> +    LIBXL_SET_MEMSTAT(VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON, maxmem - mem);
> 
> Should this just be 'mem'? On a domain with
> 
>   <memory unit='KiB'>1048576</memory>
>   <currentMemory unit='KiB'>1048576</currentMemory>
> 
> I see
> 
> # virsh dommemstat test
> actual 1024
> available 1049600
> rss 1048576
> 
> The documentation for VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON only says "Current
> balloon value (in KB)". I (perhaps incorrectly) interpret that as the amount of
> memory currently assigned to the domain.
I interpreted that it based on the size of the balloon instead of based on the
memory. But trying out with plain qemu (to see what the monitor socket returns,
since qemu driver takes those values directly) and also libvirt+qemu and it gave
me what you mentioned. VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON is indeed the
amount of memory currently assigned to the domain. Apologies for the confusion.

> 
> Also, I wonder if we should provide rss for Xen domains? I'm not sure it makes
> much sense since Xen domains are more than just a process running on the host.
> AFAIK, rss would always be set to d_info.current_memkb even if a domain was not
> actually using all the memory.
I agree, but I mainly included RSS stat for the lack of a "current memory in
use". But since this is cleared and ACTUAL_BALLOON is not the size of the
balloon but to the actual memory, I guess this renders RSS stat a bit useless,
and perhaps misleading like you suggest. This is the hunk to be applied on top
of this patch, having tested already, and make {syntax-check,test}-ed.

Regards,
Joao

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index db13fd2..c1563c2 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -4799,9 +4799,8 @@ libxlDomainMemoryStats(virDomainPtr dom,
     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_ACTUAL_BALLOON, mem);
     LIBXL_SET_MEMSTAT(VIR_DOMAIN_MEMORY_STAT_AVAILABLE, maxmem);
-    LIBXL_SET_MEMSTAT(VIR_DOMAIN_MEMORY_STAT_RSS, mem);

     ret = i;
> 
> Regards,
> Jim
> 
>> +    LIBXL_SET_MEMSTAT(VIR_DOMAIN_MEMORY_STAT_AVAILABLE, maxmem);
>> +    LIBXL_SET_MEMSTAT(VIR_DOMAIN_MEMORY_STAT_RSS, mem);
>> +
>> +    ret = i;
>> +
>> +    libxl_dominfo_dispose(&d_info);
>> +
>> + 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,
>> @@ -5342,6 +5411,7 @@ static virHypervisorDriver libxlHypervisorDriver = {
>>  #endif
>>      .nodeGetFreeMemory = libxlNodeGetFreeMemory, /* 0.9.0 */
>>      .nodeGetCellsFreeMemory = libxlNodeGetCellsFreeMemory, /* 1.1.1 */
>> +    .domainMemoryStats = libxlDomainMemoryStats, /* 1.2.22 */
>>      .domainGetCPUStats = libxlDomainGetCPUStats, /* 1.2.22 */
>>      .connectDomainEventRegister = libxlConnectDomainEventRegister, /* 0.9.0 */
>>      .connectDomainEventDeregister = libxlConnectDomainEventDeregister, /* 0.9.0 */
> 

Re: [PATCH v3 2/8] libxl: implement virDomainMemorystats

[Jim Fehlig]

On 11/18/2015 11:05 AM, Joao Martins wrote:
>
> On 11/17/2015 11:15 PM, Jim Fehlig wrote:
>> 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>
>>> ---
>>> Changes since v1:
>>>  - Cleanup properly after error fetching domain stats
>>>  - Dispose libxl_dominfo after succesfull call to
>>>  libxl_domain_info()
>>>  - Bump version to 1.2.22
>>> ---
>>>  src/libxl/libxl_driver.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 70 insertions(+)
>>>
>>> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
>>> index 50f6e34..f4fc7bc 100644
>>> --- a/src/libxl/libxl_driver.c
>>> +++ b/src/libxl/libxl_driver.c
>>> @@ -4749,6 +4749,75 @@ 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);
>>> +        goto endjob;
>>> +    }
>>> +    mem = d_info.current_memkb;
>>> +    maxmem = d_info.max_memkb;
>>> +
>>> +    LIBXL_SET_MEMSTAT(VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON, maxmem - mem);
>> Should this just be 'mem'? On a domain with
>>
>>   <memory unit='KiB'>1048576</memory>
>>   <currentMemory unit='KiB'>1048576</currentMemory>
>>
>> I see
>>
>> # virsh dommemstat test
>> actual 1024
>> available 1049600
>> rss 1048576
>>
>> The documentation for VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON only says "Current
>> balloon value (in KB)". I (perhaps incorrectly) interpret that as the amount of
>> memory currently assigned to the domain.
> I interpreted that it based on the size of the balloon instead of based on the
> memory. But trying out with plain qemu (to see what the monitor socket returns,
> since qemu driver takes those values directly) and also libvirt+qemu and it gave
> me what you mentioned. VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON is indeed the
> amount of memory currently assigned to the domain. Apologies for the confusion.
>
>> Also, I wonder if we should provide rss for Xen domains? I'm not sure it makes
>> much sense since Xen domains are more than just a process running on the host.
>> AFAIK, rss would always be set to d_info.current_memkb even if a domain was not
>> actually using all the memory.
> I agree, but I mainly included RSS stat for the lack of a "current memory in
> use". But since this is cleared and ACTUAL_BALLOON is not the size of the
> balloon but to the actual memory, I guess this renders RSS stat a bit useless,
> and perhaps misleading like you suggest. This is the hunk to be applied on top
> of this patch, having tested already, and make {syntax-check,test}-ed.
>
> Regards,
> Joao
>
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index db13fd2..c1563c2 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -4799,9 +4799,8 @@ libxlDomainMemoryStats(virDomainPtr dom,
>      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_ACTUAL_BALLOON, mem);
>      LIBXL_SET_MEMSTAT(VIR_DOMAIN_MEMORY_STAT_AVAILABLE, maxmem);
> -    LIBXL_SET_MEMSTAT(VIR_DOMAIN_MEMORY_STAT_RSS, mem);
>
>      ret = i;

Looks good, ACK with that squashed in. I've pushed it now.

Regards,
Jim

Re: [PATCH v3 2/8] libxl: implement virDomainMemorystats

[Konrad Rzeszutek Wilk]

On November 17, 2015 6:15:38 PM EST, Jim Fehlig <jfehlig@suse.com> wrote:
>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>
>> ---
>> Changes since v1:
>>  - Cleanup properly after error fetching domain stats
>>  - Dispose libxl_dominfo after succesfull call to
>>  libxl_domain_info()
>>  - Bump version to 1.2.22
>> ---
>>  src/libxl/libxl_driver.c | 70
>++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 70 insertions(+)
>> 
>> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
>> index 50f6e34..f4fc7bc 100644
>> --- a/src/libxl/libxl_driver.c
>> +++ b/src/libxl/libxl_driver.c
>> @@ -4749,6 +4749,75 @@ 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);
>> +        goto endjob;
>> +    }
>> +    mem = d_info.current_memkb;
>> +    maxmem = d_info.max_memkb;
>> +
>> +    LIBXL_SET_MEMSTAT(VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON, maxmem
>- mem);
>
>Should this just be 'mem'? On a domain with
>
>  <memory unit='KiB'>1048576</memory>
>  <currentMemory unit='KiB'>1048576</currentMemory>
>
>I see
>
># virsh dommemstat test
>actual 1024
>available 1049600
>rss 1048576
>
>The documentation for VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON only says
>"Current
>balloon value (in KB)". I (perhaps incorrectly) interpret that as the
>amount of
>memory currently assigned to the domain.
>
>Also, I wonder if we should provide rss for Xen domains? I'm not sure
>it makes
>much sense since Xen domains are more than just a process running on
>the host.
>AFAIK, rss would always be set to d_info.current_memkb even if a domain
>was not
>actually using all the memory.
>

You could go negative with that - say dom0 has 8GB and your guest has 16GB. From a Linux dom0 POV we are -8GB, and since tools would most likely use unsigned int the sum of RSS would minus total memory would be negative,but that is 0xffff... So the tools might show the guest consuming gobs of memory?


Perhaps just how much QEMU is consuming (if running)?

>Regards,
>Jim
>
>> +    LIBXL_SET_MEMSTAT(VIR_DOMAIN_MEMORY_STAT_AVAILABLE, maxmem);
>> +    LIBXL_SET_MEMSTAT(VIR_DOMAIN_MEMORY_STAT_RSS, mem);
>> +
>> +    ret = i;
>> +
>> +    libxl_dominfo_dispose(&d_info);
>> +
>> + 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,
>> @@ -5342,6 +5411,7 @@ static virHypervisorDriver
>libxlHypervisorDriver = {
>>  #endif
>>      .nodeGetFreeMemory = libxlNodeGetFreeMemory, /* 0.9.0 */
>>      .nodeGetCellsFreeMemory = libxlNodeGetCellsFreeMemory, /* 1.1.1
>*/
>> +    .domainMemoryStats = libxlDomainMemoryStats, /* 1.2.22 */
>>      .domainGetCPUStats = libxlDomainGetCPUStats, /* 1.2.22 */
>>      .connectDomainEventRegister = libxlConnectDomainEventRegister,
>/* 0.9.0 */
>>      .connectDomainEventDeregister =
>libxlConnectDomainEventDeregister, /* 0.9.0 */
>
>
>_______________________________________________
>Xen-devel mailing list
>Xen-devel@lists.xen.org
>http://lists.xen.org/xen-devel

Re: [PATCH v3 1/8] libxl: implement virDomainGetCPUStats

[Jim Fehlig]

On 11/13/2015 06:14 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>
> ---
> Changes since v1:
>  - Remove libxl_vcpuinfo_dispose() in favor or using
>  libxl_vcpuinfo_list_free(), and also removing VIR_FREE call
>  - Dispose libxl_dominfo after succesfull call to
>  libxl_domain_info()
>  - Fixed identation of parameters
>  - Bump version to 1.2.22
> ---
>  src/libxl/libxl_driver.c | 110 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 110 insertions(+)
>
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index fcdcbdb..50f6e34 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"
>  
> @@ -4641,6 +4643,113 @@ 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;
> +    }
> +
> +    if (virTypedParameterAssign(&params[0], VIR_DOMAIN_CPU_STATS_CPUTIME,
> +                                VIR_TYPED_PARAM_ULLONG, d_info.cpu_time) < 0)
> +        nparams = ret;
> +
> +    libxl_dominfo_dispose(&d_info);
> +    return nparams;

Having a local 'ret' variable but returning nparams was a bit confusing to me.
I've changed this in my local branch to

    if (virTypedParameterAssign(&params[0], VIR_DOMAIN_CPU_STATS_CPUTIME,
                                VIR_TYPED_PARAM_ULLONG, d_info.cpu_time) < 0)
        goto cleanup;

    ret = nparams;

 cleanup:
    libxl_dominfo_dispose(&d_info);
    return ret;

> +}
> +
> +static int
> +libxlDomainGetPerCPUStats(libxlDriverPrivatePtr driver,
> +                          virDomainObjPtr vm,
> +                          virTypedParameterPtr params,
> +                          unsigned int nparams,
> +                          int start_cpu,
> +                          unsigned int ncpus)
> +{
> +    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);
> +        return ret;
> +    }
> +
> +    for (i = start_cpu; i < maxcpu && i < ncpus; ++i) {
> +        if (virTypedParameterAssign(&params[(i-start_cpu)],
> +                                    VIR_DOMAIN_CPU_STATS_CPUTIME,
> +                                    VIR_TYPED_PARAM_ULLONG,
> +                                    vcpuinfo[i].vcpu_time) < 0)
> +            goto cleanup;
> +    }
> +    ret = nparams;
> +
> + cleanup:
> +    libxl_vcpuinfo_list_free(vcpuinfo, maxcpu);
> +    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)
> @@ -5233,6 +5342,7 @@ static virHypervisorDriver libxlHypervisorDriver = {
>  #endif
>      .nodeGetFreeMemory = libxlNodeGetFreeMemory, /* 0.9.0 */
>      .nodeGetCellsFreeMemory = libxlNodeGetCellsFreeMemory, /* 1.1.1 */
> +    .domainGetCPUStats = libxlDomainGetCPUStats, /* 1.2.22 */

ACK with the above nit fixed. I'll push this one too after verifying whether
next release is 1.2.22 or 1.3.0.

Regards,
Jim

Re: [PATCH v3 1/8] libxl: implement virDomainGetCPUStats

[Joao Martins]

On 11/17/2015 02:59 AM, Jim Fehlig wrote:
> On 11/13/2015 06:14 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>
>> ---
>> Changes since v1:
>>  - Remove libxl_vcpuinfo_dispose() in favor or using
>>  libxl_vcpuinfo_list_free(), and also removing VIR_FREE call
>>  - Dispose libxl_dominfo after succesfull call to
>>  libxl_domain_info()
>>  - Fixed identation of parameters
>>  - Bump version to 1.2.22
>> ---
>>  src/libxl/libxl_driver.c | 110 +++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 110 insertions(+)
>>
>> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
>> index fcdcbdb..50f6e34 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"
>>  
>> @@ -4641,6 +4643,113 @@ 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;
>> +    }
>> +
>> +    if (virTypedParameterAssign(&params[0], VIR_DOMAIN_CPU_STATS_CPUTIME,
>> +                                VIR_TYPED_PARAM_ULLONG, d_info.cpu_time) < 0)
>> +        nparams = ret;
>> +
>> +    libxl_dominfo_dispose(&d_info);
>> +    return nparams;
> 
> Having a local 'ret' variable but returning nparams was a bit confusing to me.
> I've changed this in my local branch to
> 
>     if (virTypedParameterAssign(&params[0], VIR_DOMAIN_CPU_STATS_CPUTIME,
>                                 VIR_TYPED_PARAM_ULLONG, d_info.cpu_time) < 0)
>         goto cleanup;
> 
>     ret = nparams;
> 
>  cleanup:
>     libxl_dominfo_dispose(&d_info);
>     return ret;
> 
OK. I solely did it to avoid the additional goto, but wasn't so clear in the end.

>> +}
>> +
>> +static int
>> +libxlDomainGetPerCPUStats(libxlDriverPrivatePtr driver,
>> +                          virDomainObjPtr vm,
>> +                          virTypedParameterPtr params,
>> +                          unsigned int nparams,
>> +                          int start_cpu,
>> +                          unsigned int ncpus)
>> +{
>> +    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);
>> +        return ret;
>> +    }
>> +
>> +    for (i = start_cpu; i < maxcpu && i < ncpus; ++i) {
>> +        if (virTypedParameterAssign(&params[(i-start_cpu)],
>> +                                    VIR_DOMAIN_CPU_STATS_CPUTIME,
>> +                                    VIR_TYPED_PARAM_ULLONG,
>> +                                    vcpuinfo[i].vcpu_time) < 0)
>> +            goto cleanup;
>> +    }
>> +    ret = nparams;
>> +
>> + cleanup:
>> +    libxl_vcpuinfo_list_free(vcpuinfo, maxcpu);
>> +    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)
>> @@ -5233,6 +5342,7 @@ static virHypervisorDriver libxlHypervisorDriver = {
>>  #endif
>>      .nodeGetFreeMemory = libxlNodeGetFreeMemory, /* 0.9.0 */
>>      .nodeGetCellsFreeMemory = libxlNodeGetCellsFreeMemory, /* 1.1.1 */
>> +    .domainGetCPUStats = libxlDomainGetCPUStats, /* 1.2.22 */
> 
> ACK with the above nit fixed. I'll push this one too after verifying whether
> next release is 1.2.22 or 1.3.0.
Great, Thanks! :D

Cheers,
Joao
> 
> Regards,
> Jim
> 

Re: [libvirt] [PATCH v3 1/8] libxl: implement virDomainGetCPUStats

[Jim Fehlig]

On 11/16/2015 07:59 PM, Jim Fehlig wrote:
> On 11/13/2015 06:14 AM, Joao Martins wrote:
> @@ -5233,6 +5342,7 @@ static virHypervisorDriver libxlHypervisorDriver = {
>  #endif
>      .nodeGetFreeMemory = libxlNodeGetFreeMemory, /* 0.9.0 */
>      .nodeGetCellsFreeMemory = libxlNodeGetCellsFreeMemory, /* 1.1.1 */
> +    .domainGetCPUStats = libxlDomainGetCPUStats, /* 1.2.22 */
> ACK with the above nit fixed. I'll push this one too after verifying whether
> next release is 1.2.22 or 1.3.0.

Opps, forgot to mention that I pushed this yesterday. Thanks!

Regards,
Jim

Re: [libvirt] [PATCH v3 1/8] libxl: implement virDomainGetCPUStats

[Joao Martins]

On 11/18/2015 05:33 PM, Jim Fehlig wrote:
> On 11/16/2015 07:59 PM, Jim Fehlig wrote:
>> On 11/13/2015 06:14 AM, Joao Martins wrote:
>> @@ -5233,6 +5342,7 @@ static virHypervisorDriver libxlHypervisorDriver = {
>>  #endif
>>      .nodeGetFreeMemory = libxlNodeGetFreeMemory, /* 0.9.0 */
>>      .nodeGetCellsFreeMemory = libxlNodeGetCellsFreeMemory, /* 1.1.1 */
>> +    .domainGetCPUStats = libxlDomainGetCPUStats, /* 1.2.22 */
>> ACK with the above nit fixed. I'll push this one too after verifying whether
>> next release is 1.2.22 or 1.3.0.
> 
> Opps, forgot to mention that I pushed this yesterday. Thanks!
> 
Great, so 1.2.22 then!

Joao

> Regards,
> Jim
> 

Re: [PATCH v3 5/8] libxl: implement virDomainBlockStats

[Jim Fehlig]

On 11/13/2015 06:14 AM, 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 devno libxlDiskPathToID is introduced.
> 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>
> ---
> Changes since v1:
>  - Fix identation issues
>  - Set ret to LIBXL_VBD_SECTOR_SIZE
>  - Reuse VIR_STRDUP error instead of doing virReportError
>  when we fail to set stats->backend
>  - Change virAsprintf(...) error checking
>  - Change error to VIR_ERR_OPERATION_FAILED when xenstore path
>  does not exist and when failing to read stat.
>  - Resolve issues with 'make syntax-check' with cppi installed.
>  - Remove libxlDiskPathMatches in favor of using virutil
>  virDiskNameParse to fetch disk and partition index.
>  - Rename libxlDiskPathParse to libxlDiskPathToID and rework
>  function to just convert disk and partition index to devno.
>  - Bump version to 1.2.22
> ---
>  configure.ac             |   2 +-
>  src/libxl/libxl_driver.c | 375 +++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 376 insertions(+), 1 deletion(-)
>
> diff --git a/configure.ac b/configure.ac
> index f481c50..10c56e5 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 9a5a74c..ba1d67b 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,
> @@ -4644,6 +4665,358 @@ libxlDomainIsUpdated(virDomainPtr dom)
>  }
>  
>  static int
> +libxlDiskPathToID(const char *virtpath)
> +{
> +    static char const* drive_prefix[] = {"xvd", "hd", "sd"};
> +    int disk, partition, chrused;
> +    int fmt, id;
> +    char *r;
> +    size_t i;
> +
> +    fmt = id = -1;
> +
> +    /* Find any disk prefixes we know about */
> +    for (i = 0; i < ARRAY_CARDINALITY(drive_prefix); i++) {
> +        if (STRPREFIX(virtpath, drive_prefix[i]) &&
> +            !virDiskNameParse(virtpath, &disk, &partition)) {
> +            fmt = i;
> +            break;
> +        }
> +    }
> +
> +    /* Handle it same way as xvd */
> +    if (fmt < 0 &&
> +        (sscanf(virtpath, "d%ip%i%n", &disk, &partition, &chrused) >= 2
> +         && chrused == strlen(virtpath)))
> +        fmt = 0;
> +
> +    /* Test indexes ranges and calculate the device id */
> +    switch (fmt) {
> +    case 0: /* xvd */
> +        if (disk <= 15 && partition <= 15)
> +            id = (202 << 8) | (disk << 4) | partition;
> +        else if ((disk <= ((1<<20)-1)) || partition <= 255)
> +            id = (1 << 28) | (disk << 8) | partition;
> +        break;
> +    case 1: /* hd */
> +        if (disk <= 3 && partition <= 63)
> +            id = ((disk < 2 ? 3 : 22) << 8) | ((disk & 1) << 6) | partition;
> +        break;
> +    case 2: /* sd */
> +        if (disk <= 15 && (partition <= 15))
> +            id = (8 << 8) | (disk << 4) | partition;
> +        break;
> +    default:
> +        errno = virStrToLong_i(virtpath, &r, 0, &id);
> +        if (errno || *r || id > INT_MAX)
> +            id = -1;
> +        break;
> +    }
> +    return id;
> +}
> +
> +#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"));

Probably no need to report an error here since LIBXL_VBD_SECTOR_SIZE is returned
and used by the caller. Maybe a VIR_WARN is more appropriate.

> +        return ret;
> +    }
> +
> +    if (virAsprintf(&path, "/local/domain/%d/device/vbd/%d/backend",
> +                    domid, devno) < 0)
> +        goto close;

Perhaps a bit cleaner to initialize path and val to NULL then have a single
'cleanup' label where they are freed. Then you would only need to VIR_FREE path
and val before re-purposing them.

> +
> +    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 = LIBXL_VBD_SECTOR_SIZE;
> +
> +    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 = libxlDiskPathToID(dev);
> +    int size = libxlDiskSectorSize(vm->def->id, devno);
> +#ifdef __linux__
> +    char *path, *name, *val;
> +    unsigned long long stat;
> +
> +    path = name = val = NULL;
> +    if (devno < 0) {
> +        virReportError(VIR_ERR_OPERATION_INVALID,
> +                       "%s", _("cannot find device number"));
> +        return ret;
> +    }
> +    if (VIR_STRDUP(stats->backend, "vbd") < 0)
> +        return ret;
> +
> +    if (virAsprintf(&path, "/sys/bus/xen-backend/devices/vbd-%d-%d/statistics",
> +                    vm->def->id, devno) < 0)
> +        return ret;
> +
> +    if (!virFileExists(path)) {
> +        virReportError(VIR_ERR_OPERATION_FAILED,
> +                       "%s", _("cannot open bus path"));
> +        goto cleanup;
> +    }
> +
> +# define LIBXL_SET_VBDSTAT(FIELD, VAR, MUL)           \
> +    if ((virAsprintf(&name, "%s/"FIELD, path) < 0) || \
> +        (virFileReadAll(name, 256, &val) < 0) ||      \
> +        (sscanf(val, "%llu", &stat) != 1)) {          \
> +        virReportError(VIR_ERR_OPERATION_FAILED,     \
> +                       _("cannot read %s"), name);    \
> +        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);
> +
> +# undef LIBXL_SET_VBDSTAT
> +
> +#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;

Indentation looks off.

> +    }
> +    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;
> +    }

Currently, libxlDomainObjEndJob returns false when the virDomainObj refcnt has
reached 0, in which case it will be disposed and doesn't need unlocked. Notice
the pattern in all the other calls to libxlDomainObjEndJob. (I say 'currently'
because I'm working on a patch to change the ref counting and job handling
similar to commit 540c339a for the QEMU driver.)

> +
> + 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;
> +    }

Same comment here.

Regards,
Jim

Re: [PATCH v3 5/8] libxl: implement virDomainBlockStats

[Joao Martins]

On 11/18/2015 07:01 PM, Jim Fehlig wrote:
> On 11/13/2015 06:14 AM, 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 devno libxlDiskPathToID is introduced.
>> 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>
>> ---
>> Changes since v1:
>>  - Fix identation issues
>>  - Set ret to LIBXL_VBD_SECTOR_SIZE
>>  - Reuse VIR_STRDUP error instead of doing virReportError
>>  when we fail to set stats->backend
>>  - Change virAsprintf(...) error checking
>>  - Change error to VIR_ERR_OPERATION_FAILED when xenstore path
>>  does not exist and when failing to read stat.
>>  - Resolve issues with 'make syntax-check' with cppi installed.
>>  - Remove libxlDiskPathMatches in favor of using virutil
>>  virDiskNameParse to fetch disk and partition index.
>>  - Rename libxlDiskPathParse to libxlDiskPathToID and rework
>>  function to just convert disk and partition index to devno.
>>  - Bump version to 1.2.22
>> ---
>>  configure.ac             |   2 +-
>>  src/libxl/libxl_driver.c | 375 +++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 376 insertions(+), 1 deletion(-)
>>
>> diff --git a/configure.ac b/configure.ac
>> index f481c50..10c56e5 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 9a5a74c..ba1d67b 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,
>> @@ -4644,6 +4665,358 @@ libxlDomainIsUpdated(virDomainPtr dom)
>>  }
>>  
>>  static int
>> +libxlDiskPathToID(const char *virtpath)
>> +{
>> +    static char const* drive_prefix[] = {"xvd", "hd", "sd"};
>> +    int disk, partition, chrused;
>> +    int fmt, id;
>> +    char *r;
>> +    size_t i;
>> +
>> +    fmt = id = -1;
>> +
>> +    /* Find any disk prefixes we know about */
>> +    for (i = 0; i < ARRAY_CARDINALITY(drive_prefix); i++) {
>> +        if (STRPREFIX(virtpath, drive_prefix[i]) &&
>> +            !virDiskNameParse(virtpath, &disk, &partition)) {
>> +            fmt = i;
>> +            break;
>> +        }
>> +    }
>> +
>> +    /* Handle it same way as xvd */
>> +    if (fmt < 0 &&
>> +        (sscanf(virtpath, "d%ip%i%n", &disk, &partition, &chrused) >= 2
>> +         && chrused == strlen(virtpath)))
>> +        fmt = 0;
>> +
>> +    /* Test indexes ranges and calculate the device id */
>> +    switch (fmt) {
>> +    case 0: /* xvd */
>> +        if (disk <= 15 && partition <= 15)
>> +            id = (202 << 8) | (disk << 4) | partition;
>> +        else if ((disk <= ((1<<20)-1)) || partition <= 255)
>> +            id = (1 << 28) | (disk << 8) | partition;
>> +        break;
>> +    case 1: /* hd */
>> +        if (disk <= 3 && partition <= 63)
>> +            id = ((disk < 2 ? 3 : 22) << 8) | ((disk & 1) << 6) | partition;
>> +        break;
>> +    case 2: /* sd */
>> +        if (disk <= 15 && (partition <= 15))
>> +            id = (8 << 8) | (disk << 4) | partition;
>> +        break;
>> +    default:
>> +        errno = virStrToLong_i(virtpath, &r, 0, &id);
>> +        if (errno || *r || id > INT_MAX)
>> +            id = -1;
>> +        break;
>> +    }
>> +    return id;
>> +}
>> +
>> +#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"));
> 
> Probably no need to report an error here since LIBXL_VBD_SECTOR_SIZE is returned
> and used by the caller. Maybe a VIR_WARN is more appropriate.
> 
OK.

>> +        return ret;
>> +    }
>> +
>> +    if (virAsprintf(&path, "/local/domain/%d/device/vbd/%d/backend",
>> +                    domid, devno) < 0)
>> +        goto close;
> 
> Perhaps a bit cleaner to initialize path and val to NULL then have a single
> 'cleanup' label where they are freed. Then you would only need to VIR_FREE path
> and val before re-purposing them.
>
Good point. I will rework it that way.

>> +
>> +    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 = LIBXL_VBD_SECTOR_SIZE;
>> +
>> +    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 = libxlDiskPathToID(dev);
>> +    int size = libxlDiskSectorSize(vm->def->id, devno);
>> +#ifdef __linux__
>> +    char *path, *name, *val;
>> +    unsigned long long stat;
>> +
>> +    path = name = val = NULL;
>> +    if (devno < 0) {
>> +        virReportError(VIR_ERR_OPERATION_INVALID,
>> +                       "%s", _("cannot find device number"));
>> +        return ret;
>> +    }
>> +    if (VIR_STRDUP(stats->backend, "vbd") < 0)
>> +        return ret;
>> +
>> +    if (virAsprintf(&path, "/sys/bus/xen-backend/devices/vbd-%d-%d/statistics",
>> +                    vm->def->id, devno) < 0)
>> +        return ret;
>> +
>> +    if (!virFileExists(path)) {
>> +        virReportError(VIR_ERR_OPERATION_FAILED,
>> +                       "%s", _("cannot open bus path"));
>> +        goto cleanup;
>> +    }
>> +
>> +# define LIBXL_SET_VBDSTAT(FIELD, VAR, MUL)           \
>> +    if ((virAsprintf(&name, "%s/"FIELD, path) < 0) || \
>> +        (virFileReadAll(name, 256, &val) < 0) ||      \
>> +        (sscanf(val, "%llu", &stat) != 1)) {          \
>> +        virReportError(VIR_ERR_OPERATION_FAILED,     \
>> +                       _("cannot read %s"), name);    \
>> +        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);
>> +
>> +# undef LIBXL_SET_VBDSTAT
>> +
>> +#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;
> 
> Indentation looks off.
> 
OK, and also remove this unnecessary return there.

>> +    }
>> +    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;
>> +    }
> 
> Currently, libxlDomainObjEndJob returns false when the virDomainObj refcnt has
> reached 0, in which case it will be disposed and doesn't need unlocked. Notice
> the pattern in all the other calls to libxlDomainObjEndJob. (I say 'currently'
> because I'm working on a patch to change the ref counting and job handling
> similar to commit 540c339a for the QEMU driver.)
> 
You mean the followup of this one right?
(https://www.redhat.com/archives/libvir-list/2015-June/msg00711.html). It
appears I do the same mistake on InterfaceStats and ConnectGetAllDomainStats, so
I will fix that too.

>> +
>> + 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;
>> +    }
> 
> Same comment here.
> 
OK.

> Regards,
> Jim
> 
> 

Re: [PATCH v3 6/8] libxl: implement virConnectGetAllDomainStats

[Jim Fehlig]

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@oracle.com>
> ---
> 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.

> +    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.

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 */

Re: [PATCH v3 6/8] libxl: implement virConnectGetAllDomainStats

[Joao Martins]

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@oracle.com>
>> ---
>> 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 */
> 

Re: [PATCH v3 8/8] libxl: implement virDomainGetJobStats

[Jim Fehlig]

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.

This patch, and 7/8, look good - ACK. Nice to see all the

error : virDomainGetJobInfo:8844 : this function is not supported by the
connection driver: virDomainGetJobInfo

log entries disappear when doing save, migrate, etc :-). But I'll wait until the
freeze is lifted to push them.

Regards,
Jim

> 
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> ---
> Changes since v1:
>  - Fixed indentation on libxlDomainGetJobStats()
>  - s/estimed/estimated/g
>  - Bump version to 1.2.22
> ---
>  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 b0b6ea7..dda14c2 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -5273,6 +5273,58 @@ libxlDomainGetJobInfo(virDomainPtr dom,
>      return ret;
>  }
>  
> +static int
> +libxlDomainGetJobStats(virDomainPtr dom,
> +                       int *type,
> +                       virTypedParameterPtr *params,
> +                       int *nparams,
> +                       unsigned int flags)
> +{
> +    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 estimated completion time
> +     * 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, ...) \
> @@ -6134,6 +6186,7 @@ static virHypervisorDriver libxlHypervisorDriver = {
>      .nodeGetFreeMemory = libxlNodeGetFreeMemory, /* 0.9.0 */
>      .nodeGetCellsFreeMemory = libxlNodeGetCellsFreeMemory, /* 1.1.1 */
>      .domainGetJobInfo = libxlDomainGetJobInfo, /* 1.2.22 */
> +    .domainGetJobStats = libxlDomainGetJobStats, /* 1.2.22 */
>      .domainBlockStats = libxlDomainBlockStats, /* 1.2.22 */
>      .domainBlockStatsFlags = libxlDomainBlockStatsFlags, /* 1.2.22 */
>      .domainInterfaceStats = libxlDomainInterfaceStats, /* 1.2.22 */

Re: [PATCH v3 8/8] libxl: implement virDomainGetJobStats

[Joao Martins]

On 12/03/2015 06:48 PM, Jim Fehlig wrote:
> 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.
> 
> This patch, and 7/8, look good - ACK. Nice to see all the
> 
> error : virDomainGetJobInfo:8844 : this function is not supported by the
> connection driver: virDomainGetJobInfo
> 
> log entries disappear when doing save, migrate, etc :-). But I'll wait until the
> freeze is lifted to push them.
Great! And it's also nice to see migration no longer crashing on Openstack,
since the monitoring of it relies on these API calls. Perhaps in the future with
per-domain libxl logs we could provide better support for migration progress.

Regards,
Joao

> 
> Regards,
> Jim
> 
>>
>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>> ---
>> Changes since v1:
>>  - Fixed indentation on libxlDomainGetJobStats()
>>  - s/estimed/estimated/g
>>  - Bump version to 1.2.22
>> ---
>>  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 b0b6ea7..dda14c2 100644
>> --- a/src/libxl/libxl_driver.c
>> +++ b/src/libxl/libxl_driver.c
>> @@ -5273,6 +5273,58 @@ libxlDomainGetJobInfo(virDomainPtr dom,
>>      return ret;
>>  }
>>  
>> +static int
>> +libxlDomainGetJobStats(virDomainPtr dom,
>> +                       int *type,
>> +                       virTypedParameterPtr *params,
>> +                       int *nparams,
>> +                       unsigned int flags)
>> +{
>> +    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 estimated completion time
>> +     * 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, ...) \
>> @@ -6134,6 +6186,7 @@ static virHypervisorDriver libxlHypervisorDriver = {
>>      .nodeGetFreeMemory = libxlNodeGetFreeMemory, /* 0.9.0 */
>>      .nodeGetCellsFreeMemory = libxlNodeGetCellsFreeMemory, /* 1.1.1 */
>>      .domainGetJobInfo = libxlDomainGetJobInfo, /* 1.2.22 */
>> +    .domainGetJobStats = libxlDomainGetJobStats, /* 1.2.22 */
>>      .domainBlockStats = libxlDomainBlockStats, /* 1.2.22 */
>>      .domainBlockStatsFlags = libxlDomainBlockStatsFlags, /* 1.2.22 */
>>      .domainInterfaceStats = libxlDomainInterfaceStats, /* 1.2.22 */
> 

Re: [PATCH v3 8/8] libxl: implement virDomainGetJobStats

[Jim Fehlig]

On 12/03/2015 12:29 PM, Joao Martins wrote:
>
> On 12/03/2015 06:48 PM, Jim Fehlig wrote:
>> 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.
>> This patch, and 7/8, look good - ACK. Nice to see all the
>>
>> error : virDomainGetJobInfo:8844 : this function is not supported by the
>> connection driver: virDomainGetJobInfo
>>
>> log entries disappear when doing save, migrate, etc :-). But I'll wait until the
>> freeze is lifted to push them.
> Great! And it's also nice to see migration no longer crashing on Openstack,
> since the monitoring of it relies on these API calls.

I've pushed 7 and 8 now that the release is out. Thanks!

Regards,
Jim