| Message ID | 20180807043349.27196-2-jsnow@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | jobs: defer graph changes until finalize | expand |
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
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)
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(-)