Re: [PATCH v3 02/26] vfio: Always report an error in vfio_save_setup()

[Avihai Horon]

On 06/03/2024 11:56, Avihai Horon wrote:
> On 04/03/2024 14:28, Cédric Le Goater wrote:
> > External email: Use caution opening links or attachments
> >
> >
> > This will prepare ground for futur changes adding an Error** argument
> > to the save_setup() handler. We need to make sure that on failure,
> > vfio_save_setup() always sets a new error.
> >
> > Signed-off-by: Cédric Le Goater <clg@redhat.com>
> > ---
> >   hw/vfio/migration.c | 15 ++++++++++++---
> >   1 file changed, 12 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> > index 
> > 2050ac8897231ff89cc223f0570d5c7a65dede9e..51bea536cc290ba0aa393f78b017b0650e333bff 
> > 100644
> > --- a/hw/vfio/migration.c
> > +++ b/hw/vfio/migration.c
> > @@ -383,6 +383,7 @@ static int vfio_save_setup(QEMUFile *f, void 
> > *opaque)
> >       VFIODevice *vbasedev = opaque;
> >       VFIOMigration *migration = vbasedev->migration;
> >       uint64_t stop_copy_size = VFIO_MIG_DEFAULT_DATA_BUFFER_SIZE;
> > +    int ret;
> >
> >       qemu_put_be64(f, VFIO_MIG_FLAG_DEV_SETUP_STATE);
> >
> > @@ -397,13 +398,13 @@ static int vfio_save_setup(QEMUFile *f, void 
> > *opaque)
> >       }
> >
> >       if (vfio_precopy_supported(vbasedev)) {
> > -        int ret;
> > -
> >           switch (migration->device_state) {
> >           case VFIO_DEVICE_STATE_RUNNING:
> >               ret = vfio_migration_set_state(vbasedev, 
> > VFIO_DEVICE_STATE_PRE_COPY,
> > VFIO_DEVICE_STATE_RUNNING);
> >               if (ret) {
> > +                error_report("%s: Failed to set new RUNNING state",

Oh, sorry, forgot to mention in previous mail:
s/RUNNING/PRE_COPY

> > + vbasedev->name);
> >                   return ret;
> >               }
> >
> > @@ -414,6 +415,8 @@ static int vfio_save_setup(QEMUFile *f, void 
> > *opaque)
> >               /* vfio_save_complete_precopy() will go to STOP_COPY */
> >               break;
> >           default:
> > +            error_report("%s: Invalid device state %d", vbasedev->name,
> > +                         migration->device_state);
> >               return -EINVAL;
> >           }
> >       }
> > @@ -422,7 +425,13 @@ static int vfio_save_setup(QEMUFile *f, void 
> > *opaque)
> >
> >       qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE);
> >
> > -    return qemu_file_get_error(f);
> > +    ret = qemu_file_get_error(f);
> > +    if (ret) {
> > +        error_report("%s: save setup failed : %s", vbasedev->name,
> > +                     strerror(ret));
>
> Here it should be -ret (and also later in patch #12).
>
> Thanks.
>
> > +    }
> > +
> > +    return ret;
> >   }
> >
> >   static void vfio_save_cleanup(void *opaque)
> > --
> > 2.44.0