diff mbox series

[v2,6/7] qemufile: Always return a verbose error

Message ID 20230705163502.331007-7-peterx@redhat.com
State New
Headers show
Series migration: Better error handling in return path thread | expand

Commit Message

Peter Xu July 5, 2023, 4:35 p.m. UTC
There're a lot of cases where we only have an errno set in last_error but
without a detailed error description.  When this happens, try to generate
an error contains the errno as a descriptive error.

This will be helpful in cases where one relies on the Error*.  E.g.,
migration state only caches Error* in MigrationState.error.  With this,
we'll display correct error messages in e.g. query-migrate when the error
was only set by qemu_file_set_error().

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/qemu-file.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

Comments

Fabiano Rosas July 5, 2023, 9:54 p.m. UTC | #1
Peter Xu <peterx@redhat.com> writes:

> There're a lot of cases where we only have an errno set in last_error but
> without a detailed error description.  When this happens, try to generate
> an error contains the errno as a descriptive error.
>
> This will be helpful in cases where one relies on the Error*.  E.g.,
> migration state only caches Error* in MigrationState.error.  With this,
> we'll display correct error messages in e.g. query-migrate when the error
> was only set by qemu_file_set_error().
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/qemu-file.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/migration/qemu-file.c b/migration/qemu-file.c
> index acc282654a..419b4092e7 100644
> --- a/migration/qemu-file.c
> +++ b/migration/qemu-file.c
> @@ -156,15 +156,24 @@ void qemu_file_set_hooks(QEMUFile *f, const QEMUFileHooks *hooks)
>   *
>   * Return negative error value if there has been an error on previous
>   * operations, return 0 if no error happened.
> - * Optional, it returns Error* in errp, but it may be NULL even if return value
> - * is not 0.
>   *
> + * If errp is specified, a verbose error message will be copied over.
>   */
>  int qemu_file_get_error_obj(QEMUFile *f, Error **errp)
>  {
> +    if (!f->last_error) {
> +        return 0;
> +    }
> +
> +    /* There is an error */
>      if (errp) {
> -        *errp = f->last_error_obj ? error_copy(f->last_error_obj) : NULL;
> +        if (f->last_error_obj) {
> +            *errp = error_copy(f->last_error_obj);
> +        } else {
> +            error_setg_errno(errp, -f->last_error, "Channel error");

There are a couple of places that do:

    ret = vmstate_save(f, se, ms->vmdesc);
    if (ret) {
        qemu_file_set_error(f, ret);
        break;
    }

and vmstate_save() can return > 0 on error. This would make this message
say "Unknown error". This is minor.

But take a look at qemu_fclose(). It can return f->last_error while the
function documentation says it should return negative on error.

Should we make qemu_file_set_error() check 'ret' and always set a
negative value for f->last_error?
Peter Xu July 5, 2023, 10:24 p.m. UTC | #2
On Wed, Jul 05, 2023 at 06:54:37PM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > There're a lot of cases where we only have an errno set in last_error but
> > without a detailed error description.  When this happens, try to generate
> > an error contains the errno as a descriptive error.
> >
> > This will be helpful in cases where one relies on the Error*.  E.g.,
> > migration state only caches Error* in MigrationState.error.  With this,
> > we'll display correct error messages in e.g. query-migrate when the error
> > was only set by qemu_file_set_error().
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  migration/qemu-file.c | 15 ++++++++++++---
> >  1 file changed, 12 insertions(+), 3 deletions(-)
> >
> > diff --git a/migration/qemu-file.c b/migration/qemu-file.c
> > index acc282654a..419b4092e7 100644
> > --- a/migration/qemu-file.c
> > +++ b/migration/qemu-file.c
> > @@ -156,15 +156,24 @@ void qemu_file_set_hooks(QEMUFile *f, const QEMUFileHooks *hooks)
> >   *
> >   * Return negative error value if there has been an error on previous
> >   * operations, return 0 if no error happened.
> > - * Optional, it returns Error* in errp, but it may be NULL even if return value
> > - * is not 0.
> >   *
> > + * If errp is specified, a verbose error message will be copied over.
> >   */
> >  int qemu_file_get_error_obj(QEMUFile *f, Error **errp)
> >  {
> > +    if (!f->last_error) {
> > +        return 0;
> > +    }
> > +
> > +    /* There is an error */
> >      if (errp) {
> > -        *errp = f->last_error_obj ? error_copy(f->last_error_obj) : NULL;
> > +        if (f->last_error_obj) {
> > +            *errp = error_copy(f->last_error_obj);
> > +        } else {
> > +            error_setg_errno(errp, -f->last_error, "Channel error");
> 
> There are a couple of places that do:
> 
>     ret = vmstate_save(f, se, ms->vmdesc);
>     if (ret) {
>         qemu_file_set_error(f, ret);
>         break;
>     }
> 
> and vmstate_save() can return > 0 on error. This would make this message
> say "Unknown error". This is minor.
> 
> But take a look at qemu_fclose(). It can return f->last_error while the
> function documentation says it should return negative on error.
> 
> Should we make qemu_file_set_error() check 'ret' and always set a
> negative value for f->last_error?

Yeah, maybe we can add a sanity check, but logically it's better we just
fix vmstate_save() to make sure it always returns a <0 error.

It seems to me there're so many hooks in vmstate_save_state_v() that it can
return random things.  What's the one you spot?  If it's an obvious issue
we can fix them.
Fabiano Rosas July 6, 2023, 1:56 p.m. UTC | #3
Peter Xu <peterx@redhat.com> writes:

> On Wed, Jul 05, 2023 at 06:54:37PM -0300, Fabiano Rosas wrote:
>> Peter Xu <peterx@redhat.com> writes:
>> 
>> > There're a lot of cases where we only have an errno set in last_error but
>> > without a detailed error description.  When this happens, try to generate
>> > an error contains the errno as a descriptive error.
>> >
>> > This will be helpful in cases where one relies on the Error*.  E.g.,
>> > migration state only caches Error* in MigrationState.error.  With this,
>> > we'll display correct error messages in e.g. query-migrate when the error
>> > was only set by qemu_file_set_error().
>> >
>> > Signed-off-by: Peter Xu <peterx@redhat.com>
>> > ---
>> >  migration/qemu-file.c | 15 ++++++++++++---
>> >  1 file changed, 12 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/migration/qemu-file.c b/migration/qemu-file.c
>> > index acc282654a..419b4092e7 100644
>> > --- a/migration/qemu-file.c
>> > +++ b/migration/qemu-file.c
>> > @@ -156,15 +156,24 @@ void qemu_file_set_hooks(QEMUFile *f, const QEMUFileHooks *hooks)
>> >   *
>> >   * Return negative error value if there has been an error on previous
>> >   * operations, return 0 if no error happened.
>> > - * Optional, it returns Error* in errp, but it may be NULL even if return value
>> > - * is not 0.
>> >   *
>> > + * If errp is specified, a verbose error message will be copied over.
>> >   */
>> >  int qemu_file_get_error_obj(QEMUFile *f, Error **errp)
>> >  {
>> > +    if (!f->last_error) {
>> > +        return 0;
>> > +    }
>> > +
>> > +    /* There is an error */
>> >      if (errp) {
>> > -        *errp = f->last_error_obj ? error_copy(f->last_error_obj) : NULL;
>> > +        if (f->last_error_obj) {
>> > +            *errp = error_copy(f->last_error_obj);
>> > +        } else {
>> > +            error_setg_errno(errp, -f->last_error, "Channel error");
>> 
>> There are a couple of places that do:
>> 
>>     ret = vmstate_save(f, se, ms->vmdesc);
>>     if (ret) {
>>         qemu_file_set_error(f, ret);
>>         break;
>>     }
>> 
>> and vmstate_save() can return > 0 on error. This would make this message
>> say "Unknown error". This is minor.
>> 
>> But take a look at qemu_fclose(). It can return f->last_error while the
>> function documentation says it should return negative on error.
>> 
>> Should we make qemu_file_set_error() check 'ret' and always set a
>> negative value for f->last_error?
>
> Yeah, maybe we can add a sanity check, but logically it's better we just
> fix vmstate_save() to make sure it always returns a <0 error.
>
> It seems to me there're so many hooks in vmstate_save_state_v() that it can
> return random things.  What's the one you spot?  If it's an obvious issue
> we can fix them.

I see at least:
    ret = field->info->put(f, curr_elem, size, field, vmdesc_loop);
with put_power() from target/arm/machine.c returning 1.

Since vmstate_save_state_v() is quite involved I don't think we should
block this series because of it. I can do a closer audit and send a
separate patch with it.

So:

Reviewed-by: Fabiano Rosas <farosas@suse.de>
diff mbox series

Patch

diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index acc282654a..419b4092e7 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -156,15 +156,24 @@  void qemu_file_set_hooks(QEMUFile *f, const QEMUFileHooks *hooks)
  *
  * Return negative error value if there has been an error on previous
  * operations, return 0 if no error happened.
- * Optional, it returns Error* in errp, but it may be NULL even if return value
- * is not 0.
  *
+ * If errp is specified, a verbose error message will be copied over.
  */
 int qemu_file_get_error_obj(QEMUFile *f, Error **errp)
 {
+    if (!f->last_error) {
+        return 0;
+    }
+
+    /* There is an error */
     if (errp) {
-        *errp = f->last_error_obj ? error_copy(f->last_error_obj) : NULL;
+        if (f->last_error_obj) {
+            *errp = error_copy(f->last_error_obj);
+        } else {
+            error_setg_errno(errp, -f->last_error, "Channel error");
+        }
     }
+
     return f->last_error;
 }