Jobs presently use both an Error object in the case of the create job,
and char strings in the case of generic errors elsewhere.
Unify the two paths as just j->err, and remove the extra argument from
job_completed.
Signed-off-by: John Snow <jsnow@redhat.com>
---
block/backup.c | 2 +-
block/commit.c | 2 +-
block/create.c | 5 ++---
block/mirror.c | 2 +-
block/stream.c | 2 +-
include/qemu/job.h | 10 ++++------
job-qmp.c | 5 +++--
job.c | 17 +++++------------
tests/test-bdrv-drain.c | 2 +-
tests/test-blockjob-txn.c | 2 +-
tests/test-blockjob.c | 2 +-
11 files changed, 21 insertions(+), 30 deletions(-)
diff --git a/block/backup.c b/block/backup.c
index 8630d32926..f3bf842423 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -388,7 +388,7 @@ static void backup_complete(Job *job, void *opaque)
{
BackupCompleteData *data = opaque;
- job_completed(job, data->ret, NULL);
+ job_completed(job, data->ret);
g_free(data);
}
diff --git a/block/commit.c b/block/commit.c
index e1814d9693..620666161b 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, NULL);
+ job_completed(job, ret);
g_free(data);
/* If bdrv_drop_intermediate() didn't already do that, remove the commit
diff --git a/block/create.c b/block/create.c
index 915cd41bcc..84bc74b7de 100644
--- a/block/create.c
+++ b/block/create.c
@@ -35,14 +35,13 @@ typedef struct BlockdevCreateJob {
BlockDriver *drv;
BlockdevCreateOptions *opts;
int ret;
- Error *err;
} BlockdevCreateJob;
static void blockdev_create_complete(Job *job, void *opaque)
{
BlockdevCreateJob *s = container_of(job, BlockdevCreateJob, common);
- job_completed(job, s->ret, s->err);
+ job_completed(job, s->ret);
}
static void coroutine_fn blockdev_create_run(void *opaque)
@@ -50,7 +49,7 @@ static void coroutine_fn blockdev_create_run(void *opaque)
BlockdevCreateJob *s = opaque;
job_progress_set_remaining(&s->common, 1);
- s->ret = s->drv->bdrv_co_create(s->opts, &s->err);
+ s->ret = s->drv->bdrv_co_create(s->opts, &s->common.err);
job_progress_update(&s->common, 1);
qapi_free_BlockdevCreateOptions(s->opts);
diff --git a/block/mirror.c b/block/mirror.c
index b48c3f8cf5..7c2c6ba67e 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -710,7 +710,7 @@ static void mirror_exit(Job *job, void *opaque)
blk_insert_bs(bjob->blk, mirror_top_bs, &error_abort);
bs_opaque->job = NULL;
- job_completed(job, data->ret, NULL);
+ job_completed(job, data->ret);
g_free(data);
bdrv_drained_end(src);
diff --git a/block/stream.c b/block/stream.c
index 9264b68a1e..a5d6e0cf8a 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, NULL);
+ job_completed(job, data->ret);
g_free(data);
}
diff --git a/include/qemu/job.h b/include/qemu/job.h
index 18c9223e31..845ad00c03 100644
--- a/include/qemu/job.h
+++ b/include/qemu/job.h
@@ -124,12 +124,12 @@ 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;
+ /** Error object for a failed job **/
+ Error *err;
+
/** The completion function that will be called when the job completes. */
BlockCompletionFunc *cb;
@@ -484,15 +484,13 @@ 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, Error *error);
+void job_completed(Job *job, int ret);
/** Asynchronously complete the specified @job. */
void job_complete(Job *job, Error **errp);
diff --git a/job-qmp.c b/job-qmp.c
index 410775df61..a969b2bbf0 100644
--- a/job-qmp.c
+++ b/job-qmp.c
@@ -146,8 +146,9 @@ static JobInfo *job_query_single(Job *job, Error **errp)
.status = job->status,
.current_progress = job->progress_current,
.total_progress = job->progress_total,
- .has_error = !!job->error,
- .error = g_strdup(job->error),
+ .has_error = !!job->err,
+ .error = job->err ? \
+ g_strdup(error_get_pretty(job->err)) : NULL,
};
return info;
diff --git a/job.c b/job.c
index fa671b431a..b281f30375 100644
--- a/job.c
+++ b/job.c
@@ -369,7 +369,7 @@ void job_unref(Job *job)
QLIST_REMOVE(job, job_list);
- g_free(job->error);
+ error_free(job->err);
g_free(job->id);
g_free(job);
}
@@ -535,7 +535,6 @@ void job_drain(Job *job)
}
}
-
/**
* All jobs must allow a pause point before entering their job proper. This
* ensures that jobs can be paused prior to being started, then resumed later.
@@ -666,8 +665,8 @@ static void job_update_rc(Job *job)
job->ret = -ECANCELED;
}
if (job->ret) {
- if (!job->error) {
- job->error = g_strdup(strerror(-job->ret));
+ if (!job->err) {
+ error_setg_errno(&job->err, -job->ret, "job failed");
}
job_state_transition(job, JOB_STATUS_ABORTING);
}
@@ -865,17 +864,11 @@ static void job_completed_txn_success(Job *job)
}
}
-void job_completed(Job *job, int ret, Error *error)
+void job_completed(Job *job, int ret)
{
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) {
@@ -893,7 +886,7 @@ void job_cancel(Job *job, bool force)
}
job_cancel_async(job, force);
if (!job_started(job)) {
- job_completed(job, -ECANCELED, NULL);
+ job_completed(job, -ECANCELED);
} 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 17bb8508ae..7b05082cae 100644
--- a/tests/test-bdrv-drain.c
+++ b/tests/test-bdrv-drain.c
@@ -754,7 +754,7 @@ typedef struct TestBlockJob {
static void test_job_completed(Job *job, void *opaque)
{
- job_completed(job, 0, NULL);
+ job_completed(job, 0);
}
static void coroutine_fn test_job_start(void *opaque)
diff --git a/tests/test-blockjob-txn.c b/tests/test-blockjob-txn.c
index 58d9b87fb2..fce836639a 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, NULL);
+ job_completed(job, rc);
bdrv_unref(bs);
}
diff --git a/tests/test-blockjob.c b/tests/test-blockjob.c
index cb42f06e61..e408d52351 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, NULL);
+ job_completed(job, 0);
}
static void cancel_job_complete(Job *job, Error **errp)
--
2.14.4
Am 07.08.2018 um 06:33 hat John Snow geschrieben:
> Jobs presently use both an Error object in the case of the create job,
> and char strings in the case of generic errors elsewhere.
>
> Unify the two paths as just j->err, and remove the extra argument from
> job_completed.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
Hm, not sure. Overall, this feels like a step backwards.
> diff --git a/include/qemu/job.h b/include/qemu/job.h
> index 18c9223e31..845ad00c03 100644
> --- a/include/qemu/job.h
> +++ b/include/qemu/job.h
> @@ -124,12 +124,12 @@ 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;
>
> + /** Error object for a failed job **/
> + Error *err;
> +
> /** The completion function that will be called when the job completes. */
> BlockCompletionFunc *cb;
This is the change that I could agree with, though I don't think it
makes a big difference: Whether you store the string immediately or an
Error object from which you get the string later, doesn't really make a
big difference.
Maybe we find more uses and having an Error object is common practice in
QEMU, so no objections to this change.
> @@ -484,15 +484,13 @@ 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, Error *error);
> +void job_completed(Job *job, int ret);
I don't like this one, though.
Before this change, job_completed(..., NULL) was a clear sign that the
error message probably needed an improvement, because an errno string
doesn't usually describe error situations very well. We may not have a
much better message in some cases, but in most cases we just don't pass
one because an error message after job creation is still a quite new
thing in the QAPI schema.
What we should get rid of in the long term is the int ret, not the Error
*error. I suspect callers really just distinguish success/error without
actually looking at the error code.
With this change applied, what's your new conversion plan for making
sure that every failing caller of job_completed() has set job->error
first?
> @@ -666,8 +665,8 @@ static void job_update_rc(Job *job)
> job->ret = -ECANCELED;
> }
> if (job->ret) {
> - if (!job->error) {
> - job->error = g_strdup(strerror(-job->ret));
> + if (!job->err) {
> + error_setg_errno(&job->err, -job->ret, "job failed");
> }
> job_state_transition(job, JOB_STATUS_ABORTING);
> }
This hunk just makes the error message more verbose with a "job failed"
prefix that doesn't add information. If it's the error string for a job,
of course the job failed.
Kevin
On 08/08/2018 10:57 AM, Kevin Wolf wrote:
> Am 07.08.2018 um 06:33 hat John Snow geschrieben:
>> Jobs presently use both an Error object in the case of the create job,
>> and char strings in the case of generic errors elsewhere.
>>
>> Unify the two paths as just j->err, and remove the extra argument from
>> job_completed.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>
> Hm, not sure. Overall, this feels like a step backwards.
>
>> diff --git a/include/qemu/job.h b/include/qemu/job.h
>> index 18c9223e31..845ad00c03 100644
>> --- a/include/qemu/job.h
>> +++ b/include/qemu/job.h
>> @@ -124,12 +124,12 @@ 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;
>>
>> + /** Error object for a failed job **/
>> + Error *err;
>> +
>> /** The completion function that will be called when the job completes. */
>> BlockCompletionFunc *cb;
>
> This is the change that I could agree with, though I don't think it
> makes a big difference: Whether you store the string immediately or an
> Error object from which you get the string later, doesn't really make a
> big difference.
>
> Maybe we find more uses and having an Error object is common practice in
> QEMU, so no objections to this change.
>
>> @@ -484,15 +484,13 @@ 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, Error *error);
>> +void job_completed(Job *job, int ret);
>
> I don't like this one, though.
>
> Before this change, job_completed(..., NULL) was a clear sign that the
> error message probably needed an improvement, because an errno string
> doesn't usually describe error situations very well. We may not have a
> much better message in some cases, but in most cases we just don't pass
> one because an error message after job creation is still a quite new
> thing in the QAPI schema.
>
> What we should get rid of in the long term is the int ret, not the Error
> *error. I suspect callers really just distinguish success/error without
> actually looking at the error code.
>
> With this change applied, what's your new conversion plan for making
> sure that every failing caller of job_completed() has set job->error
> first?
>
Getting rid of job_completed and moving to our fairly ubiquitous "ret &
Error *" combo.
>> @@ -666,8 +665,8 @@ static void job_update_rc(Job *job)
>> job->ret = -ECANCELED;
>> }
>> if (job->ret) {
>> - if (!job->error) {
>> - job->error = g_strdup(strerror(-job->ret));
>> + if (!job->err) {
>> + error_setg_errno(&job->err, -job->ret, "job failed");
>> }
>> job_state_transition(job, JOB_STATUS_ABORTING);
>> }
>
> This hunk just makes the error message more verbose with a "job failed"
> prefix that doesn't add information. If it's the error string for a job,
> of course the job failed.
>
> Kevin
>
Yeah, it's not a good prefix, but if I wanted to use the error object in
a general way across all jobs, I needed _some_ kind of prefix there...
Am 08.08.2018 um 17:50 hat John Snow geschrieben:
>
>
> On 08/08/2018 10:57 AM, Kevin Wolf wrote:
> > Am 07.08.2018 um 06:33 hat John Snow geschrieben:
> >> Jobs presently use both an Error object in the case of the create job,
> >> and char strings in the case of generic errors elsewhere.
> >>
> >> Unify the two paths as just j->err, and remove the extra argument from
> >> job_completed.
> >>
> >> Signed-off-by: John Snow <jsnow@redhat.com>
> >
> > Hm, not sure. Overall, this feels like a step backwards.
> >
> >> diff --git a/include/qemu/job.h b/include/qemu/job.h
> >> index 18c9223e31..845ad00c03 100644
> >> --- a/include/qemu/job.h
> >> +++ b/include/qemu/job.h
> >> @@ -124,12 +124,12 @@ 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;
> >>
> >> + /** Error object for a failed job **/
> >> + Error *err;
> >> +
> >> /** The completion function that will be called when the job completes. */
> >> BlockCompletionFunc *cb;
> >
> > This is the change that I could agree with, though I don't think it
> > makes a big difference: Whether you store the string immediately or an
> > Error object from which you get the string later, doesn't really make a
> > big difference.
> >
> > Maybe we find more uses and having an Error object is common practice in
> > QEMU, so no objections to this change.
> >
> >> @@ -484,15 +484,13 @@ 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, Error *error);
> >> +void job_completed(Job *job, int ret);
> >
> > I don't like this one, though.
> >
> > Before this change, job_completed(..., NULL) was a clear sign that the
> > error message probably needed an improvement, because an errno string
> > doesn't usually describe error situations very well. We may not have a
> > much better message in some cases, but in most cases we just don't pass
> > one because an error message after job creation is still a quite new
> > thing in the QAPI schema.
> >
> > What we should get rid of in the long term is the int ret, not the Error
> > *error. I suspect callers really just distinguish success/error without
> > actually looking at the error code.
> >
> > With this change applied, what's your new conversion plan for making
> > sure that every failing caller of job_completed() has set job->error
> > first?
> >
>
> Getting rid of job_completed and moving to our fairly ubiquitous "ret &
> Error *" combo.
Yup, with the context of the discussion for patch 2, if you make .start
(or .run) take an Error **errp, that sounds like a good plan to me.
> >> @@ -666,8 +665,8 @@ static void job_update_rc(Job *job)
> >> job->ret = -ECANCELED;
> >> }
> >> if (job->ret) {
> >> - if (!job->error) {
> >> - job->error = g_strdup(strerror(-job->ret));
> >> + if (!job->err) {
> >> + error_setg_errno(&job->err, -job->ret, "job failed");
> >> }
> >> job_state_transition(job, JOB_STATUS_ABORTING);
> >> }
> >
> > This hunk just makes the error message more verbose with a "job failed"
> > prefix that doesn't add information. If it's the error string for a job,
> > of course the job failed.
> >
> > Kevin
> >
>
> Yeah, it's not a good prefix, but if I wanted to use the error object in
> a general way across all jobs, I needed _some_ kind of prefix there...
Shouldn't this one work?
error_setg(&job->err, strerror(-job->ret));
Kevin
© 2016 - 2025 Red Hat, Inc.