Re: [PATCH v4 11/25] migration: Add Error** argument to .save_setup() handler

[Vladimir Sementsov-Ogievskiy]

On 06.03.24 16:34, Cédric Le Goater wrote:
> The purpose is to record a potential error in the migration stream if
> qemu_savevm_state_setup() fails. Most of the current .save_setup()
> handlers can be modified to use the Error argument instead of managing
> their own and calling locally error_report().
>
> Cc: Nicholas Piggin <npiggin@gmail.com>
> Cc: Harsh Prateek Bora <harshpb@linux.ibm.com>
> Cc: Halil Pasic <pasic@linux.ibm.com>
> Cc: Thomas Huth <thuth@redhat.com>
> Cc: Eric Blake <eblake@redhat.com>
> Cc: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> Cc: John Snow <jsnow@redhat.com>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Peter Xu <peterx@redhat.com>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

Still, if you resend, please add error_prepend in the case below:

> diff --git a/migration/savevm.c b/migration/savevm.c
> index 
> 63fdbb5ad7d4dbfaef1d2094350bf302cc677602..52d35b2a72c6238bfe5dcb4d81c1af8d2bf73013
>  100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1342,11 +1342,9 @@ int qemu_savevm_state_setup(QEMUFile *f, Error **errp)
>           }
>           save_section_header(f, se, QEMU_VM_SECTION_START);
>   
> -        ret = se->ops->save_setup(f, se->opaque);
> +        ret = se->ops->save_setup(f, se->opaque, errp);
>           save_section_footer(f, se);
>           if (ret < 0) {
> -            error_setg(errp, "failed to setup SaveStateEntry with id(name): "
> -                       "%d(%s): %d", se->section_id, se->idstr, ret);

You drop a good bit of information, let's use error_prepend to save it.

>               qemu_file_set_error(f, ret);
>               break;

Not about this patch:

Better do explicit "return ret" instead of this "break" (and one more break 
above in that loop):

1. making a jump to do just do "return ret" seems overkill. It would make sense if we had some more 
"cleanup" code than just a "return ret", and if so, more classic and readable thing is "goto 
fail;".
2. "break" make me think, that there may be more logic after it, which will probably 
fail, and I should be careful, as errp is already set (and second attempt to set it will crash). 
Again, "goto fail;" is better, as I don't expect more failures when see it.

--
Best regards,
Vladimir