| Message ID | 20210223131150.124867-1-sgarzare@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | blockjob: report a better error message | expand |
Am 23.02.2021 um 14:11 hat Stefano Garzarella geschrieben: > When a block job fails, we report 'strerror(-job->job.ret)' error > message, also if the job set an error object. > Let's report a better error message using 'error_get_pretty(job->job.err)'. > > If an error object was not set, strerror(-job->ret) is used as fallback, > as explained in include/qemu/job.h: > > typedef struct Job { > ... > /** > * Error object for a failed job. > * If job->ret is nonzero and an error object was not set, it will be set > * to strerror(-job->ret) during job_completed. > */ > Error *err; > } This is true, but there is a short time where job->ret is already set, but not turned into job->err yet if necessary. The latter is done in a bottom half scheduled after the former has happened. It doesn't matter for block_job_event_completed(), which is called only after both, but block_job_query() could in theory be called in this window. > Suggested-by: Kevin Wolf <kwolf@redhat.com> > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> > --- > blockjob.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/blockjob.c b/blockjob.c > index f2feff051d..a696f3408d 100644 > --- a/blockjob.c > +++ b/blockjob.c > @@ -319,7 +319,8 @@ BlockJobInfo *block_job_query(BlockJob *job, Error **errp) > info->auto_finalize = job->job.auto_finalize; > info->auto_dismiss = job->job.auto_dismiss; > info->has_error = job->job.ret != 0; > - info->error = job->job.ret ? g_strdup(strerror(-job->job.ret)) : NULL; > + info->error = job->job.ret ? > + g_strdup(error_get_pretty(job->job.err)) : NULL; So I think we can't rely on job->job.err being non-NULL here. > return info; > } > > @@ -356,7 +357,7 @@ static void block_job_event_completed(Notifier *n, void *opaque) > } > > if (job->job.ret < 0) { > - msg = strerror(-job->job.ret); > + msg = error_get_pretty(job->job.err); > } > > qapi_event_send_block_job_completed(job_type(&job->job), Kevin
On Wed, Feb 24, 2021 at 03:36:20PM +0100, Kevin Wolf wrote: >Am 23.02.2021 um 14:11 hat Stefano Garzarella geschrieben: >> When a block job fails, we report 'strerror(-job->job.ret)' error >> message, also if the job set an error object. >> Let's report a better error message using 'error_get_pretty(job->job.err)'. >> >> If an error object was not set, strerror(-job->ret) is used as fallback, >> as explained in include/qemu/job.h: >> >> typedef struct Job { >> ... >> /** >> * Error object for a failed job. >> * If job->ret is nonzero and an error object was not set, it will be set >> * to strerror(-job->ret) during job_completed. >> */ >> Error *err; >> } > >This is true, but there is a short time where job->ret is already set, >but not turned into job->err yet if necessary. The latter is done in a >bottom half scheduled after the former has happened. > >It doesn't matter for block_job_event_completed(), which is called only >after both, but block_job_query() could in theory be called in this >window. > >> Suggested-by: Kevin Wolf <kwolf@redhat.com> >> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> >> --- >> blockjob.c | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/blockjob.c b/blockjob.c >> index f2feff051d..a696f3408d 100644 >> --- a/blockjob.c >> +++ b/blockjob.c >> @@ -319,7 +319,8 @@ BlockJobInfo *block_job_query(BlockJob *job, Error **errp) >> info->auto_finalize = job->job.auto_finalize; >> info->auto_dismiss = job->job.auto_dismiss; >> info->has_error = job->job.ret != 0; >> - info->error = job->job.ret ? g_strdup(strerror(-job->job.ret)) : NULL; >> + info->error = job->job.ret ? >> + g_strdup(error_get_pretty(job->job.err)) : NULL; > >So I think we can't rely on job->job.err being non-NULL here. Do you think is better to leave it as it was or do something like this? if (job->job.ret) { info->has_error = true; info->error = job->job.err ? g_strdup(error_get_pretty(job->job.err)) : g_strdup(strerror(-job->job.ret); } Thanks, Stefano > >> return info; >> } >> >> @@ -356,7 +357,7 @@ static void block_job_event_completed(Notifier *n, void *opaque) >> } >> >> if (job->job.ret < 0) { >> - msg = strerror(-job->job.ret); >> + msg = error_get_pretty(job->job.err); >> } >> >> qapi_event_send_block_job_completed(job_type(&job->job), > >Kevin >
Am 24.02.2021 um 16:59 hat Stefano Garzarella geschrieben: > On Wed, Feb 24, 2021 at 03:36:20PM +0100, Kevin Wolf wrote: > > Am 23.02.2021 um 14:11 hat Stefano Garzarella geschrieben: > > > When a block job fails, we report 'strerror(-job->job.ret)' error > > > message, also if the job set an error object. > > > Let's report a better error message using 'error_get_pretty(job->job.err)'. > > > > > > If an error object was not set, strerror(-job->ret) is used as fallback, > > > as explained in include/qemu/job.h: > > > > > > typedef struct Job { > > > ... > > > /** > > > * Error object for a failed job. > > > * If job->ret is nonzero and an error object was not set, it will be set > > > * to strerror(-job->ret) during job_completed. > > > */ > > > Error *err; > > > } > > > > This is true, but there is a short time where job->ret is already set, > > but not turned into job->err yet if necessary. The latter is done in a > > bottom half scheduled after the former has happened. > > > > It doesn't matter for block_job_event_completed(), which is called only > > after both, but block_job_query() could in theory be called in this > > window. > > > > > Suggested-by: Kevin Wolf <kwolf@redhat.com> > > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> > > > --- > > > blockjob.c | 5 +++-- > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > diff --git a/blockjob.c b/blockjob.c > > > index f2feff051d..a696f3408d 100644 > > > --- a/blockjob.c > > > +++ b/blockjob.c > > > @@ -319,7 +319,8 @@ BlockJobInfo *block_job_query(BlockJob *job, Error **errp) > > > info->auto_finalize = job->job.auto_finalize; > > > info->auto_dismiss = job->job.auto_dismiss; > > > info->has_error = job->job.ret != 0; > > > - info->error = job->job.ret ? g_strdup(strerror(-job->job.ret)) : NULL; > > > + info->error = job->job.ret ? > > > + g_strdup(error_get_pretty(job->job.err)) : NULL; > > > > So I think we can't rely on job->job.err being non-NULL here. > > Do you think is better to leave it as it was or do something like this? > > if (job->job.ret) { > info->has_error = true; > info->error = job->job.err ? g_strdup(error_get_pretty(job->job.err)) : > g_strdup(strerror(-job->job.ret); > } Yes, I think this is the best solution. Use the error when we have it, fall back to strerror() when we don't have it. Kevin
On Wed, Feb 24, 2021 at 06:04:14PM +0100, Kevin Wolf wrote: >Am 24.02.2021 um 16:59 hat Stefano Garzarella geschrieben: >> On Wed, Feb 24, 2021 at 03:36:20PM +0100, Kevin Wolf wrote: >> > Am 23.02.2021 um 14:11 hat Stefano Garzarella geschrieben: >> > > When a block job fails, we report 'strerror(-job->job.ret)' error >> > > message, also if the job set an error object. >> > > Let's report a better error message using 'error_get_pretty(job->job.err)'. >> > > >> > > If an error object was not set, strerror(-job->ret) is used as fallback, >> > > as explained in include/qemu/job.h: >> > > >> > > typedef struct Job { >> > > ... >> > > /** >> > > * Error object for a failed job. >> > > * If job->ret is nonzero and an error object was not set, it will be set >> > > * to strerror(-job->ret) during job_completed. >> > > */ >> > > Error *err; >> > > } >> > >> > This is true, but there is a short time where job->ret is already set, >> > but not turned into job->err yet if necessary. The latter is done in a >> > bottom half scheduled after the former has happened. >> > >> > It doesn't matter for block_job_event_completed(), which is called only >> > after both, but block_job_query() could in theory be called in this >> > window. >> > >> > > Suggested-by: Kevin Wolf <kwolf@redhat.com> >> > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> >> > > --- >> > > blockjob.c | 5 +++-- >> > > 1 file changed, 3 insertions(+), 2 deletions(-) >> > > >> > > diff --git a/blockjob.c b/blockjob.c >> > > index f2feff051d..a696f3408d 100644 >> > > --- a/blockjob.c >> > > +++ b/blockjob.c >> > > @@ -319,7 +319,8 @@ BlockJobInfo *block_job_query(BlockJob *job, Error **errp) >> > > info->auto_finalize = job->job.auto_finalize; >> > > info->auto_dismiss = job->job.auto_dismiss; >> > > info->has_error = job->job.ret != 0; >> > > - info->error = job->job.ret ? g_strdup(strerror(-job->job.ret)) : NULL; >> > > + info->error = job->job.ret ? >> > > + g_strdup(error_get_pretty(job->job.err)) : NULL; >> > >> > So I think we can't rely on job->job.err being non-NULL here. >> >> Do you think is better to leave it as it was or do something like this? >> >> if (job->job.ret) { >> info->has_error = true; >> info->error = job->job.err ? g_strdup(error_get_pretty(job->job.err)) : >> g_strdup(strerror(-job->job.ret); >> } > >Yes, I think this is the best solution. Use the error when we have it, >fall back to strerror() when we don't have it. > Okay, I'll send v2 with that fixed. Thanks, Stefano
diff --git a/blockjob.c b/blockjob.c index f2feff051d..a696f3408d 100644 --- a/blockjob.c +++ b/blockjob.c @@ -319,7 +319,8 @@ BlockJobInfo *block_job_query(BlockJob *job, Error **errp) info->auto_finalize = job->job.auto_finalize; info->auto_dismiss = job->job.auto_dismiss; info->has_error = job->job.ret != 0; - info->error = job->job.ret ? g_strdup(strerror(-job->job.ret)) : NULL; + info->error = job->job.ret ? + g_strdup(error_get_pretty(job->job.err)) : NULL; return info; } @@ -356,7 +357,7 @@ static void block_job_event_completed(Notifier *n, void *opaque) } if (job->job.ret < 0) { - msg = strerror(-job->job.ret); + msg = error_get_pretty(job->job.err); } qapi_event_send_block_job_completed(job_type(&job->job),
When a block job fails, we report 'strerror(-job->job.ret)' error message, also if the job set an error object. Let's report a better error message using 'error_get_pretty(job->job.err)'. If an error object was not set, strerror(-job->ret) is used as fallback, as explained in include/qemu/job.h: typedef struct Job { ... /** * Error object for a failed job. * If job->ret is nonzero and an error object was not set, it will be set * to strerror(-job->ret) during job_completed. */ Error *err; } Suggested-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> --- blockjob.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)