Re: [Qemu-devel] [PATCH v2 03/16] job: Add error message for failing jobs

[Jeff Cody <jcody@redhat.com>]

On Tue, May 29, 2018 at 10:38:57PM +0200, Kevin Wolf wrote:
> So far we relied on job->ret and strerror() to produce an error message
> for failed jobs. Not surprisingly, this tends to result in completely
> useless messages.
> 
> This adds a Job.error field that can contain an error string for a
> failing job, and a parameter to job_completed() that sets the field. As
> a default, if NULL is passed, we continue to use strerror(job->ret).
> 
> All existing callers are changed to pass NULL. They can be improved in
> separate patches.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/qemu/job.h        |  7 ++++++-
>  block/backup.c            |  2 +-
>  block/commit.c            |  2 +-
>  block/mirror.c            |  2 +-
>  block/stream.c            |  2 +-
>  job-qmp.c                 |  9 ++-------
>  job.c                     | 16 ++++++++++++++--
>  tests/test-bdrv-drain.c   |  2 +-
>  tests/test-blockjob-txn.c |  2 +-
>  tests/test-blockjob.c     |  2 +-
>  10 files changed, 29 insertions(+), 17 deletions(-)
> 
> diff --git a/include/qemu/job.h b/include/qemu/job.h
> index 8c8badf75e..1d820530fa 100644
> --- a/include/qemu/job.h
> +++ b/include/qemu/job.h
> @@ -124,6 +124,9 @@ typedef struct Job {
>      /** Estimated progress_current value at the completion of the job */
>      int64_t progress_total;
>  
> +    /** Error string for a failed job (NULL if, and only if, job->ret == 0) */
> +    char *error;
> +
>      /** ret code passed to job_completed. */
>      int ret;
>  
> @@ -466,13 +469,15 @@ void job_transition_to_ready(Job *job);
>  /**
>   * @job: The job being completed.
>   * @ret: The status code.
> + * @error: The error message for a failing job (only with @ret < 0). If @ret is
> + *         negative, but NULL is given for @error, strerror() is used.
>   *
>   * Marks @job as completed. If @ret is non-zero, the job transaction it is part
>   * of is aborted. If @ret is zero, the job moves into the WAITING state. If it
>   * is the last job to complete in its transaction, all jobs in the transaction
>   * move from WAITING to PENDING.
>   */
> -void job_completed(Job *job, int ret);
> +void job_completed(Job *job, int ret, Error *error);
>  
>  /** Asynchronously complete the specified @job. */
>  void job_complete(Job *job, Error **errp);
> diff --git a/block/backup.c b/block/backup.c
> index 4e228e959b..5661435675 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -321,7 +321,7 @@ static void backup_complete(Job *job, void *opaque)
>  {
>      BackupCompleteData *data = opaque;
>  
> -    job_completed(job, data->ret);
> +    job_completed(job, data->ret, NULL);
>      g_free(data);
>  }
>  
> diff --git a/block/commit.c b/block/commit.c
> index 620666161b..e1814d9693 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -117,7 +117,7 @@ static void commit_complete(Job *job, void *opaque)
>       * bdrv_set_backing_hd() to fail. */
>      block_job_remove_all_bdrv(bjob);
>  
> -    job_completed(job, ret);
> +    job_completed(job, ret, NULL);
>      g_free(data);
>  
>      /* If bdrv_drop_intermediate() didn't already do that, remove the commit
> diff --git a/block/mirror.c b/block/mirror.c
> index dcb66ec3be..435268bbbf 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -581,7 +581,7 @@ static void mirror_exit(Job *job, void *opaque)
>      blk_set_perm(bjob->blk, 0, BLK_PERM_ALL, &error_abort);
>      blk_insert_bs(bjob->blk, mirror_top_bs, &error_abort);
>  
> -    job_completed(job, data->ret);
> +    job_completed(job, data->ret, NULL);
>  
>      g_free(data);
>      bdrv_drained_end(src);
> diff --git a/block/stream.c b/block/stream.c
> index a5d6e0cf8a..9264b68a1e 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -93,7 +93,7 @@ out:
>      }
>  
>      g_free(s->backing_file_str);
> -    job_completed(job, data->ret);
> +    job_completed(job, data->ret, NULL);
>      g_free(data);
>  }
>  
> diff --git a/job-qmp.c b/job-qmp.c
> index 7f38f63336..410775df61 100644
> --- a/job-qmp.c
> +++ b/job-qmp.c
> @@ -136,14 +136,9 @@ void qmp_job_dismiss(const char *id, Error **errp)
>  static JobInfo *job_query_single(Job *job, Error **errp)
>  {
>      JobInfo *info;
> -    const char *errmsg = NULL;
>  
>      assert(!job_is_internal(job));
>  
> -    if (job->ret < 0) {
> -        errmsg = strerror(-job->ret);
> -    }
> -
>      info = g_new(JobInfo, 1);
>      *info = (JobInfo) {
>          .id                 = g_strdup(job->id),
> @@ -151,8 +146,8 @@ static JobInfo *job_query_single(Job *job, Error **errp)
>          .status             = job->status,
>          .current_progress   = job->progress_current,
>          .total_progress     = job->progress_total,
> -        .has_error          = !!errmsg,
> -        .error              = g_strdup(errmsg),
> +        .has_error          = !!job->error,
> +        .error              = g_strdup(job->error),
>      };
>  
>      return info;
> diff --git a/job.c b/job.c
> index f026661b0f..84e140238b 100644
> --- a/job.c
> +++ b/job.c
> @@ -369,6 +369,7 @@ void job_unref(Job *job)
>  
>          QLIST_REMOVE(job, job_list);
>  
> +        g_free(job->error);
>          g_free(job->id);
>          g_free(job);
>      }
> @@ -660,6 +661,9 @@ static void job_update_rc(Job *job)
>          job->ret = -ECANCELED;
>      }
>      if (job->ret) {
> +        if (!job->error) {
> +            job->error = g_strdup(strerror(-job->ret));
> +        }
>          job_state_transition(job, JOB_STATUS_ABORTING);
>      }
>  }
> @@ -782,6 +786,7 @@ static int job_prepare(Job *job)
>  {
>      if (job->ret == 0 && job->driver->prepare) {
>          job->ret = job->driver->prepare(job);
> +        job_update_rc(job);
>      }
>      return job->ret;
>  }
> @@ -855,10 +860,17 @@ static void job_completed_txn_success(Job *job)
>      }
>  }
>  
> -void job_completed(Job *job, int ret)
> +void job_completed(Job *job, int ret, Error *error)
>  {
>      assert(job && job->txn && !job_is_completed(job));

Is it worth it to add '&& job->ret <=0' to the compound assert above?  As
you said, job->ret being positive is most likely a bug (we treat it like an
error return, and abort on it, though).  With the patch as-is, we will only
catch that when 'error' is set... although, I guess maybe that is by design,
since currently nothing sets 'error', we avoid any potential false-positive
asserts.

Either way:

Reviewed-by: Jeff Cody <jcody@redhat.com>



> +
>      job->ret = ret;
> +    if (error) {
> +        assert(job->ret < 0);
> +        job->error = g_strdup(error_get_pretty(error));
> +        error_free(error);
> +    }
> +
>      job_update_rc(job);
>      trace_job_completed(job, ret, job->ret);
>      if (job->ret) {
> @@ -876,7 +888,7 @@ void job_cancel(Job *job, bool force)
>      }
>      job_cancel_async(job, force);
>      if (!job_started(job)) {
> -        job_completed(job, -ECANCELED);
> +        job_completed(job, -ECANCELED, NULL);
>      } else if (job->deferred_to_main_loop) {
>          job_completed_txn_abort(job);
>      } else {
> diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c
> index 2cba63b881..a11c4cfbf2 100644
> --- a/tests/test-bdrv-drain.c
> +++ b/tests/test-bdrv-drain.c
> @@ -498,7 +498,7 @@ typedef struct TestBlockJob {
>  
>  static void test_job_completed(Job *job, void *opaque)
>  {
> -    job_completed(job, 0);
> +    job_completed(job, 0, NULL);
>  }
>  
>  static void coroutine_fn test_job_start(void *opaque)
> diff --git a/tests/test-blockjob-txn.c b/tests/test-blockjob-txn.c
> index fce836639a..58d9b87fb2 100644
> --- a/tests/test-blockjob-txn.c
> +++ b/tests/test-blockjob-txn.c
> @@ -34,7 +34,7 @@ static void test_block_job_complete(Job *job, void *opaque)
>          rc = -ECANCELED;
>      }
>  
> -    job_completed(job, rc);
> +    job_completed(job, rc, NULL);
>      bdrv_unref(bs);
>  }
>  
> diff --git a/tests/test-blockjob.c b/tests/test-blockjob.c
> index e408d52351..cb42f06e61 100644
> --- a/tests/test-blockjob.c
> +++ b/tests/test-blockjob.c
> @@ -167,7 +167,7 @@ static void cancel_job_completed(Job *job, void *opaque)
>  {
>      CancelJob *s = opaque;
>      s->completed = true;
> -    job_completed(job, 0);
> +    job_completed(job, 0, NULL);
>  }
>  
>  static void cancel_job_complete(Job *job, Error **errp)
> -- 
> 2.13.6
>