On Fri, Sep 05, 2014 at 14:42:00 -0400, John Ferlan wrote:
>
>
> On 09/01/2014 11:05 AM, Jiri Denemark wrote:
> > virDomainGetJobStats gains new VIR_DOMAIN_JOB_STATS_COMPLETED flag that
> > can be used to fetch statistics of a completed job rather than a
> > currently running job.
> >
> > Signed-off-by: Jiri Denemark <jdenemar@xxxxxxxxxx>
> > ---
> > include/libvirt/libvirt.h.in | 11 +++++++++++
> > src/libvirt.c | 8 +++++++-
> > src/qemu/qemu_domain.c | 1 +
> > src/qemu/qemu_domain.h | 1 +
> > src/qemu/qemu_driver.c | 25 +++++++++++++++++++------
> > src/qemu/qemu_migration.c | 6 ++++++
> > src/qemu/qemu_monitor_json.c | 3 ++-
> > 7 files changed, 47 insertions(+), 8 deletions(-)
> >
>
> This seems AOK - see my note at the end about qemu_monitor_json.c though.
>
>
...
> > diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> > index 62e7d5d..263fa8b 100644
> > --- a/src/qemu/qemu_monitor_json.c
> > +++ b/src/qemu/qemu_monitor_json.c
> > @@ -2478,7 +2478,8 @@ qemuMonitorJSONGetMigrationStatusReply(virJSONValuePtr reply,
> > if (rc == 0)
> > status->downtime_set = true;
> >
> > - if (status->status == QEMU_MONITOR_MIGRATION_STATUS_ACTIVE) {
> > + if (status->status == QEMU_MONITOR_MIGRATION_STATUS_ACTIVE ||
> > + status->status == QEMU_MONITOR_MIGRATION_STATUS_COMPLETED) {
> > virJSONValuePtr ram = virJSONValueObjectGet(ret, "ram");
> > if (!ram) {
> > virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> >
>
> While you're in this module - the following 3 calls don't check status
> (and were flagged by my new coverity):
>
> virJSONValueObjectGetNumberUlong(ret, "total-time", &status->total_time);
>
> virJSONValueObjectGetNumberUlong(ram, "normal", &status->ram_normal);
>
> virJSONValueObjectGetNumberUlong(ram, "normal-bytes",
> &status->ram_normal_bytes);
QEMU does not always report all values so we just ignore failures to get
some of them. I'll look at them and use ignore_value() when appropriate.
> I know "somewhat" outside the bounds of these changes but since they're
> in qemuMonitorJSONGetMigrationStatusReply() and used by the changes here
> - there's enough of a reason to adjust that I think.
>
> I don't see where "total_time" is ever used though...
Right, we compute total time ourselves. This is just for completeness to
parse all fields QEMU supports in case we want to use them sometime
somewhere :-)
Jirka
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list