Re: [PATCH 1/6] Refactor job statistics

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

 




On 09/01/2014 11:05 AM, Jiri Denemark wrote:
> Job statistics data were tracked in several structures and variables.
> Let's make a new qemuDomainJobInfo structure which can be used as a
> single source of statistics data as a preparation for storing data about
> completed a job.
> 
> Signed-off-by: Jiri Denemark <jdenemar@xxxxxxxxxx>
> ---
>  src/qemu/qemu_domain.c    | 157 ++++++++++++++++++++++++++++++++++++++++++++--
>  src/qemu/qemu_domain.h    |  24 ++++++-
>  src/qemu/qemu_driver.c    | 119 ++++-------------------------------
>  src/qemu/qemu_migration.c |  72 ++++++++-------------
>  4 files changed, 213 insertions(+), 159 deletions(-)
> 

Curious why the decision to use "Ptr" instead of inline struct for
current & completed?  When I saw an alloc in the middle of a structure
and then a few free's along the way I began to wonder and attempt to
research the various paths to make sure all accesses knew/checked that
the impending deref would be OK - I marked those I found... I think one
or two may need double check since I assume you know the overall code
paths better. I see there's a path from qemuProcessReconnect() - that's
the libvirtd restart path right? So when the VIR_ALLOC() is done for
current/completed - who's address space is that? Wouldn't that be
address space from the "old" libvirtd?

Maybe I'm overthinking it on a Friday :-)

> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index e9506e0..2c709e9 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -163,11 +163,9 @@ qemuDomainObjResetAsyncJob(qemuDomainObjPrivatePtr priv)
>      job->asyncOwner = 0;
>      job->phase = 0;
>      job->mask = DEFAULT_JOB_MASK;
> -    job->start = 0;
>      job->dump_memory_only = false;
>      job->asyncAbort = false;
> -    memset(&job->status, 0, sizeof(job->status));
> -    memset(&job->info, 0, sizeof(job->info));
> +    VIR_FREE(job->current);

This was one of those free paths where there were multiple callers and I
wasn't 100% other accesses needed to be checked w/r/t to this occurring
before an unchecked access in some other path...

>  }
>  
>  void
> @@ -200,6 +198,7 @@ qemuDomainObjTransferJob(virDomainObjPtr obj)
>  static void
>  qemuDomainObjFreeJob(qemuDomainObjPrivatePtr priv)
>  {
> +    VIR_FREE(priv->job.current);
>      virCondDestroy(&priv->job.cond);
>      virCondDestroy(&priv->job.asyncCond);
>  }
> @@ -211,6 +210,150 @@ qemuDomainTrackJob(qemuDomainJob job)
>  }
>  
>  
> +int
> +qemuDomainJobInfoUpdateTime(qemuDomainJobInfoPtr jobInfo)
> +{
> +    unsigned long long now;
> +
> +    if (!jobInfo->started)
> +        return 0;

any chance jobInfo == NULL?  I think the only path where the caller
doesn't check for current (or completed) is qemuMigrationUpdateJobStatus

> +
> +    if (virTimeMillisNow(&now) < 0)
> +        return -1;
> +
> +    jobInfo->timeElapsed = now - jobInfo->started;
> +    return 0;
> +}
> +
> +int
> +qemuDomainJobInfoToInfo(qemuDomainJobInfoPtr jobInfo,
> +                        virDomainJobInfoPtr info)
> +{

Called with job.current checked

> +    info->timeElapsed = jobInfo->timeElapsed;
> +    info->timeRemaining = jobInfo->timeRemaining;
> +
> +    info->memTotal = jobInfo->status.ram_total;
> +    info->memRemaining = jobInfo->status.ram_remaining;
> +    info->memProcessed = jobInfo->status.ram_transferred;
> +
> +    info->fileTotal = jobInfo->status.disk_total;
> +    info->fileRemaining = jobInfo->status.disk_remaining;
> +    info->fileProcessed = jobInfo->status.disk_transferred;
> +
> +    info->dataTotal = info->memTotal + info->fileTotal;
> +    info->dataRemaining = info->memRemaining + info->fileRemaining;
> +    info->dataProcessed = info->memProcessed + info->fileProcessed;
> +
> +    return 0;
> +}
> +
> +int
> +qemuDomainJobInfoToParams(qemuDomainJobInfoPtr jobInfo,
> +                          int *type,
> +                          virTypedParameterPtr *params,
> +                          int *nparams)
> +{

Called with job.current (and .completed) checked...

> +    qemuMonitorMigrationStatus *status = &jobInfo->status;
> +    virTypedParameterPtr par = NULL;
> +    int maxpar = 0;
> +    int npar = 0;
> +
> +    if (virTypedParamsAddULLong(&par, &npar, &maxpar,
> +                                VIR_DOMAIN_JOB_TIME_ELAPSED,
> +                                jobInfo->timeElapsed) < 0)
> +        goto error;
> +
> +    if (jobInfo->type == VIR_DOMAIN_JOB_BOUNDED &&
> +        virTypedParamsAddULLong(&par, &npar, &maxpar,
> +                                VIR_DOMAIN_JOB_TIME_REMAINING,
> +                                jobInfo->timeRemaining) < 0)
> +        goto error;
> +
> +    if (status->downtime_set &&
> +        virTypedParamsAddULLong(&par, &npar, &maxpar,
> +                                VIR_DOMAIN_JOB_DOWNTIME,
> +                                status->downtime) < 0)
> +        goto error;
> +
> +    if (virTypedParamsAddULLong(&par, &npar, &maxpar,
> +                                VIR_DOMAIN_JOB_DATA_TOTAL,
> +                                status->ram_total +
> +                                status->disk_total) < 0 ||
> +        virTypedParamsAddULLong(&par, &npar, &maxpar,
> +                                VIR_DOMAIN_JOB_DATA_PROCESSED,
> +                                status->ram_transferred +
> +                                status->disk_transferred) < 0 ||
> +        virTypedParamsAddULLong(&par, &npar, &maxpar,
> +                                VIR_DOMAIN_JOB_DATA_REMAINING,
> +                                status->ram_remaining +
> +                                status->disk_remaining) < 0)
> +        goto error;
> +
> +    if (virTypedParamsAddULLong(&par, &npar, &maxpar,
> +                                VIR_DOMAIN_JOB_MEMORY_TOTAL,
> +                                status->ram_total) < 0 ||
> +        virTypedParamsAddULLong(&par, &npar, &maxpar,
> +                                VIR_DOMAIN_JOB_MEMORY_PROCESSED,
> +                                status->ram_transferred) < 0 ||
> +        virTypedParamsAddULLong(&par, &npar, &maxpar,
> +                                VIR_DOMAIN_JOB_MEMORY_REMAINING,
> +                                status->ram_remaining) < 0)
> +        goto error;
> +
> +    if (status->ram_duplicate_set) {
> +        if (virTypedParamsAddULLong(&par, &npar, &maxpar,
> +                                    VIR_DOMAIN_JOB_MEMORY_CONSTANT,
> +                                    status->ram_duplicate) < 0 ||
> +            virTypedParamsAddULLong(&par, &npar, &maxpar,
> +                                    VIR_DOMAIN_JOB_MEMORY_NORMAL,
> +                                    status->ram_normal) < 0 ||
> +            virTypedParamsAddULLong(&par, &npar, &maxpar,
> +                                    VIR_DOMAIN_JOB_MEMORY_NORMAL_BYTES,
> +                                    status->ram_normal_bytes) < 0)

I have a note in patch 2 about ram_normal and ram_normal_bytes vis-a-vis
qemu_monitor_json.c not checking status on their fetch and my latest
coverity run complaining about it.

> +            goto error;
> +    }
> +
> +    if (virTypedParamsAddULLong(&par, &npar, &maxpar,
> +                                VIR_DOMAIN_JOB_DISK_TOTAL,
> +                                status->disk_total) < 0 ||
> +        virTypedParamsAddULLong(&par, &npar, &maxpar,
> +                                VIR_DOMAIN_JOB_DISK_PROCESSED,
> +                                status->disk_transferred) < 0 ||
> +        virTypedParamsAddULLong(&par, &npar, &maxpar,
> +                                VIR_DOMAIN_JOB_DISK_REMAINING,
> +                                status->disk_remaining) < 0)
> +        goto error;
> +
> +    if (status->xbzrle_set) {
> +        if (virTypedParamsAddULLong(&par, &npar, &maxpar,
> +                                    VIR_DOMAIN_JOB_COMPRESSION_CACHE,
> +                                    status->xbzrle_cache_size) < 0 ||
> +            virTypedParamsAddULLong(&par, &npar, &maxpar,
> +                                    VIR_DOMAIN_JOB_COMPRESSION_BYTES,
> +                                    status->xbzrle_bytes) < 0 ||
> +            virTypedParamsAddULLong(&par, &npar, &maxpar,
> +                                    VIR_DOMAIN_JOB_COMPRESSION_PAGES,
> +                                    status->xbzrle_pages) < 0 ||
> +            virTypedParamsAddULLong(&par, &npar, &maxpar,
> +                                    VIR_DOMAIN_JOB_COMPRESSION_CACHE_MISSES,
> +                                    status->xbzrle_cache_miss) < 0 ||
> +            virTypedParamsAddULLong(&par, &npar, &maxpar,
> +                                    VIR_DOMAIN_JOB_COMPRESSION_OVERFLOW,
> +                                    status->xbzrle_overflow) < 0)
> +            goto error;
> +    }
> +
> +    *type = jobInfo->type;
> +    *params = par;
> +    *nparams = npar;
> +    return 0;
> +
> + error:
> +    virTypedParamsFree(par, npar);
> +    return -1;
> +}
> +
> +
>  static void *
>  qemuDomainObjPrivateAlloc(void)
>  {
> @@ -1071,7 +1214,7 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver,
>      unsigned long long then;
>      bool nested = job == QEMU_JOB_ASYNC_NESTED;
>      virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
> -    int ret;
> +    int ret = -1;
>  
>      VIR_DEBUG("Starting %s: %s (async=%s vm=%p name=%s)",
>                job == QEMU_JOB_ASYNC ? "async job" : "job",

Just a note after this there's a

    priv->jobs_queued++;

which can be decremented at cleanup; however, if virTimeMillisNow()
fails, then there's a return -1 *and* a virObjectUnref(obj) before the
virObjectRef(obj)...


> @@ -1127,9 +1270,11 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver,
>                    qemuDomainAsyncJobTypeToString(asyncJob),
>                    obj, obj->def->name);
>          qemuDomainObjResetAsyncJob(priv);
> +        if (VIR_ALLOC(priv->job.current) < 0)
> +            goto cleanup;

If this fails - what are the chances some other path will check/access
current without checking for it being NULL?  Perhaps low and there's
going to be other problems anyway in a low memory situation...

>          priv->job.asyncJob = asyncJob;
>          priv->job.asyncOwner = virThreadSelfID();
> -        priv->job.start = now;
> +        priv->job.current->started = now;
>      }
>  
>      if (qemuDomainTrackJob(job))
> @@ -1163,6 +1308,8 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver,
>          virReportSystemError(errno,
>                               "%s", _("cannot acquire job mutex"));
>      }
> +
> + cleanup:
>      priv->jobs_queued--;
>      virObjectUnref(obj);
>      virObjectUnref(cfg);
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index 8736889..119590e 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -100,6 +100,18 @@ typedef enum {
>  } qemuDomainAsyncJob;
>  VIR_ENUM_DECL(qemuDomainAsyncJob)
>  
> +typedef struct _qemuDomainJobInfo qemuDomainJobInfo;
> +typedef qemuDomainJobInfo *qemuDomainJobInfoPtr;
> +struct _qemuDomainJobInfo {
> +    virDomainJobType type;
> +    unsigned long long started; /* When the async job started */
> +    /* Computed values */
> +    unsigned long long timeElapsed;
> +    unsigned long long timeRemaining;
> +    /* Raw values from QEMU */
> +    qemuMonitorMigrationStatus status;
> +};
> +
>  struct qemuDomainJobObj {
>      virCond cond;                       /* Use to coordinate jobs */
>      qemuDomainJob active;               /* Currently running job */
> @@ -110,10 +122,8 @@ struct qemuDomainJobObj {
>      unsigned long long asyncOwner;      /* Thread which set current async job */
>      int phase;                          /* Job phase (mainly for migrations) */
>      unsigned long long mask;            /* Jobs allowed during async job */
> -    unsigned long long start;           /* When the async job started */
>      bool dump_memory_only;              /* use dump-guest-memory to do dump */
> -    qemuMonitorMigrationStatus status;  /* Raw async job progress data */
> -    virDomainJobInfo info;              /* Processed async job progress data */
> +    qemuDomainJobInfoPtr current;       /* async job progress data */
>      bool asyncAbort;                    /* abort of async job requested */
>  };
>  
> @@ -378,4 +388,12 @@ bool qemuDomainDefCheckABIStability(virQEMUDriverPtr driver,
>  bool qemuDomainAgentAvailable(qemuDomainObjPrivatePtr priv,
>                                bool reportError);
>  
> +int qemuDomainJobInfoUpdateTime(qemuDomainJobInfoPtr jobInfo);
> +int qemuDomainJobInfoToInfo(qemuDomainJobInfoPtr jobInfo,
> +                            virDomainJobInfoPtr info);
> +int qemuDomainJobInfoToParams(qemuDomainJobInfoPtr jobInfo,
> +                              int *type,
> +                              virTypedParameterPtr *params,
> +                              int *nparams);
> +

Should any of these new defs need to have/use the ATTRIBUTE_NONNULL()
for params?

>  #endif /* __QEMU_DOMAIN_H__ */
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 239a300..265315d 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -2655,15 +2655,13 @@ qemuDomainGetControlInfo(virDomainPtr dom,
>      if (priv->monError) {
>          info->state = VIR_DOMAIN_CONTROL_ERROR;
>      } else if (priv->job.active) {
> -        if (!priv->monStart) {
> +        if (virTimeMillisNow(&info->stateTime) < 0)
> +            goto cleanup;
> +        if (priv->job.current) {
>              info->state = VIR_DOMAIN_CONTROL_JOB;
> -            if (virTimeMillisNow(&info->stateTime) < 0)
> -                goto cleanup;
> -            info->stateTime -= priv->job.start;
> +            info->stateTime -= priv->job.current->started;
>          } else {
>              info->state = VIR_DOMAIN_CONTROL_OCCUPIED;
> -            if (virTimeMillisNow(&info->stateTime) < 0)
> -                goto cleanup;
>              info->stateTime -= priv->monStart;
>          }
>      } else {
> @@ -3110,8 +3108,7 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver, virDomainPtr dom,
>          goto endjob;
>      }
>  
> -    memset(&priv->job.info, 0, sizeof(priv->job.info));
> -    priv->job.info.type = VIR_DOMAIN_JOB_UNBOUNDED;
> +    priv->job.current->type = VIR_DOMAIN_JOB_UNBOUNDED;

Assumes current-> exists, but occurs after BeginAsyncJob() is
successful, so it seems safe.

>  
>      /* Pause */
>      if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) {
> @@ -3460,6 +3457,7 @@ static int qemuDumpToFd(virQEMUDriverPtr driver, virDomainObjPtr vm,
>                                            fd) < 0)
>          return -1;
>  
> +    VIR_FREE(priv->job.current);
>      priv->job.dump_memory_only = true;
>  
>      if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
> @@ -11616,17 +11614,15 @@ static int qemuDomainGetJobInfo(virDomainPtr dom,
>          goto cleanup;
>  
>      if (virDomainObjIsActive(vm)) {
> -        if (priv->job.asyncJob && !priv->job.dump_memory_only) {
> -            memcpy(info, &priv->job.info, sizeof(*info));
> -
> +        if (priv->job.current) {

Here there is a check for job.current before usage

>              /* Refresh elapsed time again just to ensure it
>               * is fully updated. This is primarily for benefit
>               * of incoming migration which we don't currently
>               * monitor actively in the background thread
>               */
> -            if (virTimeMillisNow(&info->timeElapsed) < 0)
> +            if (qemuDomainJobInfoUpdateTime(priv->job.current) < 0 ||
> +                qemuDomainJobInfoToInfo(priv->job.current, info) < 0)
>                  goto cleanup;
> -            info->timeElapsed -= priv->job.start;
>          } else {
>              memset(info, 0, sizeof(*info));
>              info->type = VIR_DOMAIN_JOB_NONE;
> @@ -11655,9 +11651,6 @@ qemuDomainGetJobStats(virDomainPtr dom,
>  {
>      virDomainObjPtr vm;
>      qemuDomainObjPrivatePtr priv;
> -    virTypedParameterPtr par = NULL;
> -    int maxpar = 0;
> -    int npar = 0;
>      int ret = -1;
>  
>      virCheckFlags(0, -1);
> @@ -11676,7 +11669,7 @@ qemuDomainGetJobStats(virDomainPtr dom,
>          goto cleanup;
>      }
>  
> -    if (!priv->job.asyncJob || priv->job.dump_memory_only) {
> +    if (!priv->job.current) {
>          *type = VIR_DOMAIN_JOB_NONE;
>          *params = NULL;
>          *nparams = 0;

Here there is a check for job.current before usage below

> @@ -11689,102 +11682,16 @@ qemuDomainGetJobStats(virDomainPtr dom,
>       * of incoming migration which we don't currently
>       * monitor actively in the background thread
>       */
> -    if (virTimeMillisNow(&priv->job.info.timeElapsed) < 0)
> +    if (qemuDomainJobInfoUpdateTime(priv->job.current) < 0 ||
> +        qemuDomainJobInfoToParams(priv->job.current,
> +                                  type, params, nparams) < 0)
>          goto cleanup;
> -    priv->job.info.timeElapsed -= priv->job.start;
>  
> -    if (virTypedParamsAddULLong(&par, &npar, &maxpar,
> -                                VIR_DOMAIN_JOB_TIME_ELAPSED,
> -                                priv->job.info.timeElapsed) < 0)
> -        goto cleanup;
> -
> -    if (priv->job.info.type == VIR_DOMAIN_JOB_BOUNDED &&
> -        virTypedParamsAddULLong(&par, &npar, &maxpar,
> -                                VIR_DOMAIN_JOB_TIME_REMAINING,
> -                                priv->job.info.timeRemaining) < 0)
> -        goto cleanup;
> -
> -    if (priv->job.status.downtime_set &&
> -        virTypedParamsAddULLong(&par, &npar, &maxpar,
> -                                VIR_DOMAIN_JOB_DOWNTIME,
> -                                priv->job.status.downtime) < 0)
> -        goto cleanup;
> -
> -    if (virTypedParamsAddULLong(&par, &npar, &maxpar,
> -                                VIR_DOMAIN_JOB_DATA_TOTAL,
> -                                priv->job.info.dataTotal) < 0 ||
> -        virTypedParamsAddULLong(&par, &npar, &maxpar,
> -                                VIR_DOMAIN_JOB_DATA_PROCESSED,
> -                                priv->job.info.dataProcessed) < 0 ||
> -        virTypedParamsAddULLong(&par, &npar, &maxpar,
> -                                VIR_DOMAIN_JOB_DATA_REMAINING,
> -                                priv->job.info.dataRemaining) < 0)
> -        goto cleanup;
> -
> -    if (virTypedParamsAddULLong(&par, &npar, &maxpar,
> -                                VIR_DOMAIN_JOB_MEMORY_TOTAL,
> -                                priv->job.info.memTotal) < 0 ||
> -        virTypedParamsAddULLong(&par, &npar, &maxpar,
> -                                VIR_DOMAIN_JOB_MEMORY_PROCESSED,
> -                                priv->job.info.memProcessed) < 0 ||
> -        virTypedParamsAddULLong(&par, &npar, &maxpar,
> -                                VIR_DOMAIN_JOB_MEMORY_REMAINING,
> -                                priv->job.info.memRemaining) < 0)
> -        goto cleanup;
> -
> -    if (priv->job.status.ram_duplicate_set) {
> -        if (virTypedParamsAddULLong(&par, &npar, &maxpar,
> -                                    VIR_DOMAIN_JOB_MEMORY_CONSTANT,
> -                                    priv->job.status.ram_duplicate) < 0 ||
> -            virTypedParamsAddULLong(&par, &npar, &maxpar,
> -                                    VIR_DOMAIN_JOB_MEMORY_NORMAL,
> -                                    priv->job.status.ram_normal) < 0 ||
> -            virTypedParamsAddULLong(&par, &npar, &maxpar,
> -                                    VIR_DOMAIN_JOB_MEMORY_NORMAL_BYTES,
> -                                    priv->job.status.ram_normal_bytes) < 0)
> -            goto cleanup;
> -    }
> -
> -    if (virTypedParamsAddULLong(&par, &npar, &maxpar,
> -                                VIR_DOMAIN_JOB_DISK_TOTAL,
> -                                priv->job.info.fileTotal) < 0 ||
> -        virTypedParamsAddULLong(&par, &npar, &maxpar,
> -                                VIR_DOMAIN_JOB_DISK_PROCESSED,
> -                                priv->job.info.fileProcessed) < 0 ||
> -        virTypedParamsAddULLong(&par, &npar, &maxpar,
> -                                VIR_DOMAIN_JOB_DISK_REMAINING,
> -                                priv->job.info.fileRemaining) < 0)
> -        goto cleanup;
> -
> -    if (priv->job.status.xbzrle_set) {
> -        if (virTypedParamsAddULLong(&par, &npar, &maxpar,
> -                                    VIR_DOMAIN_JOB_COMPRESSION_CACHE,
> -                                    priv->job.status.xbzrle_cache_size) < 0 ||
> -            virTypedParamsAddULLong(&par, &npar, &maxpar,
> -                                    VIR_DOMAIN_JOB_COMPRESSION_BYTES,
> -                                    priv->job.status.xbzrle_bytes) < 0 ||
> -            virTypedParamsAddULLong(&par, &npar, &maxpar,
> -                                    VIR_DOMAIN_JOB_COMPRESSION_PAGES,
> -                                    priv->job.status.xbzrle_pages) < 0 ||
> -            virTypedParamsAddULLong(&par, &npar, &maxpar,
> -                                    VIR_DOMAIN_JOB_COMPRESSION_CACHE_MISSES,
> -                                    priv->job.status.xbzrle_cache_miss) < 0 ||
> -            virTypedParamsAddULLong(&par, &npar, &maxpar,
> -                                    VIR_DOMAIN_JOB_COMPRESSION_OVERFLOW,
> -                                    priv->job.status.xbzrle_overflow) < 0)
> -            goto cleanup;
> -    }
> -
> -    *type = priv->job.info.type;
> -    *params = par;
> -    *nparams = npar;
>      ret = 0;
>  
>   cleanup:
>      if (vm)
>          virObjectUnlock(vm);
> -    if (ret < 0)
> -        virTypedParamsFree(par, npar);
>      return ret;
>  }
>  
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 9cfb77e..64adab4 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -1723,8 +1723,9 @@ qemuMigrationUpdateJobStatus(virQEMUDriverPtr driver,
>                               qemuDomainAsyncJob asyncJob)
>  {
>      qemuDomainObjPrivatePtr priv = vm->privateData;
> -    int ret;
>      qemuMonitorMigrationStatus status;
> +    qemuDomainJobInfoPtr jobInfo;
> +    int ret;
>  
>      memset(&status, 0, sizeof(status));
>  
> @@ -1738,62 +1739,40 @@ qemuMigrationUpdateJobStatus(virQEMUDriverPtr driver,
>  
>      qemuDomainObjExitMonitor(driver, vm);
>  
> -    priv->job.status = status;
> -
> -    if (ret < 0 || virTimeMillisNow(&priv->job.info.timeElapsed) < 0)
> +    if (ret < 0 ||
> +        qemuDomainJobInfoUpdateTime(priv->job.current) < 0)

Here (and below) it's assumed job.current is valid.

>          return -1;
>  
> -    priv->job.info.timeElapsed -= priv->job.start;
> -
>      ret = -1;
> -    switch (priv->job.status.status) {
> -    case QEMU_MONITOR_MIGRATION_STATUS_INACTIVE:
> -        priv->job.info.type = VIR_DOMAIN_JOB_NONE;
> -        virReportError(VIR_ERR_OPERATION_FAILED,
> -                       _("%s: %s"), job, _("is not active"));
> -        break;
> -
> +    jobInfo = priv->job.current;
> +    switch (status.status) {
> +    case QEMU_MONITOR_MIGRATION_STATUS_COMPLETED:
> +        jobInfo->type = VIR_DOMAIN_JOB_COMPLETED;
> +        /* fall through */
>      case QEMU_MONITOR_MIGRATION_STATUS_SETUP:
> -        ret = 0;
> -        break;
> -
>      case QEMU_MONITOR_MIGRATION_STATUS_ACTIVE:
> -        priv->job.info.fileTotal = priv->job.status.disk_total;
> -        priv->job.info.fileRemaining = priv->job.status.disk_remaining;
> -        priv->job.info.fileProcessed = priv->job.status.disk_transferred;
> -
> -        priv->job.info.memTotal = priv->job.status.ram_total;
> -        priv->job.info.memRemaining = priv->job.status.ram_remaining;
> -        priv->job.info.memProcessed = priv->job.status.ram_transferred;
> -
> -        priv->job.info.dataTotal =
> -            priv->job.status.ram_total + priv->job.status.disk_total;
> -        priv->job.info.dataRemaining =
> -            priv->job.status.ram_remaining + priv->job.status.disk_remaining;
> -        priv->job.info.dataProcessed =
> -            priv->job.status.ram_transferred +
> -            priv->job.status.disk_transferred;
> -
>          ret = 0;
>          break;
>  
> -    case QEMU_MONITOR_MIGRATION_STATUS_COMPLETED:
> -        priv->job.info.type = VIR_DOMAIN_JOB_COMPLETED;
> -        ret = 0;
> +    case QEMU_MONITOR_MIGRATION_STATUS_INACTIVE:
> +        jobInfo->type = VIR_DOMAIN_JOB_NONE;
> +        virReportError(VIR_ERR_OPERATION_FAILED,
> +                       _("%s: %s"), job, _("is not active"));
>          break;
>  
>      case QEMU_MONITOR_MIGRATION_STATUS_ERROR:
> -        priv->job.info.type = VIR_DOMAIN_JOB_FAILED;
> +        jobInfo->type = VIR_DOMAIN_JOB_FAILED;
>          virReportError(VIR_ERR_OPERATION_FAILED,
>                         _("%s: %s"), job, _("unexpectedly failed"));
>          break;
>  
>      case QEMU_MONITOR_MIGRATION_STATUS_CANCELLED:
> -        priv->job.info.type = VIR_DOMAIN_JOB_CANCELLED;
> +        jobInfo->type = VIR_DOMAIN_JOB_CANCELLED;
>          virReportError(VIR_ERR_OPERATION_ABORTED,
>                         _("%s: %s"), job, _("canceled by client"));
>          break;
>      }
> +    jobInfo->status = status;
>  
>      return ret;
>  }
> @@ -1803,11 +1782,14 @@ qemuMigrationUpdateJobStatus(virQEMUDriverPtr driver,
>   * QEMU reports failed migration.
>   */
>  static int
> -qemuMigrationWaitForCompletion(virQEMUDriverPtr driver, virDomainObjPtr vm,
> +qemuMigrationWaitForCompletion(virQEMUDriverPtr driver,
> +                               virDomainObjPtr vm,
>                                 qemuDomainAsyncJob asyncJob,
> -                               virConnectPtr dconn, bool abort_on_error)
> +                               virConnectPtr dconn,
> +                               bool abort_on_error)
>  {
>      qemuDomainObjPrivatePtr priv = vm->privateData;
> +    qemuDomainJobInfoPtr jobInfo = priv->job.current;

Hopefully nothing has cleared or could clear this before this point...

>      const char *job;
>      int pauseReason;
>  
> @@ -1825,9 +1807,9 @@ qemuMigrationWaitForCompletion(virQEMUDriverPtr driver, virDomainObjPtr vm,
>          job = _("job");
>      }
>  
> -    priv->job.info.type = VIR_DOMAIN_JOB_UNBOUNDED;
> +    jobInfo->type = VIR_DOMAIN_JOB_UNBOUNDED;
>  
> -    while (priv->job.info.type == VIR_DOMAIN_JOB_UNBOUNDED) {
> +    while (jobInfo->type == VIR_DOMAIN_JOB_UNBOUNDED) {
>          /* Poll every 50ms for progress & to allow cancellation */
>          struct timespec ts = { .tv_sec = 0, .tv_nsec = 50 * 1000 * 1000ull };
>  
> @@ -1856,13 +1838,13 @@ qemuMigrationWaitForCompletion(virQEMUDriverPtr driver, virDomainObjPtr vm,
>          virObjectLock(vm);
>      }
>  
> -    if (priv->job.info.type == VIR_DOMAIN_JOB_COMPLETED) {
> +    if (jobInfo->type == VIR_DOMAIN_JOB_COMPLETED) {
>          return 0;
> -    } else if (priv->job.info.type == VIR_DOMAIN_JOB_UNBOUNDED) {
> +    } else if (jobInfo->type == VIR_DOMAIN_JOB_UNBOUNDED) {
>          /* The migration was aborted by us rather than QEMU itself so let's
>           * update the job type and notify the caller to send migrate_cancel.
>           */
> -        priv->job.info.type = VIR_DOMAIN_JOB_FAILED;
> +        jobInfo->type = VIR_DOMAIN_JOB_FAILED;
>          return -2;
>      } else {
>          return -1;
> @@ -4912,7 +4894,7 @@ qemuMigrationJobStart(virQEMUDriverPtr driver,
>                                       JOB_MASK(QEMU_JOB_MIGRATION_OP));
>      }
>  
> -    priv->job.info.type = VIR_DOMAIN_JOB_UNBOUNDED;
> +    priv->job.current->type = VIR_DOMAIN_JOB_UNBOUNDED;

This assumption of current-> should be OK since a failure from
qemuDomainObjBeginAsyncJob() wouldn't go through here.

>  
>      return 0;
>  }
> 

In general - I'm AOK with the code - my main concern has to do with
alloc'd memory...

John

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