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

[Fabiano Rosas]

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?