| Message ID | 20240304122844.1888308-3-clg@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | migration: Improve error reporting | expand |
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, > vfio_save_setup() always sets a new error. > > Signed-off-by: Cédric Le Goater <clg@redhat.com> Reviewed-by: Fabiano Rosas <farosas@suse.de>
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", > + 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 >
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 >>
On 3/6/24 10: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", >> + 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). Yes this is like qemu_fflush(). I will also change the test to if (ret < 0) As Prasad suggested. Thanks, C.
On 3/6/24 11:16, Avihai Horon wrote: > > 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 Ah yes. Parameters are <new> and <recover> state. Thanks, C. > >>> + 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 >>> >
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", + 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)); + } + + return ret; } static void vfio_save_cleanup(void *opaque)
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(-)