[libvirt] [PATCH 0/5] Get rid of qemuMonitorGetBlockExtent
There are other functions that can be used instead so avoid duplication. Peter Krempa (5): internal: Introduce virCheckNonEmptyStringArgGoto and reuse it qemu: monitor: Fix indentation in qemuMonitorJSONGetOneBlockStatsInfo qemu: monitor: Open-code retrieval of wr_highest_offset qemu: Refactor qemuDomainGetBlockInfo qemu: monitor: Remove qemuMonitorGetBlockExtent src/internal.h | 11 +++ src/libvirt-domain.c | 4 +- src/qemu/qemu_driver.c | 101 +++++++++++++------------- src/qemu/qemu_monitor.c | 16 ----- src/qemu/qemu_monitor.h | 4 +- src/qemu/qemu_monitor_json.c | 164 +++++-------------------------------------- src/qemu/qemu_monitor_json.h | 3 - src/qemu/qemu_monitor_text.c | 10 --- src/qemu/qemu_monitor_text.h | 3 - src/util/virerror.h | 11 +++ tests/qemumonitorjsontest.c | 54 ++++---------- tests/qemumonitortest.c | 10 +-- 12 files changed, 114 insertions(+), 277 deletions(-) -- 2.4.1
The helper makes sure that strings passed to APIs are non-NULL and non-empty. This allows to drop some inlined checks where it does not make sense. --- src/internal.h | 11 +++++++++++ src/libvirt-domain.c | 4 ++-- src/qemu/qemu_driver.c | 11 ----------- src/util/virerror.h | 11 +++++++++++ 4 files changed, 24 insertions(+), 13 deletions(-) diff --git a/src/internal.h b/src/internal.h index 7c042e0..db26fb0 100644 --- a/src/internal.h +++ b/src/internal.h @@ -446,6 +446,17 @@ goto label; \ } \ } while (0) +# define virCheckNonEmptyStringArgGoto(argname, label) \ + do { \ + if (argname == NULL) { \ + virReportInvalidNonNullArg(argname); \ + goto label; \ + } \ + if (*argname == '\0') { \ + virReportInvalidEmptyStringArg(argname); \ + goto label; \ + } \ + } while (0) # define virCheckPositiveArgGoto(argname, label) \ do { \ if (argname <= 0) { \ diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 4d7b88a..909c264 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -6065,7 +6065,7 @@ virDomainBlockPeek(virDomainPtr dom, conn = dom->conn; virCheckReadOnlyGoto(conn->flags, error); - virCheckNonNullArgGoto(disk, error); + virCheckNonEmptyStringArgGoto(disk, error); /* Allow size == 0 as an access test. */ if (size > 0) @@ -6333,7 +6333,7 @@ virDomainGetBlockInfo(virDomainPtr domain, const char *disk, memset(info, 0, sizeof(*info)); virCheckDomainReturn(domain, -1); - virCheckNonNullArgGoto(disk, error); + virCheckNonEmptyStringArgGoto(disk, error); virCheckNonNullArgGoto(info, error); conn = domain->conn; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c1373de..004da7e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11530,12 +11530,6 @@ qemuDomainBlockPeek(virDomainPtr dom, if (virDomainBlockPeekEnsureACL(dom->conn, vm->def) < 0) goto cleanup; - if (!path || path[0] == '\0') { - virReportError(VIR_ERR_INVALID_ARG, - "%s", _("NULL or empty path")); - goto cleanup; - } - /* Check the path belongs to this domain. */ if (!(actual = virDomainDiskPathByName(vm->def, path))) { virReportError(VIR_ERR_INVALID_ARG, @@ -11821,11 +11815,6 @@ qemuDomainGetBlockInfo(virDomainPtr dom, if (virDomainGetBlockInfoEnsureACL(dom->conn, vm->def) < 0) goto cleanup; - if (!path || path[0] == '\0') { - virReportError(VIR_ERR_INVALID_ARG, "%s", _("NULL or empty path")); - goto cleanup; - } - /* Technically, we only need a job if we are going to query the * monitor, which is only for active domains that are using * non-raw block devices. But it is easier to share code if we diff --git a/src/util/virerror.h b/src/util/virerror.h index ad3a946..c1a445e 100644 --- a/src/util/virerror.h +++ b/src/util/virerror.h @@ -95,6 +95,17 @@ void virReportSystemErrorFull(int domcode, 0, 0, \ _("%s in %s must not be NULL"), \ #argname, __FUNCTION__) +# define virReportInvalidEmptyStringArg(argname) \ + virRaiseErrorFull(__FILE__, __FUNCTION__, __LINE__, \ + VIR_FROM_THIS, \ + VIR_ERR_INVALID_ARG, \ + VIR_ERR_ERROR, \ + __FUNCTION__, \ + #argname, \ + NULL, \ + 0, 0, \ + _("string %s in %s must not be non empty"), \ + #argname, __FUNCTION__) # define virReportInvalidPositiveArg(argname) \ virRaiseErrorFull(__FILE__, __FUNCTION__, __LINE__, \ VIR_FROM_THIS, \ -- 2.4.1
On 06/23/2015 01:15 PM, Peter Krempa wrote:
The helper makes sure that strings passed to APIs are non-NULL and non-empty. This allows to drop some inlined checks where it does not make sense. --- src/internal.h | 11 +++++++++++ src/libvirt-domain.c | 4 ++-- src/qemu/qemu_driver.c | 11 ----------- src/util/virerror.h | 11 +++++++++++ 4 files changed, 24 insertions(+), 13 deletions(-)
...
diff --git a/src/util/virerror.h b/src/util/virerror.h index ad3a946..c1a445e 100644 --- a/src/util/virerror.h +++ b/src/util/virerror.h @@ -95,6 +95,17 @@ void virReportSystemErrorFull(int domcode, 0, 0, \ _("%s in %s must not be NULL"), \ #argname, __FUNCTION__) +# define virReportInvalidEmptyStringArg(argname) \ + virRaiseErrorFull(__FILE__, __FUNCTION__, __LINE__, \ + VIR_FROM_THIS, \ + VIR_ERR_INVALID_ARG, \ + VIR_ERR_ERROR, \ + __FUNCTION__, \ + #argname, \ + NULL, \ + 0, 0, \ + _("string %s in %s must not be non empty"), \
This reads strangely... The double negative (not and non) "string %s in %s must not be empty" or "string %s in %s must be non empty" Or more technically "string %s in %s cannot start with the NUL character" (yes, correct spelling for NUL - that's the '\0')
+ #argname, __FUNCTION__) # define virReportInvalidPositiveArg(argname) \ virRaiseErrorFull(__FILE__, __FUNCTION__, __LINE__, \ VIR_FROM_THIS, \
ACK with some sort of change John
On Thu, Jun 25, 2015 at 12:23:43 -0400, John Ferlan wrote:
On 06/23/2015 01:15 PM, Peter Krempa wrote:
The helper makes sure that strings passed to APIs are non-NULL and non-empty. This allows to drop some inlined checks where it does not make sense. --- src/internal.h | 11 +++++++++++ src/libvirt-domain.c | 4 ++-- src/qemu/qemu_driver.c | 11 ----------- src/util/virerror.h | 11 +++++++++++ 4 files changed, 24 insertions(+), 13 deletions(-)
...
diff --git a/src/util/virerror.h b/src/util/virerror.h index ad3a946..c1a445e 100644 --- a/src/util/virerror.h +++ b/src/util/virerror.h @@ -95,6 +95,17 @@ void virReportSystemErrorFull(int domcode, 0, 0, \ _("%s in %s must not be NULL"), \ #argname, __FUNCTION__) +# define virReportInvalidEmptyStringArg(argname) \ + virRaiseErrorFull(__FILE__, __FUNCTION__, __LINE__, \ + VIR_FROM_THIS, \ + VIR_ERR_INVALID_ARG, \ + VIR_ERR_ERROR, \ + __FUNCTION__, \ + #argname, \ + NULL, \ + 0, 0, \ + _("string %s in %s must not be non empty"), \
This reads strangely... The double negative (not and non)
"string %s in %s must not be empty" or "string %s in %s must be non empty"
I went with the first option. I goofed when I tried to touch up the message after stealing it from an existing macro :)
Or more technically
"string %s in %s cannot start with the NUL character" (yes, correct spelling for NUL - that's the '\0')
Yep, but this option does not sound user-friendly to me.
+ #argname, __FUNCTION__) # define virReportInvalidPositiveArg(argname) \ virRaiseErrorFull(__FILE__, __FUNCTION__, __LINE__, \ VIR_FROM_THIS, \
ACK with some sort of change
Thanks. Peter
--- src/qemu/qemu_monitor_json.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 71e8f4b..11c45a1 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -1725,14 +1725,14 @@ qemuMonitorJSONGetOneBlockStatsInfo(virJSONValuePtr dev, goto cleanup; \ } \ } - QEMU_MONITOR_BLOCK_STAT_GET("rd_bytes", bstats->rd_bytes, true); - QEMU_MONITOR_BLOCK_STAT_GET("wr_bytes", bstats->wr_bytes, true); - QEMU_MONITOR_BLOCK_STAT_GET("rd_operations", bstats->rd_req, true); - QEMU_MONITOR_BLOCK_STAT_GET("wr_operations", bstats->wr_req, true); - QEMU_MONITOR_BLOCK_STAT_GET("rd_total_time_ns", bstats->rd_total_times, false); - QEMU_MONITOR_BLOCK_STAT_GET("wr_total_time_ns", bstats->wr_total_times, false); - QEMU_MONITOR_BLOCK_STAT_GET("flush_operations", bstats->flush_req, false); - QEMU_MONITOR_BLOCK_STAT_GET("flush_total_time_ns", bstats->flush_total_times, false); + QEMU_MONITOR_BLOCK_STAT_GET("rd_bytes", bstats->rd_bytes, true); + QEMU_MONITOR_BLOCK_STAT_GET("wr_bytes", bstats->wr_bytes, true); + QEMU_MONITOR_BLOCK_STAT_GET("rd_operations", bstats->rd_req, true); + QEMU_MONITOR_BLOCK_STAT_GET("wr_operations", bstats->wr_req, true); + QEMU_MONITOR_BLOCK_STAT_GET("rd_total_time_ns", bstats->rd_total_times, false); + QEMU_MONITOR_BLOCK_STAT_GET("wr_total_time_ns", bstats->wr_total_times, false); + QEMU_MONITOR_BLOCK_STAT_GET("flush_operations", bstats->flush_req, false); + QEMU_MONITOR_BLOCK_STAT_GET("flush_total_time_ns", bstats->flush_total_times, false); #undef QEMU_MONITOR_BLOCK_STAT_GET /* it's ok to not have this information here. Just skip silently. */ -- 2.4.1
Instead of using qemuMonitorJSONDevGetBlockExtent (which I plan to remove later) extract the data in place. Additionally add a flag that will be set when the wr_highest_offset was extracted correctly so that callers can act according to that. The test case addition should help make sure that everything works. --- src/qemu/qemu_monitor.h | 1 + src/qemu/qemu_monitor_json.c | 10 ++++++++-- tests/qemumonitorjsontest.c | 20 +++++++++++++++----- tests/qemumonitortest.c | 10 +++++----- 4 files changed, 29 insertions(+), 12 deletions(-) diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 1afc344..b4d6538 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -379,6 +379,7 @@ struct _qemuBlockStats { unsigned long long capacity; unsigned long long physical; unsigned long long wr_highest_offset; + bool wr_highest_offset_valid; }; int qemuMonitorGetAllBlockStatsInfo(qemuMonitorPtr mon, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 11c45a1..b2ce33f 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -1699,6 +1699,8 @@ qemuMonitorJSONGetOneBlockStatsInfo(virJSONValuePtr dev, { qemuBlockStatsPtr bstats = NULL; virJSONValuePtr stats; + virJSONValuePtr parent; + virJSONValuePtr parentstats; int ret = -1; int nstats = 0; char *entry_name = qemuDomainStorageAlias(dev_name, depth); @@ -1735,8 +1737,12 @@ qemuMonitorJSONGetOneBlockStatsInfo(virJSONValuePtr dev, QEMU_MONITOR_BLOCK_STAT_GET("flush_total_time_ns", bstats->flush_total_times, false); #undef QEMU_MONITOR_BLOCK_STAT_GET - /* it's ok to not have this information here. Just skip silently. */ - qemuMonitorJSONDevGetBlockExtent(dev, &bstats->wr_highest_offset); + if ((parent = virJSONValueObjectGetObject(dev, "parent")) && + (parentstats = virJSONValueObjectGetObject(parent, "stats"))) { + if (virJSONValueObjectGetNumberUlong(parentstats, "wr_highest_offset", + &bstats->wr_highest_offset) == 0) + bstats->wr_highest_offset_valid = true; + } if (virHashAddEntry(hash, entry_name, bstats) < 0) goto cleanup; diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index 0623275..6246737 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -1546,8 +1546,16 @@ testQemuMonitorJSONqemuMonitorJSONGetBlockStatsInfo(const void *data) goto cleanup; \ } +#define CHECK0ULL(var, value) \ + if (stats->var != value) { \ + virReportError(VIR_ERR_INTERNAL_ERROR, \ + "Invalid " #var " value: %llu, expected %llu", \ + stats->var, value); \ + goto cleanup; \ + } + #define CHECK(NAME, RD_REQ, RD_BYTES, RD_TOTAL_TIMES, WR_REQ, WR_BYTES, \ - WR_TOTAL_TIMES, FLUSH_REQ, FLUSH_TOTAL_TIMES) \ + WR_TOTAL_TIMES, FLUSH_REQ, FLUSH_TOTAL_TIMES, WR_HIGHEST_OFFSET) \ if (!(stats = virHashLookup(blockstats, NAME))) { \ virReportError(VIR_ERR_INTERNAL_ERROR, \ "block stats for device '%s' is missing", NAME); \ @@ -1560,7 +1568,8 @@ testQemuMonitorJSONqemuMonitorJSONGetBlockStatsInfo(const void *data) CHECK0(wr_bytes, WR_BYTES) \ CHECK0(wr_total_times, WR_TOTAL_TIMES) \ CHECK0(flush_req, FLUSH_REQ) \ - CHECK0(flush_total_times, FLUSH_TOTAL_TIMES) + CHECK0(flush_total_times, FLUSH_TOTAL_TIMES) \ + CHECK0ULL(wr_highest_offset, WR_HIGHEST_OFFSET) if (qemuMonitorGetAllBlockStatsInfo(qemuMonitorTestGetMonitor(test), &blockstats, false) < 0) @@ -1572,9 +1581,9 @@ testQemuMonitorJSONqemuMonitorJSONGetBlockStatsInfo(const void *data) goto cleanup; } - CHECK("virtio-disk0", 1279, 28505088, 640616474, 174, 2845696, 530699221, 0, 0) - CHECK("virtio-disk1", 85, 348160, 8232156, 0, 0, 0, 0, 0) - CHECK("ide0-1-0", 16, 49250, 1004952, 0, 0, 0, 0, 0) + CHECK("virtio-disk0", 1279, 28505088, 640616474, 174, 2845696, 530699221, 0, 0, 5256018944ULL) + CHECK("virtio-disk1", 85, 348160, 8232156, 0, 0, 0, 0, 0, 0ULL) + CHECK("ide0-1-0", 16, 49250, 1004952, 0, 0, 0, 0, 0, 0ULL) if (qemuMonitorJSONGetBlockExtent(qemuMonitorTestGetMonitor(test), "virtio-disk0", &extent) < 0) @@ -1613,6 +1622,7 @@ testQemuMonitorJSONqemuMonitorJSONGetBlockStatsInfo(const void *data) #undef CHECK #undef CHECK0 +#undef CHECK0ULL cleanup: qemuMonitorTestFree(test); diff --git a/tests/qemumonitortest.c b/tests/qemumonitortest.c index 0125962..9d8d70a 100644 --- a/tests/qemumonitortest.c +++ b/tests/qemumonitortest.c @@ -94,11 +94,11 @@ struct blockInfoData { static const struct blockInfoData testBlockInfoData[] = { /* NAME, rd_req, rd_bytes, wr_req, wr_bytes, rd_total_time, wr_total_time, flush_req, flush_total_time */ - {"vda", {11, 12, 13, 14, 15, 16, 17, 18, 0, 0, 0}}, - {"vdb", {21, 22, 23, 24, 25, 26, 27, 28, 0, 0, 0}}, - {"vdc", {31, 32, 33, -1, 35, 36, 37, 38, 0, 0, 0}}, - {"vdd", {-1, -1, -1, -1, -1, -1, -1, -1, 0, 0, 0}}, - {"vde", {41, 42, 43, 44, 45, 46, 47, 48, 0, 0, 0}} + {"vda", {11, 12, 13, 14, 15, 16, 17, 18, 0, 0, 0, false}}, + {"vdb", {21, 22, 23, 24, 25, 26, 27, 28, 0, 0, 0, false}}, + {"vdc", {31, 32, 33, -1, 35, 36, 37, 38, 0, 0, 0, false}}, + {"vdd", {-1, -1, -1, -1, -1, -1, -1, -1, 0, 0, 0, false}}, + {"vde", {41, 42, 43, 44, 45, 46, 47, 48, 0, 0, 0, false}} }; static const char testBlockInfoReply[] = -- 2.4.1
On 06/23/2015 01:15 PM, Peter Krempa wrote:
Instead of using qemuMonitorJSONDevGetBlockExtent (which I plan to remove later) extract the data in place.
Additionally add a flag that will be set when the wr_highest_offset was extracted correctly so that callers can act according to that.
The test case addition should help make sure that everything works. --- src/qemu/qemu_monitor.h | 1 + src/qemu/qemu_monitor_json.c | 10 ++++++++-- tests/qemumonitorjsontest.c | 20 +++++++++++++++----- tests/qemumonitortest.c | 10 +++++----- 4 files changed, 29 insertions(+), 12 deletions(-)
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 1afc344..b4d6538 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -379,6 +379,7 @@ struct _qemuBlockStats { unsigned long long capacity; unsigned long long physical; unsigned long long wr_highest_offset; + bool wr_highest_offset_valid; };
Maybe some sort of comment that starting at wr_highest_offset, all values require both the data and a bool of the same name, but with _valid ... (see below - perhaps it'll make more sense)
int qemuMonitorGetAllBlockStatsInfo(qemuMonitorPtr mon, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 11c45a1..b2ce33f 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -1699,6 +1699,8 @@ qemuMonitorJSONGetOneBlockStatsInfo(virJSONValuePtr dev, { qemuBlockStatsPtr bstats = NULL; virJSONValuePtr stats; + virJSONValuePtr parent; + virJSONValuePtr parentstats; int ret = -1; int nstats = 0; char *entry_name = qemuDomainStorageAlias(dev_name, depth); @@ -1735,8 +1737,12 @@ qemuMonitorJSONGetOneBlockStatsInfo(virJSONValuePtr dev, QEMU_MONITOR_BLOCK_STAT_GET("flush_total_time_ns", bstats->flush_total_times, false); #undef QEMU_MONITOR_BLOCK_STAT_GET
- /* it's ok to not have this information here. Just skip silently. */ - qemuMonitorJSONDevGetBlockExtent(dev, &bstats->wr_highest_offset); + if ((parent = virJSONValueObjectGetObject(dev, "parent")) && + (parentstats = virJSONValueObjectGetObject(parent, "stats"))) { + if (virJSONValueObjectGetNumberUlong(parentstats, "wr_highest_offset", + &bstats->wr_highest_offset) == 0) + bstats->wr_highest_offset_valid = true; + }
if (virHashAddEntry(hash, entry_name, bstats) < 0) goto cleanup; diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index 0623275..6246737 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -1546,8 +1546,16 @@ testQemuMonitorJSONqemuMonitorJSONGetBlockStatsInfo(const void *data) goto cleanup; \ }
+#define CHECK0ULL(var, value) \ + if (stats->var != value) { \ + virReportError(VIR_ERR_INTERNAL_ERROR, \ + "Invalid " #var " value: %llu, expected %llu", \ + stats->var, value); \ + goto cleanup; \ + } + #define CHECK(NAME, RD_REQ, RD_BYTES, RD_TOTAL_TIMES, WR_REQ, WR_BYTES, \ - WR_TOTAL_TIMES, FLUSH_REQ, FLUSH_TOTAL_TIMES) \ + WR_TOTAL_TIMES, FLUSH_REQ, FLUSH_TOTAL_TIMES, WR_HIGHEST_OFFSET) \ if (!(stats = virHashLookup(blockstats, NAME))) { \ virReportError(VIR_ERR_INTERNAL_ERROR, \ "block stats for device '%s' is missing", NAME); \ @@ -1560,7 +1568,8 @@ testQemuMonitorJSONqemuMonitorJSONGetBlockStatsInfo(const void *data) CHECK0(wr_bytes, WR_BYTES) \ CHECK0(wr_total_times, WR_TOTAL_TIMES) \ CHECK0(flush_req, FLUSH_REQ) \ - CHECK0(flush_total_times, FLUSH_TOTAL_TIMES) + CHECK0(flush_total_times, FLUSH_TOTAL_TIMES) \ + CHECK0ULL(wr_highest_offset, WR_HIGHEST_OFFSET)
if (qemuMonitorGetAllBlockStatsInfo(qemuMonitorTestGetMonitor(test), &blockstats, false) < 0) @@ -1572,9 +1581,9 @@ testQemuMonitorJSONqemuMonitorJSONGetBlockStatsInfo(const void *data) goto cleanup; }
- CHECK("virtio-disk0", 1279, 28505088, 640616474, 174, 2845696, 530699221, 0, 0) - CHECK("virtio-disk1", 85, 348160, 8232156, 0, 0, 0, 0, 0) - CHECK("ide0-1-0", 16, 49250, 1004952, 0, 0, 0, 0, 0) + CHECK("virtio-disk0", 1279, 28505088, 640616474, 174, 2845696, 530699221, 0, 0, 5256018944ULL) + CHECK("virtio-disk1", 85, 348160, 8232156, 0, 0, 0, 0, 0, 0ULL) + CHECK("ide0-1-0", 16, 49250, 1004952, 0, 0, 0, 0, 0, 0ULL)
This assumes that the wr_highest_offset was found, what about when it's not found in the data stream? Seems that would mean either modifying the CHECK macro to add another parameter (true/false) or some sort of magic in the CHECK0LL macro that does something like "stats->#var_valid" (I never get the exact syntax correct without trying a few times) [hence my comment earlier about modifying the .h file comments to have a field and field_valid bool.
if (qemuMonitorJSONGetBlockExtent(qemuMonitorTestGetMonitor(test), "virtio-disk0", &extent) < 0) @@ -1613,6 +1622,7 @@ testQemuMonitorJSONqemuMonitorJSONGetBlockStatsInfo(const void *data)
#undef CHECK #undef CHECK0 +#undef CHECK0ULL
cleanup: qemuMonitorTestFree(test); diff --git a/tests/qemumonitortest.c b/tests/qemumonitortest.c index 0125962..9d8d70a 100644 --- a/tests/qemumonitortest.c +++ b/tests/qemumonitortest.c @@ -94,11 +94,11 @@ struct blockInfoData { static const struct blockInfoData testBlockInfoData[] = { /* NAME, rd_req, rd_bytes, wr_req, wr_bytes, rd_total_time, wr_total_time, flush_req, flush_total_time */
So currently 8 of the 11 params inside the {} are documented the "0, 0, 0" in each wasn't... (eg, capacity, physical, wr_highest_offset) then adding "false" isn't either.
- {"vda", {11, 12, 13, 14, 15, 16, 17, 18, 0, 0, 0}}, - {"vdb", {21, 22, 23, 24, 25, 26, 27, 28, 0, 0, 0}}, - {"vdc", {31, 32, 33, -1, 35, 36, 37, 38, 0, 0, 0}}, - {"vdd", {-1, -1, -1, -1, -1, -1, -1, -1, 0, 0, 0}}, - {"vde", {41, 42, 43, 44, 45, 46, 47, 48, 0, 0, 0}} + {"vda", {11, 12, 13, 14, 15, 16, 17, 18, 0, 0, 0, false}}, + {"vdb", {21, 22, 23, 24, 25, 26, 27, 28, 0, 0, 0, false}}, + {"vdc", {31, 32, 33, -1, 35, 36, 37, 38, 0, 0, 0, false}}, + {"vdd", {-1, -1, -1, -1, -1, -1, -1, -1, 0, 0, 0, false}}, + {"vde", {41, 42, 43, 44, 45, 46, 47, 48, 0, 0, 0, false}}
Should there perhaps be an entry that tests "true" and places a non zero value in wr_highest_offset, plus the comparable change to testBlockInfoReply?
};
static const char testBlockInfoReply[] =
The code seems OK to me - seems the tests need to check for the wr_highest_offset being true. ACK with some test adjustments. John
On Thu, Jun 25, 2015 at 12:56:02 -0400, John Ferlan wrote:
On 06/23/2015 01:15 PM, Peter Krempa wrote:
Instead of using qemuMonitorJSONDevGetBlockExtent (which I plan to remove later) extract the data in place.
Additionally add a flag that will be set when the wr_highest_offset was extracted correctly so that callers can act according to that.
The test case addition should help make sure that everything works. --- src/qemu/qemu_monitor.h | 1 + src/qemu/qemu_monitor_json.c | 10 ++++++++-- tests/qemumonitorjsontest.c | 20 +++++++++++++++----- tests/qemumonitortest.c | 10 +++++----- 4 files changed, 29 insertions(+), 12 deletions(-)
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 1afc344..b4d6538 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -379,6 +379,7 @@ struct _qemuBlockStats { unsigned long long capacity; unsigned long long physical; unsigned long long wr_highest_offset; + bool wr_highest_offset_valid; };
Maybe some sort of comment that starting at wr_highest_offset, all values require both the data and a bool of the same name, but with _valid ... (see below - perhaps it'll make more sense)
Fair enough. The main reason for the variable to be present is to allow differentiating between wr_highest_off being present and 0 and it not being present.
int qemuMonitorGetAllBlockStatsInfo(qemuMonitorPtr mon, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 11c45a1..b2ce33f 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -1699,6 +1699,8 @@ qemuMonitorJSONGetOneBlockStatsInfo(virJSONValuePtr dev, { qemuBlockStatsPtr bstats = NULL; virJSONValuePtr stats; + virJSONValuePtr parent; + virJSONValuePtr parentstats; int ret = -1; int nstats = 0; char *entry_name = qemuDomainStorageAlias(dev_name, depth); @@ -1735,8 +1737,12 @@ qemuMonitorJSONGetOneBlockStatsInfo(virJSONValuePtr dev, QEMU_MONITOR_BLOCK_STAT_GET("flush_total_time_ns", bstats->flush_total_times, false); #undef QEMU_MONITOR_BLOCK_STAT_GET
- /* it's ok to not have this information here. Just skip silently. */ - qemuMonitorJSONDevGetBlockExtent(dev, &bstats->wr_highest_offset); + if ((parent = virJSONValueObjectGetObject(dev, "parent")) && + (parentstats = virJSONValueObjectGetObject(parent, "stats"))) { + if (virJSONValueObjectGetNumberUlong(parentstats, "wr_highest_offset", + &bstats->wr_highest_offset) == 0) + bstats->wr_highest_offset_valid = true; + }
if (virHashAddEntry(hash, entry_name, bstats) < 0) goto cleanup; diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index 0623275..6246737 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -1546,8 +1546,16 @@ testQemuMonitorJSONqemuMonitorJSONGetBlockStatsInfo(const void *data) goto cleanup; \ }
+#define CHECK0ULL(var, value) \ + if (stats->var != value) { \ + virReportError(VIR_ERR_INTERNAL_ERROR, \ + "Invalid " #var " value: %llu, expected %llu", \ + stats->var, value); \ + goto cleanup; \ + } + #define CHECK(NAME, RD_REQ, RD_BYTES, RD_TOTAL_TIMES, WR_REQ, WR_BYTES, \ - WR_TOTAL_TIMES, FLUSH_REQ, FLUSH_TOTAL_TIMES) \ + WR_TOTAL_TIMES, FLUSH_REQ, FLUSH_TOTAL_TIMES, WR_HIGHEST_OFFSET) \ if (!(stats = virHashLookup(blockstats, NAME))) { \ virReportError(VIR_ERR_INTERNAL_ERROR, \ "block stats for device '%s' is missing", NAME); \ @@ -1560,7 +1568,8 @@ testQemuMonitorJSONqemuMonitorJSONGetBlockStatsInfo(const void *data) CHECK0(wr_bytes, WR_BYTES) \ CHECK0(wr_total_times, WR_TOTAL_TIMES) \ CHECK0(flush_req, FLUSH_REQ) \ - CHECK0(flush_total_times, FLUSH_TOTAL_TIMES) + CHECK0(flush_total_times, FLUSH_TOTAL_TIMES) \ + CHECK0ULL(wr_highest_offset, WR_HIGHEST_OFFSET)
if (qemuMonitorGetAllBlockStatsInfo(qemuMonitorTestGetMonitor(test), &blockstats, false) < 0) @@ -1572,9 +1581,9 @@ testQemuMonitorJSONqemuMonitorJSONGetBlockStatsInfo(const void *data) goto cleanup; }
- CHECK("virtio-disk0", 1279, 28505088, 640616474, 174, 2845696, 530699221, 0, 0) - CHECK("virtio-disk1", 85, 348160, 8232156, 0, 0, 0, 0, 0) - CHECK("ide0-1-0", 16, 49250, 1004952, 0, 0, 0, 0, 0) + CHECK("virtio-disk0", 1279, 28505088, 640616474, 174, 2845696, 530699221, 0, 0, 5256018944ULL) + CHECK("virtio-disk1", 85, 348160, 8232156, 0, 0, 0, 0, 0, 0ULL) + CHECK("ide0-1-0", 16, 49250, 1004952, 0, 0, 0, 0, 0, 0ULL)
This assumes that the wr_highest_offset was found, what about when it's not found in the data stream?
Seems that would mean either modifying the CHECK macro to add another parameter (true/false) or some sort of magic in the CHECK0LL macro that does something like "stats->#var_valid" (I never get the exact syntax correct without trying a few times) [hence my comment earlier about modifying the .h file comments to have a field and field_valid bool.
I'll add a check that will check the _valid field too. The check will be possible after the last patch since that one expects the wr_highest_offset field to be present.
if (qemuMonitorJSONGetBlockExtent(qemuMonitorTestGetMonitor(test), "virtio-disk0", &extent) < 0) @@ -1613,6 +1622,7 @@ testQemuMonitorJSONqemuMonitorJSONGetBlockStatsInfo(const void *data)
#undef CHECK #undef CHECK0 +#undef CHECK0ULL
cleanup: qemuMonitorTestFree(test); diff --git a/tests/qemumonitortest.c b/tests/qemumonitortest.c index 0125962..9d8d70a 100644 --- a/tests/qemumonitortest.c +++ b/tests/qemumonitortest.c @@ -94,11 +94,11 @@ struct blockInfoData { static const struct blockInfoData testBlockInfoData[] = { /* NAME, rd_req, rd_bytes, wr_req, wr_bytes, rd_total_time, wr_total_time, flush_req, flush_total_time */
So currently 8 of the 11 params inside the {} are documented the "0, 0, 0" in each wasn't... (eg, capacity, physical, wr_highest_offset) then adding "false" isn't either.
Hmm, I'll fix that then since doing a separate patch for the test doesn't make sense.
- {"vda", {11, 12, 13, 14, 15, 16, 17, 18, 0, 0, 0}}, - {"vdb", {21, 22, 23, 24, 25, 26, 27, 28, 0, 0, 0}}, - {"vdc", {31, 32, 33, -1, 35, 36, 37, 38, 0, 0, 0}}, - {"vdd", {-1, -1, -1, -1, -1, -1, -1, -1, 0, 0, 0}}, - {"vde", {41, 42, 43, 44, 45, 46, 47, 48, 0, 0, 0}} + {"vda", {11, 12, 13, 14, 15, 16, 17, 18, 0, 0, 0, false}}, + {"vdb", {21, 22, 23, 24, 25, 26, 27, 28, 0, 0, 0, false}}, + {"vdc", {31, 32, 33, -1, 35, 36, 37, 38, 0, 0, 0, false}}, + {"vdd", {-1, -1, -1, -1, -1, -1, -1, -1, 0, 0, 0, false}}, + {"vde", {41, 42, 43, 44, 45, 46, 47, 48, 0, 0, 0, false}}
Should there perhaps be an entry that tests "true" and places a non zero value in wr_highest_offset, plus the comparable change to testBlockInfoReply?
Not really. HMP does not populate that variable. As you can see the IMPL above is done only for QMP.
};
static const char testBlockInfoReply[] =
The code seems OK to me - seems the tests need to check for the wr_highest_offset being true.
ACK with some test adjustments.
I'll include the patch that will do the requested check above in this series while pushing under this ACK.
John
Thanks. Peter
Change the code so that it queries the monitor when the VM is alive. --- src/qemu/qemu_driver.c | 90 +++++++++++++++++++++++++++++--------------------- 1 file changed, 53 insertions(+), 37 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 004da7e..3da941e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11800,10 +11800,12 @@ qemuDomainGetBlockInfo(virDomainPtr dom, virQEMUDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm; int ret = -1; - virDomainDiskDefPtr disk = NULL; - virStorageSourcePtr src; - bool activeFail = false; + virDomainDiskDefPtr disk; virQEMUDriverConfigPtr cfg = NULL; + int rc; + virHashTablePtr stats = NULL; + qemuBlockStats *entry; + char *alias; virCheckFlags(0, -1); @@ -11815,11 +11817,6 @@ qemuDomainGetBlockInfo(virDomainPtr dom, if (virDomainGetBlockInfoEnsureACL(dom->conn, vm->def) < 0) goto cleanup; - /* Technically, we only need a job if we are going to query the - * monitor, which is only for active domains that are using - * non-raw block devices. But it is easier to share code if we - * always grab a job; furthermore, grabbing the job ensures that - * hot-plug won't change disk behind our backs. */ if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) goto cleanup; @@ -11829,53 +11826,72 @@ qemuDomainGetBlockInfo(virDomainPtr dom, goto endjob; } - src = disk->src; - if (virStorageSourceIsEmpty(src)) { + if (virStorageSourceIsEmpty(disk->src)) { virReportError(VIR_ERR_INVALID_ARG, _("disk '%s' does not currently have a source assigned"), path); goto endjob; } - if ((ret = qemuStorageLimitsRefresh(driver, cfg, vm, src)) < 0) + /* for inactive domains we have to peek into the files */ + if (!virDomainObjIsActive(vm)) { + if ((qemuStorageLimitsRefresh(driver, cfg, vm, disk->src)) < 0) + goto endjob; + + info->capacity = disk->src->capacity; + info->allocation = disk->src->allocation; + info->physical = disk->src->physical; + + ret = 0; + goto endjob; + } + + if (!disk->info.alias || + !(alias = qemuDomainStorageAlias(disk->info.alias, 0))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("missing disk device alias name for %s"), disk->dst); goto endjob; + } - if (!src->allocation) { - qemuDomainObjPrivatePtr priv = vm->privateData; + qemuDomainObjEnterMonitor(driver, vm); + rc = qemuMonitorGetAllBlockStatsInfo(qemuDomainGetMonitor(vm), + &stats, false); + if (rc >= 0) + rc = qemuMonitorBlockStatsUpdateCapacity(qemuDomainGetMonitor(vm), + stats, false); - /* If the guest is not running, then success/failure return - * depends on whether domain is persistent - */ - if (!virDomainObjIsActive(vm)) { - activeFail = true; + if (qemuDomainObjExitMonitor(driver, vm) < 0 || rc < 0) + goto endjob; + + if (!(entry = virHashLookup(stats, alias))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("failed to gather stats for disk '%s'"), disk->dst); + goto endjob; + } + + if (!entry->wr_highest_offset_valid) { + if (virStorageSourceGetActualType(disk->src) == VIR_STORAGE_TYPE_BLOCK && + disk->src->format != VIR_STORAGE_FILE_RAW) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("failed to query the maximum written offset of " + "block device '%s'"), disk->dst); goto endjob; } - qemuDomainObjEnterMonitor(driver, vm); - ret = qemuMonitorGetBlockExtent(priv->mon, - disk->info.alias, - &src->allocation); - if (qemuDomainObjExitMonitor(driver, vm) < 0) - ret = -1; + info->allocation = entry->physical; + } else { + info->allocation = entry->wr_highest_offset; } - if (ret == 0) { - info->capacity = src->capacity; - info->allocation = src->allocation; - info->physical = src->physical; - } + info->capacity = entry->capacity; + info->physical = entry->physical; + + ret = 0; endjob: qemuDomainObjEndJob(driver, vm); cleanup: - /* If we failed to get data from a domain because it's inactive and - * it's not a persistent domain, then force failure. - */ - if (activeFail && vm && !vm->persistent) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("domain is not running")); - ret = -1; - } + virHashFree(stats); virDomainObjEndAPI(&vm); virObjectUnref(cfg); return ret; -- 2.4.1
On 06/23/2015 01:15 PM, Peter Krempa wrote:
Change the code so that it queries the monitor when the VM is alive. --- src/qemu/qemu_driver.c | 90 +++++++++++++++++++++++++++++--------------------- 1 file changed, 53 insertions(+), 37 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 004da7e..3da941e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11800,10 +11800,12 @@ qemuDomainGetBlockInfo(virDomainPtr dom, virQEMUDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm; int ret = -1; - virDomainDiskDefPtr disk = NULL; - virStorageSourcePtr src; - bool activeFail = false; + virDomainDiskDefPtr disk; virQEMUDriverConfigPtr cfg = NULL; + int rc; + virHashTablePtr stats = NULL; + qemuBlockStats *entry; + char *alias;
virCheckFlags(0, -1);
@@ -11815,11 +11817,6 @@ qemuDomainGetBlockInfo(virDomainPtr dom, if (virDomainGetBlockInfoEnsureACL(dom->conn, vm->def) < 0) goto cleanup;
- /* Technically, we only need a job if we are going to query the - * monitor, which is only for active domains that are using - * non-raw block devices. But it is easier to share code if we - * always grab a job; furthermore, grabbing the job ensures that - * hot-plug won't change disk behind our backs. */
Was the comment wrong? I tend to like comments like this, since it gives me an understanding why something was done a particular way... I'm not going to stop a change for removing the comment though... ACK John
if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) goto cleanup;
@@ -11829,53 +11826,72 @@ qemuDomainGetBlockInfo(virDomainPtr dom, goto endjob; }
- src = disk->src; - if (virStorageSourceIsEmpty(src)) { + if (virStorageSourceIsEmpty(disk->src)) { virReportError(VIR_ERR_INVALID_ARG, _("disk '%s' does not currently have a source assigned"), path); goto endjob; }
- if ((ret = qemuStorageLimitsRefresh(driver, cfg, vm, src)) < 0) + /* for inactive domains we have to peek into the files */ + if (!virDomainObjIsActive(vm)) { + if ((qemuStorageLimitsRefresh(driver, cfg, vm, disk->src)) < 0) + goto endjob; + + info->capacity = disk->src->capacity; + info->allocation = disk->src->allocation; + info->physical = disk->src->physical; + + ret = 0; + goto endjob; + } + + if (!disk->info.alias || + !(alias = qemuDomainStorageAlias(disk->info.alias, 0))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("missing disk device alias name for %s"), disk->dst); goto endjob; + }
- if (!src->allocation) { - qemuDomainObjPrivatePtr priv = vm->privateData; + qemuDomainObjEnterMonitor(driver, vm); + rc = qemuMonitorGetAllBlockStatsInfo(qemuDomainGetMonitor(vm), + &stats, false); + if (rc >= 0) + rc = qemuMonitorBlockStatsUpdateCapacity(qemuDomainGetMonitor(vm), + stats, false);
- /* If the guest is not running, then success/failure return - * depends on whether domain is persistent - */ - if (!virDomainObjIsActive(vm)) { - activeFail = true; + if (qemuDomainObjExitMonitor(driver, vm) < 0 || rc < 0) + goto endjob; + + if (!(entry = virHashLookup(stats, alias))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("failed to gather stats for disk '%s'"), disk->dst); + goto endjob; + } + + if (!entry->wr_highest_offset_valid) { + if (virStorageSourceGetActualType(disk->src) == VIR_STORAGE_TYPE_BLOCK && + disk->src->format != VIR_STORAGE_FILE_RAW) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("failed to query the maximum written offset of " + "block device '%s'"), disk->dst); goto endjob; }
- qemuDomainObjEnterMonitor(driver, vm); - ret = qemuMonitorGetBlockExtent(priv->mon, - disk->info.alias, - &src->allocation); - if (qemuDomainObjExitMonitor(driver, vm) < 0) - ret = -1; + info->allocation = entry->physical; + } else { + info->allocation = entry->wr_highest_offset; }
- if (ret == 0) { - info->capacity = src->capacity; - info->allocation = src->allocation; - info->physical = src->physical; - } + info->capacity = entry->capacity; + info->physical = entry->physical; + + ret = 0;
endjob: qemuDomainObjEndJob(driver, vm); cleanup: - /* If we failed to get data from a domain because it's inactive and - * it's not a persistent domain, then force failure. - */ - if (activeFail && vm && !vm->persistent) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("domain is not running")); - ret = -1; - } + virHashFree(stats); virDomainObjEndAPI(&vm); virObjectUnref(cfg); return ret;
On Thu, Jun 25, 2015 at 13:26:35 -0400, John Ferlan wrote:
On 06/23/2015 01:15 PM, Peter Krempa wrote:
Change the code so that it queries the monitor when the VM is alive. --- src/qemu/qemu_driver.c | 90 +++++++++++++++++++++++++++++--------------------- 1 file changed, 53 insertions(+), 37 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 004da7e..3da941e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11800,10 +11800,12 @@ qemuDomainGetBlockInfo(virDomainPtr dom, virQEMUDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm; int ret = -1; - virDomainDiskDefPtr disk = NULL; - virStorageSourcePtr src; - bool activeFail = false; + virDomainDiskDefPtr disk; virQEMUDriverConfigPtr cfg = NULL; + int rc; + virHashTablePtr stats = NULL; + qemuBlockStats *entry; + char *alias;
virCheckFlags(0, -1);
@@ -11815,11 +11817,6 @@ qemuDomainGetBlockInfo(virDomainPtr dom, if (virDomainGetBlockInfoEnsureACL(dom->conn, vm->def) < 0) goto cleanup;
- /* Technically, we only need a job if we are going to query the - * monitor, which is only for active domains that are using - * non-raw block devices. But it is easier to share code if we - * always grab a job; furthermore, grabbing the job ensures that - * hot-plug won't change disk behind our backs. */
Was the comment wrong? I tend to like comments like this, since it gives me an understanding why something was done a particular way...
qemuStorageLimitsRefresh modifies few fields of the disk->src structure. They are only integer modifications of capacity and they are guarded by the vm lock but that still should be covered by a job semantically. There are only very few exceptions to that rule. Additionally, grabbing the job helps to prevent the situation where we'd check that the VM is alive (where we do need a job to query the monitor), then we'd enter the job, but the vm would die at that precise moment and the rollback path from that situation would be convoluted. Peter
Now that qemuMonitorGetAllBlockStatsInfo collects also wr_highest_offset the whole function can be killed. --- src/qemu/qemu_monitor.c | 16 ----- src/qemu/qemu_monitor.h | 3 - src/qemu/qemu_monitor_json.c | 138 ------------------------------------------- src/qemu/qemu_monitor_json.h | 3 - src/qemu/qemu_monitor_text.c | 10 ---- src/qemu/qemu_monitor_text.h | 3 - tests/qemumonitorjsontest.c | 34 ----------- 7 files changed, 207 deletions(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 904d682..93fcc7f 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -1829,22 +1829,6 @@ qemuMonitorBlockStatsUpdateCapacity(qemuMonitorPtr mon, int -qemuMonitorGetBlockExtent(qemuMonitorPtr mon, - const char *dev_name, - unsigned long long *extent) -{ - VIR_DEBUG("dev_name=%s", dev_name); - - QEMU_CHECK_MONITOR(mon); - - if (mon->json) - return qemuMonitorJSONGetBlockExtent(mon, dev_name, extent); - else - return qemuMonitorTextGetBlockExtent(mon, dev_name, extent); -} - - -int qemuMonitorBlockResize(qemuMonitorPtr mon, const char *device, unsigned long long size) diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index b4d6538..483496c 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -392,9 +392,6 @@ int qemuMonitorBlockStatsUpdateCapacity(qemuMonitorPtr mon, bool backingChain) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); -int qemuMonitorGetBlockExtent(qemuMonitorPtr mon, - const char *dev_name, - unsigned long long *extent); int qemuMonitorBlockResize(qemuMonitorPtr mon, const char *dev_name, unsigned long long size); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index b2ce33f..d496a32 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -1660,36 +1660,6 @@ int qemuMonitorJSONGetBlockInfo(qemuMonitorPtr mon, } -typedef enum { - QEMU_MONITOR_BLOCK_EXTENT_ERROR_OK, - QEMU_MONITOR_BLOCK_EXTENT_ERROR_NOPARENT, - QEMU_MONITOR_BLOCK_EXTENT_ERROR_NOSTATS, - QEMU_MONITOR_BLOCK_EXTENT_ERROR_NOOFFSET, -} qemuMonitorBlockExtentError; - - -static int -qemuMonitorJSONDevGetBlockExtent(virJSONValuePtr dev, - unsigned long long *extent) -{ - virJSONValuePtr stats; - virJSONValuePtr parent; - - if ((parent = virJSONValueObjectGetObject(dev, "parent")) == NULL) - return QEMU_MONITOR_BLOCK_EXTENT_ERROR_NOPARENT; - - if ((stats = virJSONValueObjectGetObject(parent, "stats")) == NULL) - return QEMU_MONITOR_BLOCK_EXTENT_ERROR_NOSTATS; - - if (virJSONValueObjectGetNumberUlong(stats, "wr_highest_offset", - extent) < 0) { - return QEMU_MONITOR_BLOCK_EXTENT_ERROR_NOOFFSET; - } - - return QEMU_MONITOR_BLOCK_EXTENT_ERROR_OK; -} - - static int qemuMonitorJSONGetOneBlockStatsInfo(virJSONValuePtr dev, const char *dev_name, @@ -1943,114 +1913,6 @@ qemuMonitorJSONBlockStatsUpdateCapacity(qemuMonitorPtr mon, } -static int -qemuMonitorJSONReportBlockExtentError(qemuMonitorBlockExtentError error) -{ - switch (error) { - case QEMU_MONITOR_BLOCK_EXTENT_ERROR_NOPARENT: - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("blockstats parent entry was not in " - "expected format")); - break; - - case QEMU_MONITOR_BLOCK_EXTENT_ERROR_NOSTATS: - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("blockstats stats entry was not in " - "expected format")); - break; - - case QEMU_MONITOR_BLOCK_EXTENT_ERROR_NOOFFSET: - virReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot read %s statistic"), - "wr_highest_offset"); - break; - - case QEMU_MONITOR_BLOCK_EXTENT_ERROR_OK: - return 0; - } - - return -1; -} - - -int qemuMonitorJSONGetBlockExtent(qemuMonitorPtr mon, - const char *dev_name, - unsigned long long *extent) -{ - int ret = -1; - size_t i; - bool found = false; - virJSONValuePtr cmd = qemuMonitorJSONMakeCommand("query-blockstats", - NULL); - virJSONValuePtr reply = NULL; - virJSONValuePtr devices; - - *extent = 0; - - if (!cmd) - return -1; - - ret = qemuMonitorJSONCommand(mon, cmd, &reply); - - if (ret == 0) - ret = qemuMonitorJSONCheckError(cmd, reply); - if (ret < 0) - goto cleanup; - ret = -1; - - if (!(devices = virJSONValueObjectGetArray(reply, "return"))) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("blockstats reply was missing device list")); - goto cleanup; - } - - for (i = 0; i < virJSONValueArraySize(devices); i++) { - virJSONValuePtr dev = virJSONValueArrayGet(devices, i); - const char *thisdev; - int err; - if (!dev || dev->type != VIR_JSON_TYPE_OBJECT) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("blockstats device entry was not in expected format")); - goto cleanup; - } - - if ((thisdev = virJSONValueObjectGetString(dev, "device")) == NULL) { - 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; - - found = true; - if ((err = qemuMonitorJSONDevGetBlockExtent(dev, extent)) != - QEMU_MONITOR_BLOCK_EXTENT_ERROR_OK) { - qemuMonitorJSONReportBlockExtentError(err); - goto cleanup; - } - } - - if (!found) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot find statistics for device '%s'"), dev_name); - goto cleanup; - } - ret = 0; - - cleanup: - virJSONValueFree(cmd); - virJSONValueFree(reply); - return ret; -} - /* Return 0 on success, -1 on failure, or -2 if not supported. Size * is in bytes. */ int qemuMonitorJSONBlockResize(qemuMonitorPtr mon, diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index c0ee4ce..b76d85b 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -77,9 +77,6 @@ int qemuMonitorJSONGetAllBlockStatsInfo(qemuMonitorPtr mon, int qemuMonitorJSONBlockStatsUpdateCapacity(qemuMonitorPtr mon, virHashTablePtr stats, bool backingChain); -int qemuMonitorJSONGetBlockExtent(qemuMonitorPtr mon, - const char *dev_name, - unsigned long long *extent); int qemuMonitorJSONBlockResize(qemuMonitorPtr mon, const char *devce, unsigned long long size); diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index 1b6bc02..2e77534 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -968,16 +968,6 @@ qemuMonitorTextGetAllBlockStatsInfo(qemuMonitorPtr mon, return ret; } - -int qemuMonitorTextGetBlockExtent(qemuMonitorPtr mon ATTRIBUTE_UNUSED, - const char *dev_name ATTRIBUTE_UNUSED, - unsigned long long *extent ATTRIBUTE_UNUSED) -{ - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("unable to query block extent with this QEMU")); - return -1; -} - /* Return 0 on success, -1 on failure, or -2 if not supported. Size * is in bytes. */ int qemuMonitorTextBlockResize(qemuMonitorPtr mon, diff --git a/src/qemu/qemu_monitor_text.h b/src/qemu/qemu_monitor_text.h index 6f9a215..3fa603b 100644 --- a/src/qemu/qemu_monitor_text.h +++ b/src/qemu/qemu_monitor_text.h @@ -63,9 +63,6 @@ int qemuMonitorTextGetBlockInfo(qemuMonitorPtr mon, int qemuMonitorTextGetAllBlockStatsInfo(qemuMonitorPtr mon, virHashTablePtr hash); -int qemuMonitorTextGetBlockExtent(qemuMonitorPtr mon, - const char *dev_name, - unsigned long long *extent); int qemuMonitorTextBlockResize(qemuMonitorPtr mon, const char *device, unsigned long long size); diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index 6246737..6df91b3 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -1438,7 +1438,6 @@ testQemuMonitorJSONqemuMonitorJSONGetBlockStatsInfo(const void *data) virHashTablePtr blockstats = NULL; qemuBlockStatsPtr stats; int ret = -1; - unsigned long long extent; const char *reply = "{" @@ -1585,39 +1584,6 @@ testQemuMonitorJSONqemuMonitorJSONGetBlockStatsInfo(const void *data) CHECK("virtio-disk1", 85, 348160, 8232156, 0, 0, 0, 0, 0, 0ULL) CHECK("ide0-1-0", 16, 49250, 1004952, 0, 0, 0, 0, 0, 0ULL) - if (qemuMonitorJSONGetBlockExtent(qemuMonitorTestGetMonitor(test), "virtio-disk0", - &extent) < 0) - goto cleanup; - - if (extent != 5256018944ULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "Invalid extent: %llu, expected 5256018944", - extent); - goto cleanup; - } - - if (qemuMonitorJSONGetBlockExtent(qemuMonitorTestGetMonitor(test), "virtio-disk1", - &extent) < 0) - goto cleanup; - - if (extent != 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "Invalid extent: %llu, expected 0", - extent); - goto cleanup; - } - - if (qemuMonitorJSONGetBlockExtent(qemuMonitorTestGetMonitor(test), "ide0-1-0", - &extent) < 0) - goto cleanup; - - if (extent != 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "Invalid extent: %llu, expected 0", - extent); - goto cleanup; - } - ret = 0; #undef CHECK -- 2.4.1
On 06/23/2015 01:15 PM, Peter Krempa wrote:
Now that qemuMonitorGetAllBlockStatsInfo collects also wr_highest_offset the whole function can be killed. --- src/qemu/qemu_monitor.c | 16 ----- src/qemu/qemu_monitor.h | 3 - src/qemu/qemu_monitor_json.c | 138 ------------------------------------------- src/qemu/qemu_monitor_json.h | 3 - src/qemu/qemu_monitor_text.c | 10 ---- src/qemu/qemu_monitor_text.h | 3 - tests/qemumonitorjsontest.c | 34 ----------- 7 files changed, 207 deletions(-)
ACK John
participants (2)
-
John Ferlan -
Peter Krempa