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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

Attachment: signature.asc
Description: Digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]