[libvirt] [PATCH 00/12] qemu: Refactor block stats gathering and implement summary stats
Peter Krempa (12): qemu: Use macro to set block stats typed parameters qemu: monitor: Drop parsing of 'errs' from block info qemu: blockstats: Switch to caller allocated hash table test: qemu: Fix qemu monitor test utils to allow testing HMP qemu: monitor: Implement HMP version for listing all block device stats qemu: monitor: Convert common code to a macro qemu: monitor: Count block stats fields in qemuMonitorGetAllBlockStatsInfo qemu: Split out working code qemuDomainBlockStats qemu: blockstats: Add support for totalled block statistics qemu: blockstats: Refactor qemuDomainBlockStatsFlags test: qemu: json: Avoid using the now obsolete functions qemu: monitor: Kill qemuMonitorGetBlockStats(Info,ParamsNumber) src/qemu/qemu_driver.c | 299 ++++++++++++++++++++----------------------- src/qemu/qemu_monitor.c | 106 ++++++--------- src/qemu/qemu_monitor.h | 15 --- src/qemu/qemu_monitor_json.c | 233 ++++----------------------------- src/qemu/qemu_monitor_json.h | 15 +-- src/qemu/qemu_monitor_text.c | 256 ++++++++++++++---------------------- src/qemu/qemu_monitor_text.h | 16 +-- tests/Makefile.am | 8 +- tests/qemumonitorjsontest.c | 65 ++++------ tests/qemumonitortest.c | 89 ++++++++++++- tests/qemumonitortestutils.c | 3 +- tools/virsh.pod | 5 +- 12 files changed, 425 insertions(+), 685 deletions(-) -- 2.2.2
All the setters are the same code except for parameter name and variable, so they can be converted to a macro to save a ton of duplicated code. --- src/qemu/qemu_driver.c | 78 +++++++++++--------------------------------------- 1 file changed, 16 insertions(+), 62 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ceceafa..e94275f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10575,7 +10575,6 @@ qemuDomainBlockStatsFlags(virDomainPtr dom, qemuDomainObjPrivatePtr priv; long long rd_req, rd_bytes, wr_req, wr_bytes, rd_total_times; long long wr_total_times, flush_req, flush_total_times, errs; - virTypedParameterPtr param; char *diskAlias = NULL; virCheckFlags(VIR_TYPED_PARAM_STRING_OKAY, -1); @@ -10656,73 +10655,28 @@ qemuDomainBlockStatsFlags(virDomainPtr dom, tmp = 0; ret = -1; - if (tmp < *nparams && wr_bytes != -1) { - param = ¶ms[tmp]; - if (virTypedParameterAssign(param, VIR_DOMAIN_BLOCK_STATS_WRITE_BYTES, - VIR_TYPED_PARAM_LLONG, wr_bytes) < 0) - goto endjob; - tmp++; +/* the following macro shall not be used with side effects statements */ +#define QEMU_BLOCK_STAT(VAR, NAME) \ + if (tmp < *nparams && (VAR) != -1) { \ + if (virTypedParameterAssign(params + tmp, NAME, VIR_TYPED_PARAM_LLONG,\ + (VAR)) < 0) \ + goto endjob; \ + tmp++; \ } - if (tmp < *nparams && wr_req != -1) { - param = ¶ms[tmp]; - if (virTypedParameterAssign(param, VIR_DOMAIN_BLOCK_STATS_WRITE_REQ, - VIR_TYPED_PARAM_LLONG, wr_req) < 0) - goto endjob; - tmp++; - } + QEMU_BLOCK_STAT(wr_bytes, VIR_DOMAIN_BLOCK_STATS_WRITE_BYTES); + QEMU_BLOCK_STAT(wr_req, VIR_DOMAIN_BLOCK_STATS_WRITE_REQ); - if (tmp < *nparams && rd_bytes != -1) { - param = ¶ms[tmp]; - if (virTypedParameterAssign(param, VIR_DOMAIN_BLOCK_STATS_READ_BYTES, - VIR_TYPED_PARAM_LLONG, rd_bytes) < 0) - goto endjob; - tmp++; - } + QEMU_BLOCK_STAT(rd_bytes, VIR_DOMAIN_BLOCK_STATS_READ_BYTES); + QEMU_BLOCK_STAT(rd_req, VIR_DOMAIN_BLOCK_STATS_READ_REQ); - if (tmp < *nparams && rd_req != -1) { - param = ¶ms[tmp]; - if (virTypedParameterAssign(param, VIR_DOMAIN_BLOCK_STATS_READ_REQ, - VIR_TYPED_PARAM_LLONG, rd_req) < 0) - goto endjob; - tmp++; - } + QEMU_BLOCK_STAT(flush_req, VIR_DOMAIN_BLOCK_STATS_FLUSH_REQ); - if (tmp < *nparams && flush_req != -1) { - param = ¶ms[tmp]; - if (virTypedParameterAssign(param, VIR_DOMAIN_BLOCK_STATS_FLUSH_REQ, - VIR_TYPED_PARAM_LLONG, flush_req) < 0) - goto endjob; - tmp++; - } + QEMU_BLOCK_STAT(wr_total_times, VIR_DOMAIN_BLOCK_STATS_WRITE_TOTAL_TIMES); + QEMU_BLOCK_STAT(rd_total_times, VIR_DOMAIN_BLOCK_STATS_READ_TOTAL_TIMES); + QEMU_BLOCK_STAT(flush_total_times, VIR_DOMAIN_BLOCK_STATS_FLUSH_TOTAL_TIMES); - if (tmp < *nparams && wr_total_times != -1) { - param = ¶ms[tmp]; - if (virTypedParameterAssign(param, - VIR_DOMAIN_BLOCK_STATS_WRITE_TOTAL_TIMES, - VIR_TYPED_PARAM_LLONG, wr_total_times) < 0) - goto endjob; - tmp++; - } - - if (tmp < *nparams && rd_total_times != -1) { - param = ¶ms[tmp]; - if (virTypedParameterAssign(param, - VIR_DOMAIN_BLOCK_STATS_READ_TOTAL_TIMES, - VIR_TYPED_PARAM_LLONG, rd_total_times) < 0) - goto endjob; - tmp++; - } - - if (tmp < *nparams && flush_total_times != -1) { - param = ¶ms[tmp]; - if (virTypedParameterAssign(param, - VIR_DOMAIN_BLOCK_STATS_FLUSH_TOTAL_TIMES, - VIR_TYPED_PARAM_LLONG, - flush_total_times) < 0) - goto endjob; - tmp++; - } +#undef QEMU_BLOCK_STAT /* Field 'errs' is meaningless for QEMU, won't set it. */ -- 2.2.2
On Tue, Mar 10, 2015 at 05:26:30PM +0100, Peter Krempa wrote:
All the setters are the same code except for parameter name and variable, so they can be converted to a macro to save a ton of duplicated code.
You saved 46 lines, which amounts to about 21.7 kg per line. Have you thought about using a smaller font?
--- src/qemu/qemu_driver.c | 78 +++++++++++--------------------------------------- 1 file changed, 16 insertions(+), 62 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ceceafa..e94275f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10575,7 +10575,6 @@ qemuDomainBlockStatsFlags(virDomainPtr dom, qemuDomainObjPrivatePtr priv; long long rd_req, rd_bytes, wr_req, wr_bytes, rd_total_times; long long wr_total_times, flush_req, flush_total_times, errs; - virTypedParameterPtr param; char *diskAlias = NULL;
virCheckFlags(VIR_TYPED_PARAM_STRING_OKAY, -1); @@ -10656,73 +10655,28 @@ qemuDomainBlockStatsFlags(virDomainPtr dom, tmp = 0; ret = -1;
- if (tmp < *nparams && wr_bytes != -1) { - param = ¶ms[tmp]; - if (virTypedParameterAssign(param, VIR_DOMAIN_BLOCK_STATS_WRITE_BYTES, - VIR_TYPED_PARAM_LLONG, wr_bytes) < 0) - goto endjob; - tmp++;
+/* the following macro shall not be used with side effects statements */
I don't think this comment is necessary: 1) the macro is only used locally, preferably on the same screen as it's defined 2) it also has the side effect of incrementing tmp
+#define QEMU_BLOCK_STAT(VAR, NAME) \
The macro would be easier to read as QEMU_ASSIGN_BLOCK_STAT, or even QEMU_ASSIGN_LLONG or QEMU_ASSIGN_PARAM since nothing in the macro code is block-stat-specific. ACK if you add a verb to the macro name. Jan
The error count statistic is not supported by qemu, so there's no need to pass the variables around if the result is ignored anyways. --- src/qemu/qemu_driver.c | 11 ++++++----- src/qemu/qemu_monitor.c | 9 +++------ src/qemu/qemu_monitor.h | 3 +-- src/qemu/qemu_monitor_json.c | 6 ++---- src/qemu/qemu_monitor_json.h | 3 +-- src/qemu/qemu_monitor_text.c | 5 ++--- src/qemu/qemu_monitor_text.h | 3 +-- tests/qemumonitorjsontest.c | 19 +++++++++---------- 8 files changed, 25 insertions(+), 34 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e94275f..06168b1 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10536,6 +10536,9 @@ qemuDomainBlockStats(virDomainPtr dom, priv = vm->privateData; + /* qemu doesn't report the error count */ + stats->errs = -1; + qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorGetBlockStatsInfo(priv->mon, diskAlias, @@ -10546,8 +10549,7 @@ qemuDomainBlockStats(virDomainPtr dom, &stats->wr_bytes, NULL, NULL, - NULL, - &stats->errs); + NULL); if (qemuDomainObjExitMonitor(driver, vm) < 0) ret = -1; @@ -10574,7 +10576,7 @@ qemuDomainBlockStatsFlags(virDomainPtr dom, virDomainObjPtr vm; qemuDomainObjPrivatePtr priv; long long rd_req, rd_bytes, wr_req, wr_bytes, rd_total_times; - long long wr_total_times, flush_req, flush_total_times, errs; + long long wr_total_times, flush_req, flush_total_times; char *diskAlias = NULL; virCheckFlags(VIR_TYPED_PARAM_STRING_OKAY, -1); @@ -10643,8 +10645,7 @@ qemuDomainBlockStatsFlags(virDomainPtr dom, &wr_bytes, &wr_total_times, &flush_req, - &flush_total_times, - &errs); + &flush_total_times); if (qemuDomainObjExitMonitor(driver, vm) < 0) ret = -1; diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 94495cd..24e87b7 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -1817,8 +1817,7 @@ int qemuMonitorGetBlockStatsInfo(qemuMonitorPtr mon, long long *wr_bytes, long long *wr_total_times, long long *flush_req, - long long *flush_total_times, - long long *errs) + long long *flush_total_times) { int ret; VIR_DEBUG("mon=%p dev=%s", mon, dev_name); @@ -1836,8 +1835,7 @@ int qemuMonitorGetBlockStatsInfo(qemuMonitorPtr mon, wr_req, wr_bytes, wr_total_times, flush_req, - flush_total_times, - errs); + flush_total_times); else ret = qemuMonitorTextGetBlockStatsInfo(mon, dev_name, rd_req, rd_bytes, @@ -1845,8 +1843,7 @@ int qemuMonitorGetBlockStatsInfo(qemuMonitorPtr mon, wr_req, wr_bytes, wr_total_times, flush_req, - flush_total_times, - errs); + flush_total_times); return ret; } diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 133d42d..72498b3 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -366,8 +366,7 @@ int qemuMonitorGetBlockStatsInfo(qemuMonitorPtr mon, long long *wr_bytes, long long *wr_total_times, long long *flush_req, - long long *flush_total_times, - long long *errs); + long long *flush_total_times); typedef struct _qemuBlockStats qemuBlockStats; typedef qemuBlockStats *qemuBlockStatsPtr; diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 832f589..612553b 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -1677,15 +1677,14 @@ int qemuMonitorJSONGetBlockStatsInfo(qemuMonitorPtr mon, long long *wr_bytes, long long *wr_total_times, long long *flush_req, - long long *flush_total_times, - long long *errs) + long long *flush_total_times) { qemuBlockStats *stats; virHashTablePtr blockstats = NULL; int ret = -1; *rd_req = *rd_bytes = -1; - *wr_req = *wr_bytes = *errs = -1; + *wr_req = *wr_bytes = -1; if (rd_total_times) *rd_total_times = -1; @@ -1709,7 +1708,6 @@ int qemuMonitorJSONGetBlockStatsInfo(qemuMonitorPtr mon, *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; diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 1da1a00..23589cf 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -80,8 +80,7 @@ int qemuMonitorJSONGetBlockStatsInfo(qemuMonitorPtr mon, long long *wr_bytes, long long *wr_total_times, long long *flush_req, - long long *flush_total_times, - long long *errs); + long long *flush_total_times); int qemuMonitorJSONGetAllBlockStatsInfo(qemuMonitorPtr mon, virHashTablePtr *ret_stats, bool backingChain); diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index 70aeaca..2de281f 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -847,8 +847,7 @@ int qemuMonitorTextGetBlockStatsInfo(qemuMonitorPtr mon, long long *wr_bytes, long long *wr_total_times, long long *flush_req, - long long *flush_total_times, - long long *errs) + long long *flush_total_times) { char *info = NULL; int ret = -1; @@ -872,7 +871,7 @@ int qemuMonitorTextGetBlockStatsInfo(qemuMonitorPtr mon, } *rd_req = *rd_bytes = -1; - *wr_req = *wr_bytes = *errs = -1; + *wr_req = *wr_bytes = -1; if (rd_total_times) *rd_total_times = -1; diff --git a/src/qemu/qemu_monitor_text.h b/src/qemu/qemu_monitor_text.h index f118a30..695ac28 100644 --- a/src/qemu/qemu_monitor_text.h +++ b/src/qemu/qemu_monitor_text.h @@ -69,8 +69,7 @@ int qemuMonitorTextGetBlockStatsInfo(qemuMonitorPtr mon, long long *wr_bytes, long long *wr_total_times, long long *flush_req, - long long *flush_total_times, - long long *errs); + long long *flush_total_times); int qemuMonitorTextGetBlockStatsParamsNumber(qemuMonitorPtr mon, int *nparams); int qemuMonitorTextGetBlockExtent(qemuMonitorPtr mon, diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index bd92e63..da9cd6c 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -1438,7 +1438,7 @@ testQemuMonitorJSONqemuMonitorJSONGetBlockStatsInfo(const void *data) int ret = -1; long long rd_req, rd_bytes, rd_total_times; long long wr_req, wr_bytes, wr_total_times; - long long flush_req, flush_total_times, errs; + long long flush_req, flush_total_times; int nparams; unsigned long long extent; @@ -1552,7 +1552,7 @@ testQemuMonitorJSONqemuMonitorJSONGetBlockStatsInfo(const void *data) } #define CHECK(RD_REQ, RD_BYTES, RD_TOTAL_TIMES, WR_REQ, WR_BYTES, WR_TOTAL_TIMES, \ - FLUSH_REQ, FLUSH_TOTAL_TIMES, ERRS) \ + FLUSH_REQ, FLUSH_TOTAL_TIMES) \ CHECK0(rd_req, RD_REQ) \ CHECK0(rd_bytes, RD_BYTES) \ CHECK0(rd_total_times, RD_TOTAL_TIMES) \ @@ -1560,32 +1560,31 @@ 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(errs, ERRS) + CHECK0(flush_total_times, FLUSH_TOTAL_TIMES) if (qemuMonitorJSONGetBlockStatsInfo(qemuMonitorTestGetMonitor(test), "virtio-disk0", &rd_req, &rd_bytes, &rd_total_times, &wr_req, &wr_bytes, &wr_total_times, - &flush_req, &flush_total_times, &errs) < 0) + &flush_req, &flush_total_times) < 0) goto cleanup; - CHECK(1279, 28505088, 640616474, 174, 2845696, 530699221, 0, 0, -1) + CHECK(1279, 28505088, 640616474, 174, 2845696, 530699221, 0, 0) if (qemuMonitorJSONGetBlockStatsInfo(qemuMonitorTestGetMonitor(test), "virtio-disk1", &rd_req, &rd_bytes, &rd_total_times, &wr_req, &wr_bytes, &wr_total_times, - &flush_req, &flush_total_times, &errs) < 0) + &flush_req, &flush_total_times) < 0) goto cleanup; - CHECK(85, 348160, 8232156, 0, 0, 0, 0, 0, -1) + CHECK(85, 348160, 8232156, 0, 0, 0, 0, 0) if (qemuMonitorJSONGetBlockStatsInfo(qemuMonitorTestGetMonitor(test), "ide0-1-0", &rd_req, &rd_bytes, &rd_total_times, &wr_req, &wr_bytes, &wr_total_times, - &flush_req, &flush_total_times, &errs) < 0) + &flush_req, &flush_total_times) < 0) goto cleanup; - CHECK(16, 49250, 1004952, 0, 0, 0, 0, 0, -1) + CHECK(16, 49250, 1004952, 0, 0, 0, 0, 0) if (qemuMonitorJSONGetBlockStatsParamsNumber(qemuMonitorTestGetMonitor(test), &nparams) < 0) -- 2.2.2
Allocate the hash table in the monitor wrapper function instead of the worker itself so that the text monitor impl that will be added in the next patch doesn't have to duplicate it. --- src/qemu/qemu_monitor.c | 13 ++++++++++++- src/qemu/qemu_monitor_json.c | 14 +++++--------- src/qemu/qemu_monitor_json.h | 2 +- 3 files changed, 18 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 24e87b7..95a6989 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -1864,7 +1864,18 @@ qemuMonitorGetAllBlockStatsInfo(qemuMonitorPtr mon, return -1; } - return qemuMonitorJSONGetAllBlockStatsInfo(mon, ret_stats, backingChain); + if (!(*ret_stats = virHashCreate(10, virHashValueFree))) + goto error; + + if (qemuMonitorJSONGetAllBlockStatsInfo(mon, *ret_stats, backingChain) < 0) + goto error; + + return 0; + + error: + virHashFree(*ret_stats); + *ret_stats = NULL; + return -1; } diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 612553b..c88c7c3 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -1695,7 +1695,10 @@ int qemuMonitorJSONGetBlockStatsInfo(qemuMonitorPtr mon, if (flush_total_times) *flush_total_times = -1; - if (qemuMonitorJSONGetAllBlockStatsInfo(mon, &blockstats, false) < 0) + if (!(blockstats = virHashCreate(10, virHashValueFree))) + goto cleanup; + + if (qemuMonitorJSONGetAllBlockStatsInfo(mon, blockstats, false) < 0) goto cleanup; if (!(stats = virHashLookup(blockstats, dev_name))) { @@ -1870,7 +1873,7 @@ qemuMonitorJSONGetOneBlockStatsInfo(virJSONValuePtr dev, int qemuMonitorJSONGetAllBlockStatsInfo(qemuMonitorPtr mon, - virHashTablePtr *ret_stats, + virHashTablePtr hash, bool backingChain) { int ret = -1; @@ -1879,14 +1882,10 @@ qemuMonitorJSONGetAllBlockStatsInfo(qemuMonitorPtr mon, virJSONValuePtr cmd; virJSONValuePtr reply = NULL; virJSONValuePtr devices; - virHashTablePtr hash = NULL; if (!(cmd = qemuMonitorJSONMakeCommand("query-blockstats", NULL))) return -1; - if (!(hash = virHashCreate(10, virHashValueFree))) - goto cleanup; - if ((rc = qemuMonitorJSONCommand(mon, cmd, &reply)) < 0) goto cleanup; @@ -1924,12 +1923,9 @@ qemuMonitorJSONGetAllBlockStatsInfo(qemuMonitorPtr mon, } - *ret_stats = hash; - hash = NULL; ret = 0; cleanup: - 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 23589cf..0fcb0c0 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -82,7 +82,7 @@ int qemuMonitorJSONGetBlockStatsInfo(qemuMonitorPtr mon, long long *flush_req, long long *flush_total_times); int qemuMonitorJSONGetAllBlockStatsInfo(qemuMonitorPtr mon, - virHashTablePtr *ret_stats, + virHashTablePtr hash, bool backingChain); int qemuMonitorJSONBlockStatsUpdateCapacity(qemuMonitorPtr mon, virHashTablePtr stats, -- 2.2.2
qemu HMP commands sent by libvirt are terminated just by a '\r'. The fake monitor used in tests wasn't prepared to handle this and the communication would hang on an attempt to do a HMP conversation. Add a special case for handling commands separated by \r in case HMP is used. --- tests/qemumonitortestutils.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/qemumonitortestutils.c b/tests/qemumonitortestutils.c index 8155a69..3d34942 100644 --- a/tests/qemumonitortestutils.c +++ b/tests/qemumonitortestutils.c @@ -251,7 +251,8 @@ qemuMonitorTestIO(virNetSocketPtr sock, * if so, handle that command */ t1 = test->incoming; - while ((t2 = strstr(t1, "\n"))) { + while ((t2 = strstr(t1, "\n")) || + (!test->json && (t2 = strstr(t1, "\r")))) { *t2 = '\0'; if (qemuMonitorTestProcessCommand(test, t1) < 0) { -- 2.2.2
Add a different version of parser for "info blockstats" that basically parses the same information as the existing copy of the function. This will allow us to remove the single device version qemuMonitorGetBlockStatsInfo in the future. The new implementation uses few new helpers so it should be more understandable and provides a test case to verify that it works. --- src/qemu/qemu_monitor.c | 16 +++++- src/qemu/qemu_monitor_text.c | 129 +++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_text.h | 3 + tests/Makefile.am | 8 ++- tests/qemumonitortest.c | 89 ++++++++++++++++++++++++++++- 5 files changed, 240 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 95a6989..149e743 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -1867,8 +1867,20 @@ qemuMonitorGetAllBlockStatsInfo(qemuMonitorPtr mon, if (!(*ret_stats = virHashCreate(10, virHashValueFree))) goto error; - if (qemuMonitorJSONGetAllBlockStatsInfo(mon, *ret_stats, backingChain) < 0) - goto error; + if (mon->json) { + if (qemuMonitorJSONGetAllBlockStatsInfo(mon, *ret_stats, backingChain) < 0) + goto error; + } else { + if (backingChain) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("text monitor doesn't support block stats for " + "backing chain members")); + goto error; + } + + if (qemuMonitorTextGetAllBlockStatsInfo(mon, *ret_stats) < 0) + goto error; + } return 0; diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index 2de281f..8b2ef90 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -838,6 +838,135 @@ int qemuMonitorTextGetBlockInfo(qemuMonitorPtr mon, return ret; } + +int +qemuMonitorTextGetAllBlockStatsInfo(qemuMonitorPtr mon, + virHashTablePtr hash) +{ + qemuBlockStatsPtr stats = NULL; + char *info = NULL; + char *dev_name; + char **lines = NULL; + char **values = NULL; + char *line; + char *value; + char *key; + size_t i; + size_t j; + int ret = -1; + + if (qemuMonitorHMPCommand(mon, "info blockstats", &info) < 0) + goto cleanup; + + /* If the command isn't supported then qemu prints the supported info + * commands, so the output starts "info ". Since this is unlikely to be + * the name of a block device, we can use this to detect if qemu supports + * the command. */ + if (strstr(info, "\ninfo ")) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("'info blockstats' not supported by this qemu")); + goto cleanup; + } + + /* The output format for both qemu & KVM is: + * blockdevice: rd_bytes=% wr_bytes=% rd_operations=% wr_operations=% + * (repeated for each block device) + * where '%' is a 64 bit number. + */ + if (!(lines = virStringSplit(info, "\n", 0))) + goto cleanup; + + for (i = 0; lines[i] && *lines[i]; i++) { + line = lines[i]; + + if (VIR_ALLOC(stats) < 0) + goto cleanup; + + /* set the entries to -1, the JSON monitor enforces them, but it would + * be overly complex to achieve this here */ + stats->rd_req = -1; + stats->rd_bytes = -1; + stats->wr_req = -1; + stats->wr_bytes = -1; + stats->rd_total_times = -1; + stats->wr_total_times = -1; + stats->flush_req = -1; + stats->flush_total_times = -1; + + /* extract device name and make sure that it's followed by + * a colon and space */ + dev_name = line; + if (!(line = strchr(line, ':')) && line[1] != ' ') { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("info blockstats reply was malformed")); + goto cleanup; + } + + *line = '\0'; + line += 2; + + if (STRPREFIX(dev_name, QEMU_DRIVE_HOST_PREFIX)) + dev_name += strlen(QEMU_DRIVE_HOST_PREFIX); + + if (!(values = virStringSplit(line, " ", 0))) + goto cleanup; + + for (j = 0; values[j] && *values[j]; j++) { + key = values[j]; + + if (!(value = strchr(key, '='))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("info blockstats entry was malformed")); + goto cleanup; + } + + *value = '\0'; + value++; + +#define QEMU_MONITOR_TEXT_READ_BLOCK_STAT(NAME, VAR) \ + if (STREQ(key, NAME)) { \ + if (virStrToLong_ll(value, NULL, 10, &VAR) < 0) { \ + virReportError(VIR_ERR_INTERNAL_ERROR, \ + _("'info blockstats' contains malformed " \ + "parameter '%s' value '%s'"), NAME, value);\ + goto cleanup; \ + } \ + continue; \ + } + + QEMU_MONITOR_TEXT_READ_BLOCK_STAT("rd_bytes", stats->rd_bytes); + QEMU_MONITOR_TEXT_READ_BLOCK_STAT("wr_bytes", stats->wr_bytes); + QEMU_MONITOR_TEXT_READ_BLOCK_STAT("rd_operations", stats->rd_req); + QEMU_MONITOR_TEXT_READ_BLOCK_STAT("wr_operations", stats->wr_req); + QEMU_MONITOR_TEXT_READ_BLOCK_STAT("rd_total_time_ns", stats->rd_total_times); + QEMU_MONITOR_TEXT_READ_BLOCK_STAT("wr_total_time_ns", stats->wr_total_times); + QEMU_MONITOR_TEXT_READ_BLOCK_STAT("flush_operations", stats->flush_req); + QEMU_MONITOR_TEXT_READ_BLOCK_STAT("flush_total_time_ns", stats->flush_total_times); +#undef QEMU_MONITOR_TEXT_READ_BLOCK_STAT + + /* log if we get statistic element different from the above */ + VIR_DEBUG("unknown block stat field '%s'", key); + } + + if (virHashAddEntry(hash, dev_name, stats) < 0) + goto cleanup; + stats = NULL; + + virStringFreeList(values); + values = NULL; + } + + ret = 0; + + cleanup: + virStringFreeList(lines); + virStringFreeList(values); + VIR_FREE(stats); + VIR_FREE(info); + return ret; +} + + int qemuMonitorTextGetBlockStatsInfo(qemuMonitorPtr mon, const char *dev_name, long long *rd_req, diff --git a/src/qemu/qemu_monitor_text.h b/src/qemu/qemu_monitor_text.h index 695ac28..a1bc2b2 100644 --- a/src/qemu/qemu_monitor_text.h +++ b/src/qemu/qemu_monitor_text.h @@ -60,6 +60,9 @@ int qemuMonitorTextGetMemoryStats(qemuMonitorPtr mon, unsigned int nr_stats); int qemuMonitorTextGetBlockInfo(qemuMonitorPtr mon, virHashTablePtr table); + +int qemuMonitorTextGetAllBlockStatsInfo(qemuMonitorPtr mon, + virHashTablePtr hash); int qemuMonitorTextGetBlockStatsInfo(qemuMonitorPtr mon, const char *dev_name, long long *rd_req, diff --git a/tests/Makefile.am b/tests/Makefile.am index 938270c..9277c13 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -568,8 +568,12 @@ qemuargv2xmltest_LDADD = $(qemu_LDADDS) $(LDADDS) qemuhelptest_SOURCES = qemuhelptest.c testutils.c testutils.h qemuhelptest_LDADD = $(qemu_LDADDS) $(LDADDS) -qemumonitortest_SOURCES = qemumonitortest.c testutils.c testutils.h -qemumonitortest_LDADD = $(qemu_LDADDS) $(LDADDS) +qemumonitortest_SOURCES = \ + qemumonitortest.c \ + testutils.c testutils.h \ + testutilsqemu.c testutilsqemu.h +qemumonitortest_LDADD = libqemumonitortestutils.la \ + $(qemu_LDADDS) $(LDADDS) qemumonitorjsontest_SOURCES = \ qemumonitorjsontest.c \ diff --git a/tests/qemumonitortest.c b/tests/qemumonitortest.c index 1c13a89..d73bbf1 100644 --- a/tests/qemumonitortest.c +++ b/tests/qemumonitortest.c @@ -12,6 +12,10 @@ # include "internal.h" # include "viralloc.h" # include "qemu/qemu_monitor.h" +# include "qemu/qemu_monitor_text.h" +# include "qemumonitortestutils.h" + +# define VIR_FROM_THIS VIR_FROM_NONE struct testEscapeString { @@ -86,21 +90,104 @@ static int testUnescapeArg(const void *data ATTRIBUTE_UNUSED) return 0; } +struct blockInfoData { + const char *dev; + qemuBlockStats data; +}; + +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}} +}; + +static const char testBlockInfoReply[] = +"(qemu) info blockstats\r\n" +"vda: rd_operations=11 rd_bytes=12 wr_operations=13 wr_bytes=14 rd_total_time_ns=15 wr_total_time_ns=16 flush_operations=17 flush_total_time_ns=18\n" +"vdb: rd_total_time_ns=25 wr_total_time_ns=26 flush_operations=27 flush_total_time_ns=28 rd_operations=21 rd_bytes=22 wr_operations=23 wr_bytes=24 \n" +"drive-vdc: rd_operations=31 rd_bytes=32 wr_operations=33 rd_total_time_ns=35 wr_total_time_ns=36 flush_operations=37 flush_total_time_ns=38\n" +"vdd: \n" +"vde: rd_operations=41 rd_bytes=42 wr_operations=43 wr_bytes=44 rd_total_time_ns=45 wr_total_time_ns=46 flush_operations=47 flush_total_time_ns=48\n" +"(qemu) "; + +static int +testMonitorTextBlockInfo(const void *opaque) +{ + virDomainXMLOptionPtr xmlopt = (virDomainXMLOptionPtr) opaque; + qemuMonitorTestPtr test = qemuMonitorTestNewSimple(false, xmlopt); + virHashTablePtr blockstats = NULL; + size_t i; + int ret = -1; + + if (!test) + return -1; + + if (!(blockstats = virHashCreate(10, virHashValueFree))) + goto cleanup; + + if (qemuMonitorTestAddItem(test, "info", testBlockInfoReply) < 0) + goto cleanup; + + if (qemuMonitorTextGetAllBlockStatsInfo(qemuMonitorTestGetMonitor(test), + blockstats) < 0) + goto cleanup; + + for (i = 0; i < ARRAY_CARDINALITY(testBlockInfoData); i++) { + qemuBlockStatsPtr entry; + + if (!(entry = virHashLookup(blockstats, testBlockInfoData[i].dev))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "device '%s' was not found in text block stats reply", + testBlockInfoData[i].dev); + goto cleanup; + } + + if (memcmp(entry, &testBlockInfoData[i].data, sizeof(qemuBlockStats)) != 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "block stats for device '%s' differ", + testBlockInfoData[i].dev); + goto cleanup; + } + } + + ret = 0; + + cleanup: + qemuMonitorTestFree(test); + virHashFree(blockstats); + return ret; +} + + static int mymain(void) { + virDomainXMLOptionPtr xmlopt; int result = 0; + if (virThreadInitialize() < 0 || + !(xmlopt = virQEMUDriverCreateXMLConf(NULL))) + return EXIT_FAILURE; + + virEventRegisterDefaultImpl(); + # define DO_TEST(_name) \ do { \ if (virtTestRun("qemu monitor "#_name, test##_name, \ - NULL) < 0) { \ + xmlopt) < 0) { \ result = -1; \ } \ } while (0) DO_TEST(EscapeArg); DO_TEST(UnescapeArg); + DO_TEST(MonitorTextBlockInfo); + + virObjectUnref(xmlopt); return result == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 2.2.2
The function that is extracting block stats data from the QMP monitor reply contains a lot of repeated code. Since I'd be changing each of the copies in the next patch, lets convert it to a macro right away. --- src/qemu/qemu_monitor_json.c | 77 ++++++++++---------------------------------- 1 file changed, 17 insertions(+), 60 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index c88c7c3..52e1ad6 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -1789,66 +1789,23 @@ qemuMonitorJSONGetOneBlockStatsInfo(virJSONValuePtr dev, goto cleanup; } - if (virJSONValueObjectGetNumberLong(stats, "rd_bytes", - &bstats->rd_bytes) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot read %s statistic"), - "rd_bytes"); - goto cleanup; - } - if (virJSONValueObjectGetNumberLong(stats, "rd_operations", - &bstats->rd_req) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot read %s statistic"), - "rd_operations"); - goto cleanup; - } - if (virJSONValueObjectHasKey(stats, "rd_total_time_ns") && - (virJSONValueObjectGetNumberLong(stats, "rd_total_time_ns", - &bstats->rd_total_times) < 0)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot read %s statistic"), - "rd_total_time_ns"); - goto cleanup; - } - if (virJSONValueObjectGetNumberLong(stats, "wr_bytes", - &bstats->wr_bytes) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot read %s statistic"), - "wr_bytes"); - goto cleanup; - } - if (virJSONValueObjectGetNumberLong(stats, "wr_operations", - &bstats->wr_req) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot read %s statistic"), - "wr_operations"); - goto cleanup; - } - if (virJSONValueObjectHasKey(stats, "wr_total_time_ns") && - (virJSONValueObjectGetNumberLong(stats, "wr_total_time_ns", - &bstats->wr_total_times) < 0)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot read %s statistic"), - "wr_total_time_ns"); - goto cleanup; - } - if (virJSONValueObjectHasKey(stats, "flush_operations") && - (virJSONValueObjectGetNumberLong(stats, "flush_operations", - &bstats->flush_req) < 0)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot read %s statistic"), - "flush_operations"); - goto cleanup; - } - if (virJSONValueObjectHasKey(stats, "flush_total_time_ns") && - (virJSONValueObjectGetNumberLong(stats, "flush_total_time_ns", - &bstats->flush_total_times) < 0)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot read %s statistic"), - "flush_total_time_ns"); - goto cleanup; - } +#define QEMU_MONITOR_JSON_BLOCK_STAT(NAME, VAR, MANDATORY) \ + if (MANDATORY || virJSONValueObjectHasKey(stats, NAME)) { \ + if (virJSONValueObjectGetNumberLong(stats, NAME, &VAR) < 0) { \ + virReportError(VIR_ERR_INTERNAL_ERROR, \ + _("cannot read %s statistic"), NAME); \ + goto cleanup; \ + } \ + } + QEMU_MONITOR_JSON_BLOCK_STAT("rd_bytes", bstats->rd_bytes, true); + QEMU_MONITOR_JSON_BLOCK_STAT("wr_bytes", bstats->wr_bytes, true); + QEMU_MONITOR_JSON_BLOCK_STAT("rd_operations", bstats->rd_req, true); + QEMU_MONITOR_JSON_BLOCK_STAT("wr_operations", bstats->wr_req, true); + QEMU_MONITOR_JSON_BLOCK_STAT("rd_total_time_ns", bstats->rd_total_times, false); + QEMU_MONITOR_JSON_BLOCK_STAT("wr_total_time_ns", bstats->wr_total_times, false); + QEMU_MONITOR_JSON_BLOCK_STAT("flush_operations", bstats->flush_req, false); + QEMU_MONITOR_JSON_BLOCK_STAT("flush_total_time_ns", bstats->flush_total_times, false); +#undef QEMU_MONITOR_JSON_BLOCK_STAT /* it's ok to not have this information here. Just skip silently. */ qemuMonitorJSONDevGetBlockExtent(dev, &bstats->wr_highest_offset); -- 2.2.2
On Tue, Mar 10, 2015 at 05:26:35PM +0100, Peter Krempa wrote:
The function that is extracting block stats data from the QMP monitor reply contains a lot of repeated code. Since I'd be changing each of the copies in the next patch, lets convert it to a macro right away. --- src/qemu/qemu_monitor_json.c | 77 ++++++++++---------------------------------- 1 file changed, 17 insertions(+), 60 deletions(-)
+#define QEMU_MONITOR_JSON_BLOCK_STAT(NAME, VAR, MANDATORY) \ + if (MANDATORY || virJSONValueObjectHasKey(stats, NAME)) { \ + if (virJSONValueObjectGetNumberLong(stats, NAME, &VAR) < 0) { \ + virReportError(VIR_ERR_INTERNAL_ERROR, \ + _("cannot read %s statistic"), NAME); \ + goto cleanup; \ + } \ + } + QEMU_MONITOR_JSON_BLOCK_STAT("rd_bytes", bstats->rd_bytes, true); + QEMU_MONITOR_JSON_BLOCK_STAT("wr_bytes", bstats->wr_bytes, true); + QEMU_MONITOR_JSON_BLOCK_STAT("rd_operations", bstats->rd_req, true); + QEMU_MONITOR_JSON_BLOCK_STAT("wr_operations", bstats->wr_req, true); + QEMU_MONITOR_JSON_BLOCK_STAT("rd_total_time_ns", bstats->rd_total_times, false); + QEMU_MONITOR_JSON_BLOCK_STAT("wr_total_time_ns", bstats->wr_total_times, false); + QEMU_MONITOR_JSON_BLOCK_STAT("flush_operations", bstats->flush_req, false); + QEMU_MONITOR_JSON_BLOCK_STAT("flush_total_time_ns", bstats->flush_total_times, false); +#undef QEMU_MONITOR_JSON_BLOCK_STAT
This macro could also use a verb. Jan
Our virDomainBlockStatsFlags API uses the old approach where, when it's called without the typed parameter array, returs the count of parameters supported by qemu. The supported parameter count is obtained via separate monitor calls which is a waste since we can calculate it when gathering the data. This patch adds code to the qemuMonitorGetAllBlockStatsInfo workers that allows to track the count of supported fields reported by qemu and will allow to remove the old duplicate code. --- src/qemu/qemu_monitor.c | 26 +++++++++++++++++++------- src/qemu/qemu_monitor_json.c | 15 +++++++++++---- src/qemu/qemu_monitor_text.c | 10 +++++++++- 3 files changed, 39 insertions(+), 12 deletions(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 149e743..e4ff06e 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -1848,14 +1848,24 @@ int qemuMonitorGetBlockStatsInfo(qemuMonitorPtr mon, } -/* Creates a hash table in 'ret_stats' with all block stats. - * Returns <0 on error, 0 on success. +/** + * qemuMonitorGetAllBlockStatsInfo: + * @mon: monitor object + * @ret_stats: pointer that is filled with a hash table containing the stats + * @backingChain: recurse into the bakcing chain of devices + * + * Creates a hash table in @ret_stats with block stats of all devices. In case + * @backingChain is true @ret_stats will additionally contain stats for + * backing chain members of block devices. + * + * Returns < 0 on error, count of supported block stats fields on success. */ int qemuMonitorGetAllBlockStatsInfo(qemuMonitorPtr mon, virHashTablePtr *ret_stats, bool backingChain) { + int ret = -1; VIR_DEBUG("mon=%p ret_stats=%p, backing=%d", mon, ret_stats, backingChain); if (!mon->json) { @@ -1868,8 +1878,8 @@ qemuMonitorGetAllBlockStatsInfo(qemuMonitorPtr mon, goto error; if (mon->json) { - if (qemuMonitorJSONGetAllBlockStatsInfo(mon, *ret_stats, backingChain) < 0) - goto error; + ret = qemuMonitorJSONGetAllBlockStatsInfo(mon, *ret_stats, + backingChain); } else { if (backingChain) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", @@ -1878,11 +1888,13 @@ qemuMonitorGetAllBlockStatsInfo(qemuMonitorPtr mon, goto error; } - if (qemuMonitorTextGetAllBlockStatsInfo(mon, *ret_stats) < 0) - goto error; + ret = qemuMonitorTextGetAllBlockStatsInfo(mon, *ret_stats); } - return 0; + if (ret < 0) + goto error; + + return ret; error: virHashFree(*ret_stats); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 52e1ad6..76baaf6 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -1773,6 +1773,7 @@ qemuMonitorJSONGetOneBlockStatsInfo(virJSONValuePtr dev, qemuBlockStatsPtr bstats = NULL; virJSONValuePtr stats; int ret = -1; + int nstats = 0; char *entry_name = qemuDomainStorageAlias(dev_name, depth); virJSONValuePtr backing; @@ -1791,6 +1792,7 @@ qemuMonitorJSONGetOneBlockStatsInfo(virJSONValuePtr dev, #define QEMU_MONITOR_JSON_BLOCK_STAT(NAME, VAR, MANDATORY) \ if (MANDATORY || virJSONValueObjectHasKey(stats, NAME)) { \ + nstats++; \ if (virJSONValueObjectGetNumberLong(stats, NAME, &VAR) < 0) { \ virReportError(VIR_ERR_INTERNAL_ERROR, \ _("cannot read %s statistic"), NAME); \ @@ -1820,7 +1822,7 @@ qemuMonitorJSONGetOneBlockStatsInfo(virJSONValuePtr dev, hash, true) < 0) goto cleanup; - ret = 0; + ret = nstats; cleanup: VIR_FREE(bstats); VIR_FREE(entry_name); @@ -1834,6 +1836,7 @@ qemuMonitorJSONGetAllBlockStatsInfo(qemuMonitorPtr mon, bool backingChain) { int ret = -1; + int nstats = 0; int rc; size_t i; virJSONValuePtr cmd; @@ -1874,13 +1877,17 @@ qemuMonitorJSONGetAllBlockStatsInfo(qemuMonitorPtr mon, goto cleanup; } - if (qemuMonitorJSONGetOneBlockStatsInfo(dev, dev_name, 0, hash, - backingChain) < 0) + rc = qemuMonitorJSONGetOneBlockStatsInfo(dev, dev_name, 0, hash, + backingChain); + + if (rc < 0) goto cleanup; + if (rc > nstats) + nstats = rc; } - ret = 0; + ret = nstats; cleanup: virJSONValueFree(cmd); diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index 8b2ef90..203859c 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -854,6 +854,8 @@ qemuMonitorTextGetAllBlockStatsInfo(qemuMonitorPtr mon, size_t i; size_t j; int ret = -1; + int nstats; + int maxstats = 0; if (qemuMonitorHMPCommand(mon, "info blockstats", &info) < 0) goto cleanup; @@ -911,6 +913,8 @@ qemuMonitorTextGetAllBlockStatsInfo(qemuMonitorPtr mon, if (!(values = virStringSplit(line, " ", 0))) goto cleanup; + nstats = 0; + for (j = 0; values[j] && *values[j]; j++) { key = values[j]; @@ -925,6 +929,7 @@ qemuMonitorTextGetAllBlockStatsInfo(qemuMonitorPtr mon, #define QEMU_MONITOR_TEXT_READ_BLOCK_STAT(NAME, VAR) \ if (STREQ(key, NAME)) { \ + nstats++; \ if (virStrToLong_ll(value, NULL, 10, &VAR) < 0) { \ virReportError(VIR_ERR_INTERNAL_ERROR, \ _("'info blockstats' contains malformed " \ @@ -948,6 +953,9 @@ qemuMonitorTextGetAllBlockStatsInfo(qemuMonitorPtr mon, VIR_DEBUG("unknown block stat field '%s'", key); } + if (nstats > maxstats) + maxstats = nstats; + if (virHashAddEntry(hash, dev_name, stats) < 0) goto cleanup; stats = NULL; @@ -956,7 +964,7 @@ qemuMonitorTextGetAllBlockStatsInfo(qemuMonitorPtr mon, values = NULL; } - ret = 0; + ret = maxstats; cleanup: virStringFreeList(lines); -- 2.2.2
On Tue, Mar 10, 2015 at 05:26:36PM +0100, Peter Krempa wrote:
Our virDomainBlockStatsFlags API uses the old approach where, when it's called without the typed parameter array, returs the count of parameters
s/returs/returns/
supported by qemu.
The supported parameter count is obtained via separate monitor calls which is a waste since we can calculate it when gathering the data.
This patch adds code to the qemuMonitorGetAllBlockStatsInfo workers that allows to track the count of supported fields reported by qemu and will allow to remove the old duplicate code. --- src/qemu/qemu_monitor.c | 26 +++++++++++++++++++------- src/qemu/qemu_monitor_json.c | 15 +++++++++++---- src/qemu/qemu_monitor_text.c | 10 +++++++++- 3 files changed, 39 insertions(+), 12 deletions(-)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 149e743..e4ff06e 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -1848,14 +1848,24 @@ int qemuMonitorGetBlockStatsInfo(qemuMonitorPtr mon, }
-/* Creates a hash table in 'ret_stats' with all block stats. - * Returns <0 on error, 0 on success. +/** + * qemuMonitorGetAllBlockStatsInfo: + * @mon: monitor object + * @ret_stats: pointer that is filled with a hash table containing the stats + * @backingChain: recurse into the bakcing chain of devices
s/bakcing/backing/ Jan
Extract the code to look up the disk alias and return the block stats struct so that it can be reused later in qemuDomainBlockStatsFlags. --- src/qemu/qemu_driver.c | 122 +++++++++++++++++++++++++++++++++---------------- 1 file changed, 82 insertions(+), 40 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 06168b1..e280cdf 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10480,6 +10480,80 @@ qemuDomainBlockResize(virDomainPtr dom, return ret; } + +/** + * qemuDomainBlocksStatsGather: + * @driver: driver object + * @vm: domain object + * @path: to gather the statistics for + * @retstats: returns pointer to structure holding the stats + * + * Gathers the block statistics for use in qemuDomainBlockStats* APIs. + * + * Returns -1 on error; number of filled block statistics on success. + */ +static int +qemuDomainBlocksStatsGather(virQEMUDriverPtr driver, + virDomainObjPtr vm, + const char *path, + qemuBlockStatsPtr *retstats) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + virDomainDiskDefPtr disk; + virHashTablePtr blockstats = NULL; + qemuBlockStatsPtr stats; + int nstats; + char *diskAlias = NULL; + int ret = -1; + + if (*path) { + int idx; + + if ((idx = virDomainDiskIndexByName(vm->def, path, false)) < 0) { + virReportError(VIR_ERR_INVALID_ARG, _("invalid path: %s"), path); + goto cleanup; + } + disk = vm->def->disks[idx]; + + if (!disk->info.alias) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("missing disk device alias name for %s"), disk->dst); + goto cleanup; + } + + if (VIR_STRDUP(diskAlias, disk->info.alias) < 0) + goto cleanup; + } else { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("summary statistics are not supported yet")); + goto cleanup; + } + + qemuDomainObjEnterMonitor(driver, vm); + nstats = qemuMonitorGetAllBlockStatsInfo(priv->mon, &blockstats, false); + if (qemuDomainObjExitMonitor(driver, vm) < 0 || nstats < 0) + goto cleanup; + + if (VIR_ALLOC(*retstats) < 0) + goto cleanup; + + if (!(stats = virHashLookup(blockstats, diskAlias))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot find statistics for device '%s'"), diskAlias); + goto cleanup; + } + + **retstats = *stats; + + ret = nstats; + + cleanup: + VIR_FREE(diskAlias); + virHashFree(blockstats); + return ret; +} + + /* This uses the 'info blockstats' monitor command which was * integrated into both qemu & kvm in late 2007. If the command is * not supported we detect this and return the appropriate error. @@ -10490,18 +10564,9 @@ qemuDomainBlockStats(virDomainPtr dom, virDomainBlockStatsPtr stats) { virQEMUDriverPtr driver = dom->conn->privateData; - int idx; + qemuBlockStatsPtr blockstats = NULL; int ret = -1; virDomainObjPtr vm; - virDomainDiskDefPtr disk = NULL; - qemuDomainObjPrivatePtr priv; - char *diskAlias = NULL; - - if (!*path) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", - _("summary statistics are not supported yet")); - return ret; - } if (!(vm = qemuDomObjFromDomain(dom))) goto cleanup; @@ -10518,47 +10583,24 @@ qemuDomainBlockStats(virDomainPtr dom, goto endjob; } - if ((idx = virDomainDiskIndexByName(vm->def, path, false)) < 0) { - virReportError(VIR_ERR_INVALID_ARG, - _("invalid path: %s"), path); + if (qemuDomainBlocksStatsGather(driver, vm, path, &blockstats) < 0) goto endjob; - } - disk = vm->def->disks[idx]; - - if (!disk->info.alias) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("missing disk device alias name for %s"), disk->dst); - goto endjob; - } - - if (VIR_STRDUP(diskAlias, disk->info.alias) < 0) - goto endjob; - - priv = vm->privateData; + stats->rd_req = blockstats->rd_req; + stats->rd_bytes = blockstats->rd_bytes; + stats->wr_req = blockstats->wr_req; + stats->wr_bytes = blockstats->wr_bytes; /* qemu doesn't report the error count */ stats->errs = -1; - qemuDomainObjEnterMonitor(driver, vm); - ret = qemuMonitorGetBlockStatsInfo(priv->mon, - diskAlias, - &stats->rd_req, - &stats->rd_bytes, - NULL, - &stats->wr_req, - &stats->wr_bytes, - NULL, - NULL, - NULL); - if (qemuDomainObjExitMonitor(driver, vm) < 0) - ret = -1; + ret = 0; endjob: qemuDomainObjEndJob(driver, vm); cleanup: qemuDomObjEndAPI(&vm); - VIR_FREE(diskAlias); + VIR_FREE(blockstats); return ret; } -- 2.2.2
On Tue, Mar 10, 2015 at 05:26:37PM +0100, Peter Krempa wrote:
Extract the code to look up the disk alias and return the block stats struct so that it can be reused later in qemuDomainBlockStatsFlags.
Maybe the commit message should mention that it now uses qemuMonitorGetAllBlockStatsInfo directly instead of qemuMonitorGetBlockStatsInfo? Jan
In the LXC driver, if the disk path is not provided the API returns total statistics for all disks of the domain. With the new text monitor implementation this can be now done in the qemu driver too. Add code that wil total the stats for all disks if the path is not provided. --- src/qemu/qemu_driver.c | 44 ++++++++++++++++++++++++++++++++++---------- 1 file changed, 34 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e280cdf..4458c52 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10481,6 +10481,30 @@ qemuDomainBlockResize(virDomainPtr dom, } +static void +qemuDomainBlockStatsGatherTotals(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *opaque) +{ + qemuBlockStatsPtr data = payload; + qemuBlockStatsPtr total = opaque; + +#define QEMU_BLOCK_STAT_TOTAL(NAME) \ + if (data->NAME > 0) \ + total->NAME += data->NAME + + QEMU_BLOCK_STAT_TOTAL(wr_bytes); + QEMU_BLOCK_STAT_TOTAL(wr_req); + QEMU_BLOCK_STAT_TOTAL(rd_bytes); + QEMU_BLOCK_STAT_TOTAL(rd_req); + QEMU_BLOCK_STAT_TOTAL(flush_req); + QEMU_BLOCK_STAT_TOTAL(wr_total_times); + QEMU_BLOCK_STAT_TOTAL(rd_total_times); + QEMU_BLOCK_STAT_TOTAL(flush_total_times); +#undef QEMU_BLOCK_STAT_TOTAL +} + + /** * qemuDomainBlocksStatsGather: * @driver: driver object @@ -10523,10 +10547,6 @@ qemuDomainBlocksStatsGather(virQEMUDriverPtr driver, if (VIR_STRDUP(diskAlias, disk->info.alias) < 0) goto cleanup; - } else { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", - _("summary statistics are not supported yet")); - goto cleanup; } qemuDomainObjEnterMonitor(driver, vm); @@ -10537,13 +10557,17 @@ qemuDomainBlocksStatsGather(virQEMUDriverPtr driver, if (VIR_ALLOC(*retstats) < 0) goto cleanup; - if (!(stats = virHashLookup(blockstats, diskAlias))) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot find statistics for device '%s'"), diskAlias); - goto cleanup; - } + if (diskAlias) { + if (!(stats = virHashLookup(blockstats, diskAlias))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot find statistics for device '%s'"), diskAlias); + goto cleanup; + } - **retstats = *stats; + **retstats = *stats; + } else { + virHashForEach(blockstats, qemuDomainBlockStatsGatherTotals, *retstats); + } ret = nstats; -- 2.2.2
Now that qemuDomainBlocksStatsGather provides functions of both qemuMonitorGetBlockStatsParamsNumber and qemuMonitorGetBlockStatsInfo we can reuse it and kill a lot of code. Additionally as a bonus qemuDomainBlockStatsFlags will now support summary statistics so add a statement to the virsh man page about that. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1142636 man --- src/qemu/qemu_driver.c | 82 +++++++++++--------------------------------------- tools/virsh.pod | 5 +-- 2 files changed, 21 insertions(+), 66 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 4458c52..c35637b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10637,21 +10637,14 @@ qemuDomainBlockStatsFlags(virDomainPtr dom, unsigned int flags) { virQEMUDriverPtr driver = dom->conn->privateData; - int idx; - int tmp, ret = -1; virDomainObjPtr vm; - qemuDomainObjPrivatePtr priv; - long long rd_req, rd_bytes, wr_req, wr_bytes, rd_total_times; - long long wr_total_times, flush_req, flush_total_times; - char *diskAlias = NULL; + qemuBlockStatsPtr blockstats = NULL; + int nstats; + int ret = -1; - virCheckFlags(VIR_TYPED_PARAM_STRING_OKAY, -1); + VIR_DEBUG("params=%p, flags=%x", params, flags); - if (!*path) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", - _("summary statistics are not supported yet")); - return ret; - } + virCheckFlags(VIR_TYPED_PARAM_STRING_OKAY, -1); /* We don't return strings, and thus trivially support this flag. */ flags &= ~VIR_TYPED_PARAM_STRING_OKAY; @@ -10671,64 +10664,26 @@ qemuDomainBlockStatsFlags(virDomainPtr dom, goto endjob; } - if (*nparams != 0) { - virDomainDiskDefPtr disk = NULL; - if ((idx = virDomainDiskIndexByName(vm->def, path, false)) < 0) { - virReportError(VIR_ERR_INVALID_ARG, - _("invalid path: %s"), path); - goto endjob; - } - disk = vm->def->disks[idx]; - - if (!disk->info.alias) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("missing disk device alias name for %s"), - disk->dst); - goto endjob; - } - if (VIR_STRDUP(diskAlias, disk->info.alias) < 0) - goto endjob; - } - - priv = vm->privateData; - VIR_DEBUG("priv=%p, params=%p, flags=%x", priv, params, flags); - - qemuDomainObjEnterMonitor(driver, vm); - tmp = *nparams; - ret = qemuMonitorGetBlockStatsParamsNumber(priv->mon, nparams); - - if (tmp == 0 || ret < 0) { - ignore_value(qemuDomainObjExitMonitor(driver, vm)); + if ((nstats = qemuDomainBlocksStatsGather(driver, vm, path, + &blockstats)) < 0) goto endjob; - } - - ret = qemuMonitorGetBlockStatsInfo(priv->mon, - diskAlias, - &rd_req, - &rd_bytes, - &rd_total_times, - &wr_req, - &wr_bytes, - &wr_total_times, - &flush_req, - &flush_total_times); - - if (qemuDomainObjExitMonitor(driver, vm) < 0) - ret = -1; - if (ret < 0) + /* return count of supported stats */ + if (*nparams == 0) { + *nparams = nstats; + ret = 0; goto endjob; + } - tmp = 0; - ret = -1; + nstats = 0; /* the following macro shall not be used with side effects statements */ #define QEMU_BLOCK_STAT(VAR, NAME) \ - if (tmp < *nparams && (VAR) != -1) { \ - if (virTypedParameterAssign(params + tmp, NAME, VIR_TYPED_PARAM_LLONG,\ - (VAR)) < 0) \ + if (nstats < *nparams && (blockstats->VAR) != -1) { \ + if (virTypedParameterAssign(params + nstats, NAME, \ + VIR_TYPED_PARAM_LLONG, (blockstats->VAR)) < 0) \ goto endjob; \ - tmp++; \ + nstats++; \ } QEMU_BLOCK_STAT(wr_bytes, VIR_DOMAIN_BLOCK_STATS_WRITE_BYTES); @@ -10748,13 +10703,12 @@ qemuDomainBlockStatsFlags(virDomainPtr dom, /* Field 'errs' is meaningless for QEMU, won't set it. */ ret = 0; - *nparams = tmp; + *nparams = nstats; endjob: qemuDomainObjEndJob(driver, vm); cleanup: - VIR_FREE(diskAlias); qemuDomObjEndAPI(&vm); return ret; } diff --git a/tools/virsh.pod b/tools/virsh.pod index e65378e..afc380d 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -709,8 +709,9 @@ return an error instead. Get device block stats for a running domain. A I<block-device> corresponds to a unique target name (<target dev='name'/>) or source file (<source file='name'/>) for one of the disk devices attached to I<domain> (see -also B<domblklist> for listing these names). On a lxc domain, omitting the -I<block-device> yields device block stats summarily for the entire domain. +also B<domblklist> for listing these names). On a lxc or qemu domain, +omitting the I<block-device> yields device block stats summarily for the +entire domain. Use I<--human> for a more human readable output. -- 2.2.2
On Tue, Mar 10, 2015 at 05:26:39PM +0100, Peter Krempa wrote:
Now that qemuDomainBlocksStatsGather provides functions of both qemuMonitorGetBlockStatsParamsNumber and qemuMonitorGetBlockStatsInfo we can reuse it and kill a lot of code.
Additionally as a bonus qemuDomainBlockStatsFlags will now support summary statistics so add a statement to the virsh man page about that.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1142636
man
What manual page do you want?
--- src/qemu/qemu_driver.c | 82 +++++++++++--------------------------------------- tools/virsh.pod | 5 +-- 2 files changed, 21 insertions(+), 66 deletions(-)
@@ -10748,13 +10703,12 @@ qemuDomainBlockStatsFlags(virDomainPtr dom, /* Field 'errs' is meaningless for QEMU, won't set it. */
Do we need this comment?
ret = 0; - *nparams = tmp; + *nparams = nstats;
endjob: qemuDomainObjEndJob(driver, vm);
cleanup: - VIR_FREE(diskAlias);
VIR_FREE(blockstats);
qemuDomObjEndAPI(&vm); return ret; }
ACK with the leak fixed. Jan
Use the new single function instead of calling qemuMonitorJSONGetBlockStatsInfo and qemuMonitorJSONGetBlockStatsParamsNumber. This will allow to delete the functions later while still maintaining coverage. --- tests/qemumonitorjsontest.c | 62 +++++++++++++++------------------------------ 1 file changed, 21 insertions(+), 41 deletions(-) diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index da9cd6c..1380df1 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -1435,11 +1435,9 @@ testQemuMonitorJSONqemuMonitorJSONGetBlockStatsInfo(const void *data) { virDomainXMLOptionPtr xmlopt = (virDomainXMLOptionPtr)data; qemuMonitorTestPtr test = qemuMonitorTestNewSimple(true, xmlopt); + virHashTablePtr blockstats = NULL; + qemuBlockStatsPtr stats; int ret = -1; - long long rd_req, rd_bytes, rd_total_times; - long long wr_req, wr_bytes, wr_total_times; - long long flush_req, flush_total_times; - int nparams; unsigned long long extent; const char *reply = @@ -1537,22 +1535,24 @@ testQemuMonitorJSONqemuMonitorJSONGetBlockStatsInfo(const void *data) if (qemuMonitorTestAddItem(test, "query-blockstats", reply) < 0 || qemuMonitorTestAddItem(test, "query-blockstats", reply) < 0 || qemuMonitorTestAddItem(test, "query-blockstats", reply) < 0 || - qemuMonitorTestAddItem(test, "query-blockstats", reply) < 0 || - qemuMonitorTestAddItem(test, "query-blockstats", reply) < 0 || - qemuMonitorTestAddItem(test, "query-blockstats", reply) < 0 || qemuMonitorTestAddItem(test, "query-blockstats", reply) < 0) goto cleanup; #define CHECK0(var, value) \ - if (var != value) { \ + if (stats->var != value) { \ virReportError(VIR_ERR_INTERNAL_ERROR, \ "Invalid " #var " value: %lld, expected %d", \ - var, value); \ + stats->var, value); \ goto cleanup; \ } -#define CHECK(RD_REQ, RD_BYTES, RD_TOTAL_TIMES, WR_REQ, WR_BYTES, WR_TOTAL_TIMES, \ - FLUSH_REQ, FLUSH_TOTAL_TIMES) \ +#define CHECK(NAME, RD_REQ, RD_BYTES, RD_TOTAL_TIMES, WR_REQ, WR_BYTES, \ + WR_TOTAL_TIMES, FLUSH_REQ, FLUSH_TOTAL_TIMES) \ + if (!(stats = virHashLookup(blockstats, NAME))) { \ + virReportError(VIR_ERR_INTERNAL_ERROR, \ + "block stats for device '%s' is missing", NAME); \ + goto cleanup; \ + } \ CHECK0(rd_req, RD_REQ) \ CHECK0(rd_bytes, RD_BYTES) \ CHECK0(rd_total_times, RD_TOTAL_TIMES) \ @@ -1562,41 +1562,20 @@ testQemuMonitorJSONqemuMonitorJSONGetBlockStatsInfo(const void *data) CHECK0(flush_req, FLUSH_REQ) \ CHECK0(flush_total_times, FLUSH_TOTAL_TIMES) - if (qemuMonitorJSONGetBlockStatsInfo(qemuMonitorTestGetMonitor(test), "virtio-disk0", - &rd_req, &rd_bytes, &rd_total_times, - &wr_req, &wr_bytes, &wr_total_times, - &flush_req, &flush_total_times) < 0) - goto cleanup; - - CHECK(1279, 28505088, 640616474, 174, 2845696, 530699221, 0, 0) - - if (qemuMonitorJSONGetBlockStatsInfo(qemuMonitorTestGetMonitor(test), "virtio-disk1", - &rd_req, &rd_bytes, &rd_total_times, - &wr_req, &wr_bytes, &wr_total_times, - &flush_req, &flush_total_times) < 0) + if (qemuMonitorGetAllBlockStatsInfo(qemuMonitorTestGetMonitor(test), + &blockstats, false) < 0) goto cleanup; - CHECK(85, 348160, 8232156, 0, 0, 0, 0, 0) - - if (qemuMonitorJSONGetBlockStatsInfo(qemuMonitorTestGetMonitor(test), "ide0-1-0", - &rd_req, &rd_bytes, &rd_total_times, - &wr_req, &wr_bytes, &wr_total_times, - &flush_req, &flush_total_times) < 0) - goto cleanup; - - CHECK(16, 49250, 1004952, 0, 0, 0, 0, 0) - - if (qemuMonitorJSONGetBlockStatsParamsNumber(qemuMonitorTestGetMonitor(test), - &nparams) < 0) - goto cleanup; - - if (nparams != 8) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "Invalid number of stats: %d, expected 8", - nparams); + if (!blockstats) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + "qemuMonitorJSONGetBlockStatsInfo didn't return stats"); 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) + if (qemuMonitorJSONGetBlockExtent(qemuMonitorTestGetMonitor(test), "virtio-disk0", &extent) < 0) goto cleanup; @@ -1637,6 +1616,7 @@ testQemuMonitorJSONqemuMonitorJSONGetBlockStatsInfo(const void *data) cleanup: qemuMonitorTestFree(test); + virHashFree(blockstats); return ret; } -- 2.2.2
The functions and their QMP and HMP implementations are no longer needed since everything is now done via the *AllStats functions. --- src/qemu/qemu_monitor.c | 62 -------------- src/qemu/qemu_monitor.h | 14 --- src/qemu/qemu_monitor_json.c | 135 ----------------------------- src/qemu/qemu_monitor_json.h | 12 --- src/qemu/qemu_monitor_text.c | 198 ------------------------------------------- src/qemu/qemu_monitor_text.h | 12 --- 6 files changed, 433 deletions(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index e4ff06e..4bdd8d8 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -1808,45 +1808,6 @@ qemuMonitorBlockInfoLookup(virHashTablePtr blockInfo, return info; } -int qemuMonitorGetBlockStatsInfo(qemuMonitorPtr mon, - const char *dev_name, - long long *rd_req, - long long *rd_bytes, - long long *rd_total_times, - long long *wr_req, - long long *wr_bytes, - long long *wr_total_times, - long long *flush_req, - long long *flush_total_times) -{ - int ret; - VIR_DEBUG("mon=%p dev=%s", mon, dev_name); - - if (!mon) { - virReportError(VIR_ERR_INVALID_ARG, "%s", - _("monitor must not be NULL")); - return -1; - } - - if (mon->json) - ret = qemuMonitorJSONGetBlockStatsInfo(mon, dev_name, - rd_req, rd_bytes, - rd_total_times, - wr_req, wr_bytes, - wr_total_times, - flush_req, - flush_total_times); - else - ret = qemuMonitorTextGetBlockStatsInfo(mon, dev_name, - rd_req, rd_bytes, - rd_total_times, - wr_req, wr_bytes, - wr_total_times, - flush_req, - flush_total_times); - return ret; -} - /** * qemuMonitorGetAllBlockStatsInfo: @@ -1921,29 +1882,6 @@ qemuMonitorBlockStatsUpdateCapacity(qemuMonitorPtr mon, } -/* Return 0 and update @nparams with the number of block stats - * QEMU supports if success. Return -1 if failure. - */ -int qemuMonitorGetBlockStatsParamsNumber(qemuMonitorPtr mon, - int *nparams) -{ - int ret; - VIR_DEBUG("mon=%p nparams=%p", mon, nparams); - - if (!mon) { - virReportError(VIR_ERR_INVALID_ARG, "%s", - _("monitor must not be NULL")); - return -1; - } - - if (mon->json) - ret = qemuMonitorJSONGetBlockStatsParamsNumber(mon, nparams); - else - ret = qemuMonitorTextGetBlockStatsParamsNumber(mon, nparams); - - return ret; -} - int qemuMonitorGetBlockExtent(qemuMonitorPtr mon, const char *dev_name, unsigned long long *extent) diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 72498b3..b30da34 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -357,17 +357,6 @@ struct qemuDomainDiskInfo * qemuMonitorBlockInfoLookup(virHashTablePtr blockInfo, const char *dev_name); -int qemuMonitorGetBlockStatsInfo(qemuMonitorPtr mon, - const char *dev_name, - long long *rd_req, - long long *rd_bytes, - long long *rd_total_times, - long long *wr_req, - long long *wr_bytes, - long long *wr_total_times, - long long *flush_req, - long long *flush_total_times); - typedef struct _qemuBlockStats qemuBlockStats; typedef qemuBlockStats *qemuBlockStatsPtr; struct _qemuBlockStats { @@ -394,9 +383,6 @@ int qemuMonitorBlockStatsUpdateCapacity(qemuMonitorPtr mon, bool backingChain) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); -int qemuMonitorGetBlockStatsParamsNumber(qemuMonitorPtr mon, - int *nparams); - int qemuMonitorGetBlockExtent(qemuMonitorPtr mon, const char *dev_name, unsigned long long *extent); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 76baaf6..0f32a0a 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -1668,67 +1668,6 @@ int qemuMonitorJSONGetBlockInfo(qemuMonitorPtr mon, } -int qemuMonitorJSONGetBlockStatsInfo(qemuMonitorPtr mon, - const char *dev_name, - long long *rd_req, - long long *rd_bytes, - long long *rd_total_times, - long long *wr_req, - long long *wr_bytes, - long long *wr_total_times, - long long *flush_req, - long long *flush_total_times) -{ - qemuBlockStats *stats; - virHashTablePtr blockstats = NULL; - int ret = -1; - - *rd_req = *rd_bytes = -1; - *wr_req = *wr_bytes = -1; - - if (rd_total_times) - *rd_total_times = -1; - if (wr_total_times) - *wr_total_times = -1; - if (flush_req) - *flush_req = -1; - if (flush_total_times) - *flush_total_times = -1; - - if (!(blockstats = virHashCreate(10, virHashValueFree))) - goto cleanup; - - if (qemuMonitorJSONGetAllBlockStatsInfo(mon, blockstats, false) < 0) - goto cleanup; - - 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; - - if (rd_total_times) - *rd_total_times = stats->rd_total_times; - if (wr_total_times) - *wr_total_times = stats->wr_total_times; - if (flush_req) - *flush_req = stats->flush_req; - if (flush_total_times) - *flush_total_times = stats->flush_total_times; - - ret = 0; - - cleanup: - virHashFree(blockstats); - return ret; -} - - typedef enum { QEMU_MONITOR_BLOCK_EXTENT_ERROR_OK, QEMU_MONITOR_BLOCK_EXTENT_ERROR_NOPARENT, @@ -2013,80 +1952,6 @@ qemuMonitorJSONBlockStatsUpdateCapacity(qemuMonitorPtr mon, } -int qemuMonitorJSONGetBlockStatsParamsNumber(qemuMonitorPtr mon, - int *nparams) -{ - int ret, num = 0; - size_t i; - virJSONValuePtr cmd = qemuMonitorJSONMakeCommand("query-blockstats", - NULL); - virJSONValuePtr reply = NULL; - virJSONValuePtr devices = NULL; - virJSONValuePtr dev = NULL; - virJSONValuePtr stats = NULL; - - if (!cmd) - return -1; - - ret = qemuMonitorJSONCommand(mon, cmd, &reply); - - if (ret == 0) - ret = qemuMonitorJSONCheckError(cmd, reply); - if (ret < 0) - goto cleanup; - ret = -1; - - devices = virJSONValueObjectGet(reply, "return"); - if (!devices || devices->type != VIR_JSON_TYPE_ARRAY) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("blockstats reply was missing device list")); - goto cleanup; - } - - dev = virJSONValueArrayGet(devices, 0); - - 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 ((stats = virJSONValueObjectGet(dev, "stats")) == NULL || - stats->type != VIR_JSON_TYPE_OBJECT) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("blockstats stats entry was not in expected format")); - goto cleanup; - } - - for (i = 0; i < stats->data.object.npairs; i++) { - const char *key = stats->data.object.pairs[i].key; - - if (STREQ(key, "rd_bytes") || - STREQ(key, "rd_operations") || - STREQ(key, "rd_total_time_ns") || - STREQ(key, "wr_bytes") || - STREQ(key, "wr_operations") || - STREQ(key, "wr_total_time_ns") || - STREQ(key, "flush_operations") || - STREQ(key, "flush_total_time_ns")) { - num++; - } else { - /* wr_highest_offset is parsed by qemuMonitorJSONGetBlockExtent. */ - if (STRNEQ(key, "wr_highest_offset")) - VIR_DEBUG("Missed block stat: %s", key); - } - } - - *nparams = num; - ret = 0; - - cleanup: - virJSONValueFree(cmd); - virJSONValueFree(reply); - return ret; -} - - static int qemuMonitorJSONReportBlockExtentError(qemuMonitorBlockExtentError error) { diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 0fcb0c0..8ceea8a 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -71,24 +71,12 @@ int qemuMonitorJSONSetMemoryStatsPeriod(qemuMonitorPtr mon, int period); int qemuMonitorJSONGetBlockInfo(qemuMonitorPtr mon, virHashTablePtr table); -int qemuMonitorJSONGetBlockStatsInfo(qemuMonitorPtr mon, - const char *dev_name, - long long *rd_req, - long long *rd_bytes, - long long *rd_total_times, - long long *wr_req, - long long *wr_bytes, - long long *wr_total_times, - long long *flush_req, - long long *flush_total_times); int qemuMonitorJSONGetAllBlockStatsInfo(qemuMonitorPtr mon, virHashTablePtr hash, bool backingChain); int qemuMonitorJSONBlockStatsUpdateCapacity(qemuMonitorPtr mon, virHashTablePtr stats, bool backingChain); -int qemuMonitorJSONGetBlockStatsParamsNumber(qemuMonitorPtr mon, - int *nparams); int qemuMonitorJSONGetBlockExtent(qemuMonitorPtr mon, const char *dev_name, unsigned long long *extent); diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index 203859c..9973a17 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -975,204 +975,6 @@ qemuMonitorTextGetAllBlockStatsInfo(qemuMonitorPtr mon, } -int qemuMonitorTextGetBlockStatsInfo(qemuMonitorPtr mon, - const char *dev_name, - long long *rd_req, - long long *rd_bytes, - long long *rd_total_times, - long long *wr_req, - long long *wr_bytes, - long long *wr_total_times, - long long *flush_req, - long long *flush_total_times) -{ - char *info = NULL; - int ret = -1; - char *dummy; - const char *p, *eol; - int devnamelen = strlen(dev_name); - - if (qemuMonitorHMPCommand(mon, "info blockstats", &info) < 0) - goto cleanup; - - /* If the command isn't supported then qemu prints the supported - * info commands, so the output starts "info ". Since this is - * unlikely to be the name of a block device, we can use this - * to detect if qemu supports the command. - */ - if (strstr(info, "\ninfo ")) { - virReportError(VIR_ERR_OPERATION_INVALID, - "%s", - _("'info blockstats' not supported by this qemu")); - goto cleanup; - } - - *rd_req = *rd_bytes = -1; - *wr_req = *wr_bytes = -1; - - if (rd_total_times) - *rd_total_times = -1; - if (wr_total_times) - *wr_total_times = -1; - if (flush_req) - *flush_req = -1; - if (flush_total_times) - *flush_total_times = -1; - - /* The output format for both qemu & KVM is: - * blockdevice: rd_bytes=% wr_bytes=% rd_operations=% wr_operations=% - * (repeated for each block device) - * where '%' is a 64 bit number. - */ - p = info; - - while (*p) { - /* 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(p, QEMU_DRIVE_HOST_PREFIX)) - p += strlen(QEMU_DRIVE_HOST_PREFIX); - - if (STREQLEN(p, dev_name, devnamelen) - && p[devnamelen] == ':' && p[devnamelen+1] == ' ') { - - eol = strchr(p, '\n'); - if (!eol) - eol = p + strlen(p); - - p += devnamelen+2; /* Skip to first label. */ - - while (*p) { - if (STRPREFIX(p, "rd_bytes=")) { - p += strlen("rd_bytes="); - if (virStrToLong_ll(p, &dummy, 10, rd_bytes) == -1) - VIR_DEBUG("error reading rd_bytes: %s", p); - } else if (STRPREFIX(p, "wr_bytes=")) { - p += strlen("wr_bytes="); - if (virStrToLong_ll(p, &dummy, 10, wr_bytes) == -1) - VIR_DEBUG("error reading wr_bytes: %s", p); - } else if (STRPREFIX(p, "rd_operations=")) { - p += strlen("rd_operations="); - if (virStrToLong_ll(p, &dummy, 10, rd_req) == -1) - VIR_DEBUG("error reading rd_req: %s", p); - } else if (STRPREFIX(p, "wr_operations=")) { - p += strlen("wr_operations="); - if (virStrToLong_ll(p, &dummy, 10, wr_req) == -1) - VIR_DEBUG("error reading wr_req: %s", p); - } else if (rd_total_times && - STRPREFIX(p, "rd_total_time_ns=")) { - p += strlen("rd_total_time_ns="); - if (virStrToLong_ll(p, &dummy, 10, rd_total_times) == -1) - VIR_DEBUG("error reading rd_total_times: %s", p); - } else if (wr_total_times && - STRPREFIX(p, "wr_total_time_ns=")) { - p += strlen("wr_total_time_ns="); - if (virStrToLong_ll(p, &dummy, 10, wr_total_times) == -1) - VIR_DEBUG("error reading wr_total_times: %s", p); - } else if (flush_req && - STRPREFIX(p, "flush_operations=")) { - p += strlen("flush_operations="); - if (virStrToLong_ll(p, &dummy, 10, flush_req) == -1) - VIR_DEBUG("error reading flush_req: %s", p); - } else if (flush_total_times && - STRPREFIX(p, "flush_total_time_ns=")) { - p += strlen("flush_total_time_ns="); - if (virStrToLong_ll(p, &dummy, 10, flush_total_times) == -1) - VIR_DEBUG("error reading flush_total_times: %s", p); - } else { - VIR_DEBUG("unknown block stat near %s", p); - } - - /* Skip to next label. */ - p = strchr(p, ' '); - if (!p || p >= eol) break; - p++; - } - ret = 0; - goto cleanup; - } - - /* Skip to next line. */ - p = strchr(p, '\n'); - if (!p) break; - p++; - } - - /* If we reach here then the device was not found. */ - virReportError(VIR_ERR_INVALID_ARG, - _("no stats found for device %s"), dev_name); - - cleanup: - VIR_FREE(info); - return ret; -} - -int qemuMonitorTextGetBlockStatsParamsNumber(qemuMonitorPtr mon, - int *nparams) -{ - char *info = NULL; - int ret = -1; - int num = 0; - const char *p, *eol; - - if (qemuMonitorHMPCommand(mon, "info blockstats", &info) < 0) - goto cleanup; - - /* If the command isn't supported then qemu prints the supported - * info commands, so the output starts "info ". Since this is - * unlikely to be the name of a block device, we can use this - * to detect if qemu supports the command. - */ - if (strstr(info, "\ninfo ")) { - virReportError(VIR_ERR_OPERATION_INVALID, - "%s", - _("'info blockstats' not supported by this qemu")); - goto cleanup; - } - - /* The output format for both qemu & KVM is: - * blockdevice: rd_bytes=% wr_bytes=% rd_operations=% wr_operations=% - * (repeated for each block device) - * where '%' is a 64 bit number. - */ - p = info; - - eol = strchr(p, '\n'); - if (!eol) - eol = p + strlen(p); - - /* Skip the device name and following ":", and spaces (e.g. - * "floppy0: ") - */ - p = strchr(p, ' '); - - while (p && p < eol) { - if (STRPREFIX(p, " rd_bytes=") || - STRPREFIX(p, " wr_bytes=") || - STRPREFIX(p, " rd_operations=") || - STRPREFIX(p, " wr_operations=") || - STRPREFIX(p, " rd_total_time_ns=") || - STRPREFIX(p, " wr_total_time_ns=") || - STRPREFIX(p, " flush_operations=") || - STRPREFIX(p, " flush_total_time_ns=")) { - num++; - } else { - VIR_DEBUG("unknown block stat near %s", p); - } - - /* Skip to next label. */ - p = strchr(p + 1, ' '); - } - - *nparams = num; - ret = 0; - - cleanup: - VIR_FREE(info); - return ret; -} - int qemuMonitorTextGetBlockExtent(qemuMonitorPtr mon ATTRIBUTE_UNUSED, const char *dev_name ATTRIBUTE_UNUSED, unsigned long long *extent ATTRIBUTE_UNUSED) diff --git a/src/qemu/qemu_monitor_text.h b/src/qemu/qemu_monitor_text.h index a1bc2b2..40edc9a 100644 --- a/src/qemu/qemu_monitor_text.h +++ b/src/qemu/qemu_monitor_text.h @@ -63,18 +63,6 @@ int qemuMonitorTextGetBlockInfo(qemuMonitorPtr mon, int qemuMonitorTextGetAllBlockStatsInfo(qemuMonitorPtr mon, virHashTablePtr hash); -int qemuMonitorTextGetBlockStatsInfo(qemuMonitorPtr mon, - const char *dev_name, - long long *rd_req, - long long *rd_bytes, - long long *rd_total_times, - long long *wr_req, - long long *wr_bytes, - long long *wr_total_times, - long long *flush_req, - long long *flush_total_times); -int qemuMonitorTextGetBlockStatsParamsNumber(qemuMonitorPtr mon, - int *nparams); int qemuMonitorTextGetBlockExtent(qemuMonitorPtr mon, const char *dev_name, unsigned long long *extent); -- 2.2.2
On Tue, Mar 10, 2015 at 05:26:29PM +0100, Peter Krempa wrote:
Peter Krempa (12): qemu: Use macro to set block stats typed parameters qemu: monitor: Drop parsing of 'errs' from block info qemu: blockstats: Switch to caller allocated hash table test: qemu: Fix qemu monitor test utils to allow testing HMP qemu: monitor: Implement HMP version for listing all block device stats qemu: monitor: Convert common code to a macro qemu: monitor: Count block stats fields in qemuMonitorGetAllBlockStatsInfo qemu: Split out working code qemuDomainBlockStats qemu: blockstats: Add support for totalled block statistics qemu: blockstats: Refactor qemuDomainBlockStatsFlags test: qemu: json: Avoid using the now obsolete functions qemu: monitor: Kill qemuMonitorGetBlockStats(Info,ParamsNumber)
src/qemu/qemu_driver.c | 299 ++++++++++++++++++++----------------------- src/qemu/qemu_monitor.c | 106 ++++++--------- src/qemu/qemu_monitor.h | 15 --- src/qemu/qemu_monitor_json.c | 233 ++++----------------------------- src/qemu/qemu_monitor_json.h | 15 +-- src/qemu/qemu_monitor_text.c | 256 ++++++++++++++---------------------- src/qemu/qemu_monitor_text.h | 16 +-- tests/Makefile.am | 8 +- tests/qemumonitorjsontest.c | 65 ++++------ tests/qemumonitortest.c | 89 ++++++++++++- tests/qemumonitortestutils.c | 3 +- tools/virsh.pod | 5 +- 12 files changed, 425 insertions(+), 685 deletions(-)
ACK series. Did you just add a feature while removing 260 lines of code? Jan
On Wed, Mar 11, 2015 at 10:05:59 +0100, Ján Tomko wrote:
On Tue, Mar 10, 2015 at 05:26:29PM +0100, Peter Krempa wrote:
Peter Krempa (12): qemu: Use macro to set block stats typed parameters qemu: monitor: Drop parsing of 'errs' from block info qemu: blockstats: Switch to caller allocated hash table test: qemu: Fix qemu monitor test utils to allow testing HMP qemu: monitor: Implement HMP version for listing all block device stats qemu: monitor: Convert common code to a macro qemu: monitor: Count block stats fields in qemuMonitorGetAllBlockStatsInfo qemu: Split out working code qemuDomainBlockStats qemu: blockstats: Add support for totalled block statistics qemu: blockstats: Refactor qemuDomainBlockStatsFlags test: qemu: json: Avoid using the now obsolete functions qemu: monitor: Kill qemuMonitorGetBlockStats(Info,ParamsNumber)
src/qemu/qemu_driver.c | 299 ++++++++++++++++++++----------------------- src/qemu/qemu_monitor.c | 106 ++++++--------- src/qemu/qemu_monitor.h | 15 --- src/qemu/qemu_monitor_json.c | 233 ++++----------------------------- src/qemu/qemu_monitor_json.h | 15 +-- src/qemu/qemu_monitor_text.c | 256 ++++++++++++++---------------------- src/qemu/qemu_monitor_text.h | 16 +-- tests/Makefile.am | 8 +- tests/qemumonitorjsontest.c | 65 ++++------ tests/qemumonitortest.c | 89 ++++++++++++- tests/qemumonitortestutils.c | 3 +- tools/virsh.pod | 5 +- 12 files changed, 425 insertions(+), 685 deletions(-)
ACK series.
Thanks for the review.
Did you just add a feature while removing 260 lines of code?
I'd gladly kill even more code in the text monitor section if I would be allowed to :)
Jan
Peter
On Wed, Mar 11, 2015 at 10:05:59 +0100, Ján Tomko wrote:
On Tue, Mar 10, 2015 at 05:26:29PM +0100, Peter Krempa wrote:
Peter Krempa (12): qemu: Use macro to set block stats typed parameters qemu: monitor: Drop parsing of 'errs' from block info qemu: blockstats: Switch to caller allocated hash table test: qemu: Fix qemu monitor test utils to allow testing HMP qemu: monitor: Implement HMP version for listing all block device stats qemu: monitor: Convert common code to a macro qemu: monitor: Count block stats fields in qemuMonitorGetAllBlockStatsInfo qemu: Split out working code qemuDomainBlockStats qemu: blockstats: Add support for totalled block statistics qemu: blockstats: Refactor qemuDomainBlockStatsFlags test: qemu: json: Avoid using the now obsolete functions qemu: monitor: Kill qemuMonitorGetBlockStats(Info,ParamsNumber)
src/qemu/qemu_driver.c | 299 ++++++++++++++++++++----------------------- src/qemu/qemu_monitor.c | 106 ++++++--------- src/qemu/qemu_monitor.h | 15 --- src/qemu/qemu_monitor_json.c | 233 ++++----------------------------- src/qemu/qemu_monitor_json.h | 15 +-- src/qemu/qemu_monitor_text.c | 256 ++++++++++++++---------------------- src/qemu/qemu_monitor_text.h | 16 +-- tests/Makefile.am | 8 +- tests/qemumonitorjsontest.c | 65 ++++------ tests/qemumonitortest.c | 89 ++++++++++++- tests/qemumonitortestutils.c | 3 +- tools/virsh.pod | 5 +- 12 files changed, 425 insertions(+), 685 deletions(-)
ACK series.
Did you just add a feature while removing 260 lines of code?
I've fixed the problems you've pointed out and pushed the series. Thanks. Peter
participants (2)
-
Ján Tomko -
Peter Krempa