Re: [Qemu-block] [PATCH v2 03/16] job: Add error message for failing jobs
On 2018-05-29 22:38, 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 > --- > 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(-) Reviewed-by: Max Reitz signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH v2 03/16] job: Add error message for failing jobs
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
> ---
> 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, _abort);
> blk_insert_bs(bjob->blk, mirror_top_bs, _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
[Qemu-block] [PATCH v2 03/16] 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
---
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, _abort);
blk_insert_bs(bjob->blk, mirror_top_bs, _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) {
+
