job: Add error message for failing jobs

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 <[email protected]>
Reviewed-by: Max Reitz <[email protected]>
Reviewed-by: Jeff Cody <[email protected]>

Author: kevmw

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);
 }
 

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

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);

block/stream.c

@@ -93,7 +93,7 @@ static void stream_complete(Job *job, void *opaque)
     }
 
     g_free(s->backing_file_str);
-    job_completed(job, data->ret);
+    job_completed(job, data->ret, NULL);
     g_free(data);
 }
 

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);

job-qmp.c

@@ -136,23 +136,18 @@ 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),
         .type               = job_type(job),
         .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;

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));
+
     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 {

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)

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);
 }
 

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)