[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] blockjob: report a better error message
From: |
Kevin Wolf |
Subject: |
Re: [PATCH] blockjob: report a better error message |
Date: |
Wed, 24 Feb 2021 18:04:14 +0100 |
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