Re: [libvirt] [PATCH 3/5] qemu: monitor: Open-code retrieval of wr_highest_offset

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