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 <kw...@redhat.com> > > > Signed-off-by: Stefano Garzarella <sgarz...@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