[libvirt] [PATCH 2/6] Add support for fetching statistics of completed jobs

[Jiri Denemark]

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 at redhat.com>
> > ---
> >  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