* [PATCH] blockjob: report a better error message
@ 2021-02-23 13:11 Stefano Garzarella
2021-02-24 14:36 ` Kevin Wolf
0 siblings, 1 reply; 5+ messages in thread
From: Stefano Garzarella @ 2021-02-23 13:11 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, John Snow, qemu-block,
Max Reitz
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(-)
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),
--
2.29.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] blockjob: report a better error message
2021-02-23 13:11 [PATCH] blockjob: report a better error message Stefano Garzarella
@ 2021-02-24 14:36 ` Kevin Wolf
2021-02-24 15:59 ` Stefano Garzarella
0 siblings, 1 reply; 5+ messages in thread
From: Kevin Wolf @ 2021-02-24 14:36 UTC (permalink / raw)
To: Stefano Garzarella
Cc: Vladimir Sementsov-Ogievskiy, John Snow, qemu-devel, qemu-block,
Max Reitz
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] blockjob: report a better error message
2021-02-24 14:36 ` Kevin Wolf
@ 2021-02-24 15:59 ` Stefano Garzarella
2021-02-24 17:04 ` Kevin Wolf
0 siblings, 1 reply; 5+ messages in thread
From: Stefano Garzarella @ 2021-02-24 15:59 UTC (permalink / raw)
To: Kevin Wolf
Cc: Vladimir Sementsov-Ogievskiy, John Snow, qemu-devel, qemu-block,
Max Reitz
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] blockjob: report a better error message
2021-02-24 15:59 ` Stefano Garzarella
@ 2021-02-24 17:04 ` Kevin Wolf
2021-02-24 17:12 ` Stefano Garzarella
0 siblings, 1 reply; 5+ messages in thread
From: Kevin Wolf @ 2021-02-24 17:04 UTC (permalink / raw)
To: Stefano Garzarella
Cc: Vladimir Sementsov-Ogievskiy, John Snow, qemu-devel, qemu-block,
Max Reitz
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] blockjob: report a better error message
2021-02-24 17:04 ` Kevin Wolf
@ 2021-02-24 17:12 ` Stefano Garzarella
0 siblings, 0 replies; 5+ messages in thread
From: Stefano Garzarella @ 2021-02-24 17:12 UTC (permalink / raw)
To: Kevin Wolf
Cc: Vladimir Sementsov-Ogievskiy, John Snow, qemu-devel, qemu-block,
Max Reitz
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-02-24 17:23 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-23 13:11 [PATCH] blockjob: report a better error message Stefano Garzarella
2021-02-24 14:36 ` Kevin Wolf
2021-02-24 15:59 ` Stefano Garzarella
2021-02-24 17:04 ` Kevin Wolf
2021-02-24 17:12 ` Stefano Garzarella
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.