From 46a13012c80b732c4a1794bc237cdec7a9a65323 Mon Sep 17 00:00:00 2001

Message-Id: <46a13012c80b732c4a1794bc237cdec7a9a65323@dist-git>

From: Peter Krempa <pkrempa@redhat.com>

Date: Wed, 1 Oct 2014 11:20:21 +0200

Subject: [PATCH] qemu: monitor: return block stats data as a hash to avoid

 disk mixup

https://bugzilla.redhat.com/show_bug.cgi?id=1113116

The current block stats code matched up the disk name with the actual

stats by the order in the data returned from qemu. This unfortunately

isn't right as qemu may return the disks in any order. Fix this by

returning a hash of stats and index them by the disk alias.

(cherry picked from commit 96c0f57a82ef73ca924b7333d1001f05ecf5df86)

Signed-off-by: Jiri Denemark <jdenemar@redhat.com>

---

 src/qemu/qemu_driver.c       |  44 ++++++++---------

 src/qemu/qemu_monitor.c      |  14 ++----

 src/qemu/qemu_monitor.h      |   6 +--

 src/qemu/qemu_monitor_json.c | 111 ++++++++++++++++++++-----------------------

 src/qemu/qemu_monitor_json.h |   4 +-

 5 files changed, 81 insertions(+), 98 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c

index 8e90cc6..1a16da7 100644

--- a/src/qemu/qemu_driver.c

+++ b/src/qemu/qemu_driver.c

@@ -17714,24 +17714,18 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver,

 {

     size_t i;

     int ret = -1;

-    int nstats = dom->def->ndisks;

-    qemuBlockStatsPtr stats = NULL;

+    int rc;

+    virHashTablePtr stats = NULL;

     qemuDomainObjPrivatePtr priv = dom->privateData;

     if (!HAVE_JOB(privflags) || !virDomainObjIsActive(dom))

         return 0; /* it's ok, just go ahead silently */

-    if (VIR_ALLOC_N(stats, nstats) < 0)

-        return -1;

-

     qemuDomainObjEnterMonitor(driver, dom);

-

-    nstats = qemuMonitorGetAllBlockStatsInfo(priv->mon, NULL,

-                                             stats, nstats);

-

+    rc = qemuMonitorGetAllBlockStatsInfo(priv->mon, &stats);

     qemuDomainObjExitMonitor(driver, dom);

-    if (nstats < 0) {

+    if (rc < 0) {

         virResetLastError();

         ret = 0; /* still ok, again go ahead silently */

         goto cleanup;

@@ -17739,32 +17733,38 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver,

     QEMU_ADD_COUNT_PARAM(record, maxparams, "block", dom->def->ndisks);

-    for (i = 0; i < nstats; i++) {

-        QEMU_ADD_NAME_PARAM(record, maxparams,

-                            "block", i, dom->def->disks[i]->dst);

+    for (i = 0; i < dom->def->ndisks; i++) {

+        qemuBlockStats *entry;

+        virDomainDiskDefPtr disk = dom->def->disks[i];

+

+        QEMU_ADD_NAME_PARAM(record, maxparams, "block", i, disk->dst);

+

+        if (!disk->info.alias ||

+            !(entry = virHashLookup(stats, disk->info.alias)))

+            continue;

         QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, i,

-                                "rd.reqs", stats[i].rd_req);

+                                "rd.reqs", entry->rd_req);

         QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, i,

-                                "rd.bytes", stats[i].rd_bytes);

+                                "rd.bytes", entry->rd_bytes);

         QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, i,

-                                "rd.times", stats[i].rd_total_times);

+                                "rd.times", entry->rd_total_times);

         QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, i,

-                                "wr.reqs", stats[i].wr_req);

+                                "wr.reqs", entry->wr_req);

         QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, i,

-                                "wr.bytes", stats[i].wr_bytes);

+                                "wr.bytes", entry->wr_bytes);

         QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, i,

-                                "wr.times", stats[i].wr_total_times);

+                                "wr.times", entry->wr_total_times);

         QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, i,

-                                "fl.reqs", stats[i].flush_req);

+                                "fl.reqs", entry->flush_req);

         QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, i,

-                                "fl.times", stats[i].flush_total_times);

+                                "fl.times", entry->flush_total_times);

     }

     ret = 0;

  cleanup:

-    VIR_FREE(stats);

+    virHashFree(stats);

     return ret;

 }

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c

index afe7959..93e8f22 100644

--- a/src/qemu/qemu_monitor.c

+++ b/src/qemu/qemu_monitor.c

@@ -1767,23 +1767,17 @@ int qemuMonitorGetBlockStatsInfo(qemuMonitorPtr mon,

  */

 int

 qemuMonitorGetAllBlockStatsInfo(qemuMonitorPtr mon,

-                                const char *dev_name,

-                                qemuBlockStatsPtr stats,

-                                int nstats)

+                                virHashTablePtr *ret_stats)

 {

-    int ret;

-    VIR_DEBUG("mon=%p dev=%s", mon, dev_name);

+    VIR_DEBUG("mon=%p ret_stats=%p", mon, ret_stats);

-    if (mon->json) {

-        ret = qemuMonitorJSONGetAllBlockStatsInfo(mon, dev_name,

-                                                  stats, nstats);

-    } else {

+    if (!mon->json) {

         virReportError(VIR_ERR_INTERNAL_ERROR, "%s",

                        _("unable to query all block stats with this QEMU"));

         return -1;

     }

-    return ret;

+    return qemuMonitorJSONGetAllBlockStatsInfo(mon, ret_stats);

 }

 /* Return 0 and update @nparams with the number of block stats

diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h

index b7f9fd1..2551272 100644

--- a/src/qemu/qemu_monitor.h

+++ b/src/qemu/qemu_monitor.h

@@ -361,10 +361,8 @@ struct _qemuBlockStats {

 };

 int qemuMonitorGetAllBlockStatsInfo(qemuMonitorPtr mon,

-                                    const char *dev_name,

-                                    qemuBlockStatsPtr stats,

-                                    int nstats)

-    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3);

+                                    virHashTablePtr *ret_stats)

+    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);

 int qemuMonitorGetBlockStatsParamsNumber(qemuMonitorPtr mon,

                                          int *nparams);

diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c

index c013d49..db553fb 100644

--- a/src/qemu/qemu_monitor_json.c

+++ b/src/qemu/qemu_monitor_json.c

@@ -1708,7 +1708,8 @@ int qemuMonitorJSONGetBlockStatsInfo(qemuMonitorPtr mon,

                                      long long *flush_total_times,

                                      long long *errs)

 {

-    qemuBlockStats stats;

+    qemuBlockStats *stats;

+    virHashTablePtr blockstats = NULL;

     int ret = -1;

     *rd_req = *rd_bytes = -1;

@@ -1723,56 +1724,61 @@ int qemuMonitorJSONGetBlockStatsInfo(qemuMonitorPtr mon,

     if (flush_total_times)

         *flush_total_times = -1;

-    if (qemuMonitorJSONGetAllBlockStatsInfo(mon, dev_name, &stats, 1) != 1)

+    if (qemuMonitorJSONGetAllBlockStatsInfo(mon, &blockstats) < 0)

         goto cleanup;

-    *rd_req = stats.rd_req;

-    *rd_bytes = stats.rd_bytes;

-    *wr_req = stats.wr_req;

-    *wr_bytes = stats.wr_bytes;

+    if (!(stats = virHashLookup(blockstats, dev_name))) {

+        virReportError(VIR_ERR_INTERNAL_ERROR,

+                       _("cannot find statistics for device '%s'"), dev_name);

+        goto cleanup;

+    }

+

+    *rd_req = stats->rd_req;

+    *rd_bytes = stats->rd_bytes;

+    *wr_req = stats->wr_req;

+    *wr_bytes = stats->wr_bytes;

     *errs = -1; /* QEMU does not have this */

     if (rd_total_times)

-        *rd_total_times = stats.rd_total_times;

+        *rd_total_times = stats->rd_total_times;

     if (wr_total_times)

-        *wr_total_times = stats.wr_total_times;

+        *wr_total_times = stats->wr_total_times;

     if (flush_req)

-        *flush_req = stats.flush_req;

+        *flush_req = stats->flush_req;

     if (flush_total_times)

-        *flush_total_times = stats.flush_total_times;

+        *flush_total_times = stats->flush_total_times;

     ret = 0;

  cleanup:

+    virHashFree(blockstats);

     return ret;

 }

 int qemuMonitorJSONGetAllBlockStatsInfo(qemuMonitorPtr mon,

-                                        const char *dev_name,

-                                        qemuBlockStatsPtr bstats,

-                                        int nstats)

+                                        virHashTablePtr *ret_stats)

 {

-    int ret, count;

+    int ret = -1;

+    int rc;

     size_t i;

-    virJSONValuePtr cmd = qemuMonitorJSONMakeCommand("query-blockstats",

-                                                     NULL);

+    virJSONValuePtr cmd;

     virJSONValuePtr reply = NULL;

     virJSONValuePtr devices;

+    qemuBlockStatsPtr bstats = NULL;

+    virHashTablePtr hash = NULL;

-    if (!cmd)

+    if (!(cmd = qemuMonitorJSONMakeCommand("query-blockstats", NULL)))

         return -1;

-    if (!bstats || nstats <= 0)

-        return -1;

+    if (!(hash = virHashCreate(10, virHashValueFree)))

+        goto cleanup;

-    ret = qemuMonitorJSONCommand(mon, cmd, &reply);

+    if ((rc = qemuMonitorJSONCommand(mon, cmd, &reply)) < 0)

+        goto cleanup;

-    if (ret == 0)

-        ret = qemuMonitorJSONCheckError(cmd, reply);

-    if (ret < 0)

+    if (qemuMonitorJSONCheckError(cmd, reply) < 0)

         goto cleanup;

-    ret = -1;

     devices = virJSONValueObjectGet(reply, "return");

     if (!devices || devices->type != VIR_JSON_TYPE_ARRAY) {

@@ -1781,10 +1787,14 @@ int qemuMonitorJSONGetAllBlockStatsInfo(qemuMonitorPtr mon,

         goto cleanup;

     }

-    count = 0;

-    for (i = 0; i < virJSONValueArraySize(devices) && count < nstats; i++) {

+    for (i = 0; i < virJSONValueArraySize(devices); i++) {

         virJSONValuePtr dev = virJSONValueArrayGet(devices, i);

         virJSONValuePtr stats;

+        const char *devname;

+

+        if (VIR_ALLOC(bstats) < 0)

+            goto cleanup;

+

         if (!dev || dev->type != VIR_JSON_TYPE_OBJECT) {

             virReportError(VIR_ERR_INTERNAL_ERROR, "%s",

                            _("blockstats device entry was not "

@@ -1792,29 +1802,16 @@ int qemuMonitorJSONGetAllBlockStatsInfo(qemuMonitorPtr mon,

             goto cleanup;

         }

-        /* If dev_name is specified, we are looking for a specific device,

-         * so we must be stricter.

-         */

-        if (dev_name) {

-            const char *thisdev = virJSONValueObjectGetString(dev, "device");

-            if (!thisdev) {

-                virReportError(VIR_ERR_INTERNAL_ERROR, "%s",

-                               _("blockstats device entry was not "

-                                 "in expected format"));

-                goto cleanup;

-            }

-

-            /* New QEMU has separate names for host & guest side of the disk

-             * and libvirt gives the host side a 'drive-' prefix. The passed

-             * in dev_name is the guest side though

-             */

-            if (STRPREFIX(thisdev, QEMU_DRIVE_HOST_PREFIX))

-                thisdev += strlen(QEMU_DRIVE_HOST_PREFIX);

-

-            if (STRNEQ(thisdev, dev_name))

-                continue;

+        if (!(devname = virJSONValueObjectGetString(dev, "device"))) {

+            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",

+                           _("blockstats device entry was not "

+                             "in expected format"));

+            goto cleanup;

         }

+        if (STRPREFIX(devname, QEMU_DRIVE_HOST_PREFIX))

+            devname += strlen(QEMU_DRIVE_HOST_PREFIX);

+

         if ((stats = virJSONValueObjectGet(dev, "stats")) == NULL ||

             stats->type != VIR_JSON_TYPE_OBJECT) {

             virReportError(VIR_ERR_INTERNAL_ERROR, "%s",

@@ -1884,22 +1881,18 @@ int qemuMonitorJSONGetAllBlockStatsInfo(qemuMonitorPtr mon,

             goto cleanup;

         }

-        count++;

-        bstats++;

-

-        if (dev_name && count)

-            break;

-    }

-

-    if (dev_name && !count) {

-        virReportError(VIR_ERR_INTERNAL_ERROR,

-                       _("cannot find statistics for device '%s'"), dev_name);

-        goto cleanup;

+        if (virHashAddEntry(hash, devname, bstats) < 0)

+            goto cleanup;

+        bstats = NULL;

     }

-    ret = count;

+    *ret_stats = hash;

+    hash = NULL;

+    ret = 0;

  cleanup:

+    VIR_FREE(bstats);

+    virHashFree(hash);

     virJSONValueFree(cmd);

     virJSONValueFree(reply);

     return ret;

diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h

index 2402b5a..8e65c4c 100644

--- a/src/qemu/qemu_monitor_json.h

+++ b/src/qemu/qemu_monitor_json.h

@@ -80,9 +80,7 @@ int qemuMonitorJSONGetBlockStatsInfo(qemuMonitorPtr mon,

                                      long long *flush_total_times,

                                      long long *errs);

 int qemuMonitorJSONGetAllBlockStatsInfo(qemuMonitorPtr mon,

-                                        const char *dev_name,

-                                        qemuBlockStatsPtr stats,

-                                        int nstats);

+                                        virHashTablePtr *ret_stats);

 int qemuMonitorJSONGetBlockStatsParamsNumber(qemuMonitorPtr mon,

                                              int *nparams);

 int qemuMonitorJSONGetBlockExtent(qemuMonitorPtr mon,

-- 

2.1.2