[PATCH] blockjob: report a better error message

[PATCH] blockjob: report a better error message

[Stefano Garzarella]

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

Re: [PATCH] blockjob: report a better error message

[Kevin Wolf]

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

Re: [PATCH] blockjob: report a better error message

[Stefano Garzarella]

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
>

Re: [PATCH] blockjob: report a better error message

[Kevin Wolf]

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

Re: [PATCH] blockjob: report a better error message

[Stefano Garzarella]

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