Re: [PATCH v3 04/26] migration: Always report an error in ram_save_setup()

[Cédric Le Goater]

On 3/4/24 22:30, Fabiano Rosas wrote:
> Cédric Le Goater <clg@redhat.com> writes:
>
> > This will prepare ground for futur changes adding an Error** argument
> > to the save_setup() handler. We need to make sure that on failure,
> > ram_save_setup() sets a new error.
> >
> > Signed-off-by: Cédric Le Goater <clg@redhat.com>
> > ---
> >   migration/ram.c | 11 ++++++++++-
> >   1 file changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/migration/ram.c b/migration/ram.c
> > index 
> > 45a00b45edd429ef0568adce09b7459883e00167..dbd04d8ee2167881007c836a6bbc79c1aced72d0
> >  100644
> > --- a/migration/ram.c
> > +++ b/migration/ram.c
> > @@ -2937,12 +2937,14 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
> >       int ret;
> >   
> >       if (compress_threads_save_setup()) {
> > +        error_report("%s: failed to start compress threads", __func__);
> >           return -1;
> >       }
> >   
> >       /* migration has already setup the bitmap, reuse it. */
> >       if (!migration_in_colo_state()) {
> >           if (ram_init_all(rsp) != 0) {
> > +            error_report("%s: failed to setup RAM for migration", __func__);
> >               compress_threads_save_cleanup();
> >               return -1;
> >           }
> > @@ -2969,12 +2971,14 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
> >   
> >       ret = rdma_registration_start(f, RAM_CONTROL_SETUP);
> >       if (ret < 0) {
> > +        error_report("%s: failed to start RDMA registration", __func__);
> >           qemu_file_set_error(f, ret);
> >           return ret;
> >       }
> >   
> >       ret = rdma_registration_stop(f, RAM_CONTROL_SETUP);
> >       if (ret < 0) {
> > +        error_report("%s: failed to stop RDMA registration", __func__);
> >           qemu_file_set_error(f, ret);
> >           return ret;
> >       }
> > @@ -2986,6 +2990,7 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
> >       ret = multifd_send_sync_main();
> >       bql_lock();
> >       if (ret < 0) {
> > +        error_report("%s: multifd synchronization failed", __func__);
> >           return ret;
> >       }
> >   
> > @@ -2994,7 +2999,11 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
> >       }
> >   
> >       qemu_put_be64(f, RAM_SAVE_FLAG_EOS);
> > -    return qemu_fflush(f);
> > +    ret = qemu_fflush(f);
> > +    if (ret) {
> > +        error_report("%s failed : %s", __func__, strerror(ret));
>
> Here it should be -ret
>
> The qemu_fflush function returns the QEMUFile error (f->last_error) and
> that is expected to be a -errno value.

oh yes. I should have know that since qemu_file_set_error() takes a -errno.

 
> I started making sure all callers of qemu_file_set_error() use a
> negative value, but got lost in the vmstate code and never finished:
>
> https://lore.kernel.org/r/20230706195201.18595-1-farosas@suse.de

Thanks for the pointer.

Thanks,

C.