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

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(a)redhat.com&gt; > --- > 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);

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...