| Message ID | 20240207133347.1115903-2-clg@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | migration: Improve error reporting | expand |
On 7/2/24 14:33, 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(). The following patches > will introduce such changes for VFIO first. > > Signed-off-by: Cédric Le Goater <clg@redhat.com> > --- > include/migration/register.h | 2 +- > hw/ppc/spapr.c | 2 +- > hw/s390x/s390-stattrib.c | 2 +- > hw/vfio/migration.c | 2 +- > migration/block-dirty-bitmap.c | 2 +- > migration/block.c | 2 +- > migration/ram.c | 2 +- > migration/savevm.c | 4 ++-- > 8 files changed, 9 insertions(+), 9 deletions(-) > > diff --git a/include/migration/register.h b/include/migration/register.h > index 9ab1f79512c605f0c88a45b560c57486fa054441..831600a00eae4efd0464b60925d65de4d9dbcff8 100644 > --- a/include/migration/register.h > +++ b/include/migration/register.h > @@ -25,7 +25,7 @@ typedef struct SaveVMHandlers { > * used to perform early checks. > */ > int (*save_prepare)(void *opaque, Error **errp); > - int (*save_setup)(QEMUFile *f, void *opaque); > + int (*save_setup)(QEMUFile *f, void *opaque, Error **errp); Since you change this, do you mind adding a docstring describing this prototype? Otherwise, Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
On Wed, Feb 07, 2024 at 02:33:34PM +0100, Cédric Le Goater wrote: > diff --git a/migration/ram.c b/migration/ram.c > index d5b7cd5ac2f31aabf4a248b966153401c48912cf..136c237f4079f68d4e578cf1c72eec2efc815bc8 100644 > --- a/migration/ram.c > +++ b/migration/ram.c > @@ -2931,7 +2931,7 @@ void qemu_guest_free_page_hint(void *addr, size_t len) > * @f: QEMUFile where to send the data > * @opaque: RAMState pointer Document may need a touch-up. > */ > -static int ram_save_setup(QEMUFile *f, void *opaque) > +static int ram_save_setup(QEMUFile *f, void *opaque, Error **errp) > { > RAMState **rsp = opaque; > RAMBlock *block; Besides: Reviewed-by: Peter Xu <peterx@redhat.com>
On 2/7/24 21:11, Philippe Mathieu-Daudé wrote: > On 7/2/24 14:33, 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(). The following patches >> will introduce such changes for VFIO first. >> >> Signed-off-by: Cédric Le Goater <clg@redhat.com> >> --- >> include/migration/register.h | 2 +- >> hw/ppc/spapr.c | 2 +- >> hw/s390x/s390-stattrib.c | 2 +- >> hw/vfio/migration.c | 2 +- >> migration/block-dirty-bitmap.c | 2 +- >> migration/block.c | 2 +- >> migration/ram.c | 2 +- >> migration/savevm.c | 4 ++-- >> 8 files changed, 9 insertions(+), 9 deletions(-) >> >> diff --git a/include/migration/register.h b/include/migration/register.h >> index 9ab1f79512c605f0c88a45b560c57486fa054441..831600a00eae4efd0464b60925d65de4d9dbcff8 100644 >> --- a/include/migration/register.h >> +++ b/include/migration/register.h >> @@ -25,7 +25,7 @@ typedef struct SaveVMHandlers { >> * used to perform early checks. >> */ >> int (*save_prepare)(void *opaque, Error **errp); >> - int (*save_setup)(QEMUFile *f, void *opaque); >> + int (*save_setup)(QEMUFile *f, void *opaque, Error **errp); > > Since you change this, do you mind adding a docstring > describing this prototype? I can send an initial patch adding the documentation tags and then resend the same patch with the updates people will provide. I don't have the knowledge to cover all of the SaveVMHandlers struct on my own. Thanks, C. > Otherwise, > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> >
Hi, Cedric On 07/02/2024 15:33, Cédric Le Goater wrote: > External email: Use caution opening links or attachments > > > 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(). The following patches > will introduce such changes for VFIO first. > > Signed-off-by: Cédric Le Goater <clg@redhat.com> > --- > include/migration/register.h | 2 +- > hw/ppc/spapr.c | 2 +- > hw/s390x/s390-stattrib.c | 2 +- > hw/vfio/migration.c | 2 +- > migration/block-dirty-bitmap.c | 2 +- > migration/block.c | 2 +- > migration/ram.c | 2 +- > migration/savevm.c | 4 ++-- > 8 files changed, 9 insertions(+), 9 deletions(-) > > diff --git a/include/migration/register.h b/include/migration/register.h > index 9ab1f79512c605f0c88a45b560c57486fa054441..831600a00eae4efd0464b60925d65de4d9dbcff8 100644 > --- a/include/migration/register.h > +++ b/include/migration/register.h > @@ -25,7 +25,7 @@ typedef struct SaveVMHandlers { > * used to perform early checks. > */ > int (*save_prepare)(void *opaque, Error **errp); > - int (*save_setup)(QEMUFile *f, void *opaque); > + int (*save_setup)(QEMUFile *f, void *opaque, Error **errp); > void (*save_cleanup)(void *opaque); > int (*save_live_complete_postcopy)(QEMUFile *f, void *opaque); > int (*save_live_complete_precopy)(QEMUFile *f, void *opaque); > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 0d72d286d80f0435122593555f79fae4d90acf81..a1b0aa02582ad2d68a13476c1859b18143da7bb8 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -2142,7 +2142,7 @@ static const VMStateDescription vmstate_spapr = { > } > }; > > -static int htab_save_setup(QEMUFile *f, void *opaque) > +static int htab_save_setup(QEMUFile *f, void *opaque, Error **errp) > { > SpaprMachineState *spapr = opaque; > > diff --git a/hw/s390x/s390-stattrib.c b/hw/s390x/s390-stattrib.c > index c483b62a9b5f71772639fc180bdad15ecb6711cb..c934df424a555d83d2198f5ddfc0cbe0ea98e9ec 100644 > --- a/hw/s390x/s390-stattrib.c > +++ b/hw/s390x/s390-stattrib.c > @@ -166,7 +166,7 @@ static int cmma_load(QEMUFile *f, void *opaque, int version_id) > return ret; > } > > -static int cmma_save_setup(QEMUFile *f, void *opaque) > +static int cmma_save_setup(QEMUFile *f, void *opaque, Error **errp) > { > S390StAttribState *sas = S390_STATTRIB(opaque); > S390StAttribClass *sac = S390_STATTRIB_GET_CLASS(sas); > diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c > index 70e6b1a709f9b67e4c9eb41033d76347275cac42..8bcb4bc73cd5ba5338e3ffa4d907d0e6bfbb9485 100644 > --- a/hw/vfio/migration.c > +++ b/hw/vfio/migration.c > @@ -378,7 +378,7 @@ static int vfio_save_prepare(void *opaque, Error **errp) > return 0; > } > > -static int vfio_save_setup(QEMUFile *f, void *opaque) > +static int vfio_save_setup(QEMUFile *f, void *opaque, Error **errp) > { > VFIODevice *vbasedev = opaque; > VFIOMigration *migration = vbasedev->migration; > diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c > index 2708abf3d762de774ed294d3fdb8e56690d2974c..16f84e6c57c2403a8c2d6319f4e7b6360dade28c 100644 > --- a/migration/block-dirty-bitmap.c > +++ b/migration/block-dirty-bitmap.c > @@ -1213,7 +1213,7 @@ fail: > return ret; > } > > -static int dirty_bitmap_save_setup(QEMUFile *f, void *opaque) > +static int dirty_bitmap_save_setup(QEMUFile *f, void *opaque, Error **errp) > { > DBMSaveState *s = &((DBMState *)opaque)->save; > SaveBitmapState *dbms = NULL; > diff --git a/migration/block.c b/migration/block.c > index 8c6ebafacc1ffe930d1d4f19d968817b14852c69..df15319ceab66201b043f15eac1b0a7d6522b60c 100644 > --- a/migration/block.c > +++ b/migration/block.c > @@ -708,7 +708,7 @@ static void block_migration_cleanup(void *opaque) > blk_mig_unlock(); > } > > -static int block_save_setup(QEMUFile *f, void *opaque) > +static int block_save_setup(QEMUFile *f, void *opaque, Error **errp) > { > int ret; > > diff --git a/migration/ram.c b/migration/ram.c > index d5b7cd5ac2f31aabf4a248b966153401c48912cf..136c237f4079f68d4e578cf1c72eec2efc815bc8 100644 > --- a/migration/ram.c > +++ b/migration/ram.c > @@ -2931,7 +2931,7 @@ void qemu_guest_free_page_hint(void *addr, size_t len) > * @f: QEMUFile where to send the data > * @opaque: RAMState pointer > */ > -static int ram_save_setup(QEMUFile *f, void *opaque) > +static int ram_save_setup(QEMUFile *f, void *opaque, Error **errp) > { > RAMState **rsp = opaque; > RAMBlock *block; > diff --git a/migration/savevm.c b/migration/savevm.c > index d612c8a9020b204d5d078d5df85f0e6449c27645..f2ae799bad13e631bccf733a34c3a8fd22e8dd48 100644 > --- a/migration/savevm.c > +++ b/migration/savevm.c > @@ -1342,10 +1342,10 @@ void qemu_savevm_state_setup(QEMUFile *f) > } > 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, &local_err); > save_section_footer(f, se); > if (ret < 0) { > - qemu_file_set_error(f, ret); > + qemu_file_set_error_obj(f, ret, local_err); Should we set local_err = NULL? Because it is re-used a few lines after this, by precopy_notify(). BTW, I think that if we add Error** parameter to functions we must make sure all their error flows set errp as well. According to Error API: * - On success, the function should not touch *errp. On failure, it * should set a new error, e.g. with error_setg(errp, ...), or * propagate an existing one, e.g. with error_propagate(errp, ...). For example, a caller that handles errors by printing them with error_report_err() would crash when trying to access NULL error object (if some error path didn't set errp). If you agree, we should check it throughout the series. Thanks. > break; > } > } > -- > 2.43.0 > >
On 2/12/24 09:36, Avihai Horon wrote: > Hi, Cedric > > On 07/02/2024 15:33, Cédric Le Goater wrote: >> External email: Use caution opening links or attachments >> >> >> 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(). The following patches >> will introduce such changes for VFIO first. >> >> Signed-off-by: Cédric Le Goater <clg@redhat.com> >> --- >> include/migration/register.h | 2 +- >> hw/ppc/spapr.c | 2 +- >> hw/s390x/s390-stattrib.c | 2 +- >> hw/vfio/migration.c | 2 +- >> migration/block-dirty-bitmap.c | 2 +- >> migration/block.c | 2 +- >> migration/ram.c | 2 +- >> migration/savevm.c | 4 ++-- >> 8 files changed, 9 insertions(+), 9 deletions(-) >> >> diff --git a/include/migration/register.h b/include/migration/register.h >> index 9ab1f79512c605f0c88a45b560c57486fa054441..831600a00eae4efd0464b60925d65de4d9dbcff8 100644 >> --- a/include/migration/register.h >> +++ b/include/migration/register.h >> @@ -25,7 +25,7 @@ typedef struct SaveVMHandlers { >> * used to perform early checks. >> */ >> int (*save_prepare)(void *opaque, Error **errp); >> - int (*save_setup)(QEMUFile *f, void *opaque); >> + int (*save_setup)(QEMUFile *f, void *opaque, Error **errp); >> void (*save_cleanup)(void *opaque); >> int (*save_live_complete_postcopy)(QEMUFile *f, void *opaque); >> int (*save_live_complete_precopy)(QEMUFile *f, void *opaque); >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c >> index 0d72d286d80f0435122593555f79fae4d90acf81..a1b0aa02582ad2d68a13476c1859b18143da7bb8 100644 >> --- a/hw/ppc/spapr.c >> +++ b/hw/ppc/spapr.c >> @@ -2142,7 +2142,7 @@ static const VMStateDescription vmstate_spapr = { >> } >> }; >> >> -static int htab_save_setup(QEMUFile *f, void *opaque) >> +static int htab_save_setup(QEMUFile *f, void *opaque, Error **errp) >> { >> SpaprMachineState *spapr = opaque; >> >> diff --git a/hw/s390x/s390-stattrib.c b/hw/s390x/s390-stattrib.c >> index c483b62a9b5f71772639fc180bdad15ecb6711cb..c934df424a555d83d2198f5ddfc0cbe0ea98e9ec 100644 >> --- a/hw/s390x/s390-stattrib.c >> +++ b/hw/s390x/s390-stattrib.c >> @@ -166,7 +166,7 @@ static int cmma_load(QEMUFile *f, void *opaque, int version_id) >> return ret; >> } >> >> -static int cmma_save_setup(QEMUFile *f, void *opaque) >> +static int cmma_save_setup(QEMUFile *f, void *opaque, Error **errp) >> { >> S390StAttribState *sas = S390_STATTRIB(opaque); >> S390StAttribClass *sac = S390_STATTRIB_GET_CLASS(sas); >> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c >> index 70e6b1a709f9b67e4c9eb41033d76347275cac42..8bcb4bc73cd5ba5338e3ffa4d907d0e6bfbb9485 100644 >> --- a/hw/vfio/migration.c >> +++ b/hw/vfio/migration.c >> @@ -378,7 +378,7 @@ static int vfio_save_prepare(void *opaque, Error **errp) >> return 0; >> } >> >> -static int vfio_save_setup(QEMUFile *f, void *opaque) >> +static int vfio_save_setup(QEMUFile *f, void *opaque, Error **errp) >> { >> VFIODevice *vbasedev = opaque; >> VFIOMigration *migration = vbasedev->migration; >> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c >> index 2708abf3d762de774ed294d3fdb8e56690d2974c..16f84e6c57c2403a8c2d6319f4e7b6360dade28c 100644 >> --- a/migration/block-dirty-bitmap.c >> +++ b/migration/block-dirty-bitmap.c >> @@ -1213,7 +1213,7 @@ fail: >> return ret; >> } >> >> -static int dirty_bitmap_save_setup(QEMUFile *f, void *opaque) >> +static int dirty_bitmap_save_setup(QEMUFile *f, void *opaque, Error **errp) >> { >> DBMSaveState *s = &((DBMState *)opaque)->save; >> SaveBitmapState *dbms = NULL; >> diff --git a/migration/block.c b/migration/block.c >> index 8c6ebafacc1ffe930d1d4f19d968817b14852c69..df15319ceab66201b043f15eac1b0a7d6522b60c 100644 >> --- a/migration/block.c >> +++ b/migration/block.c >> @@ -708,7 +708,7 @@ static void block_migration_cleanup(void *opaque) >> blk_mig_unlock(); >> } >> >> -static int block_save_setup(QEMUFile *f, void *opaque) >> +static int block_save_setup(QEMUFile *f, void *opaque, Error **errp) >> { >> int ret; >> >> diff --git a/migration/ram.c b/migration/ram.c >> index d5b7cd5ac2f31aabf4a248b966153401c48912cf..136c237f4079f68d4e578cf1c72eec2efc815bc8 100644 >> --- a/migration/ram.c >> +++ b/migration/ram.c >> @@ -2931,7 +2931,7 @@ void qemu_guest_free_page_hint(void *addr, size_t len) >> * @f: QEMUFile where to send the data >> * @opaque: RAMState pointer >> */ >> -static int ram_save_setup(QEMUFile *f, void *opaque) >> +static int ram_save_setup(QEMUFile *f, void *opaque, Error **errp) >> { >> RAMState **rsp = opaque; >> RAMBlock *block; >> diff --git a/migration/savevm.c b/migration/savevm.c >> index d612c8a9020b204d5d078d5df85f0e6449c27645..f2ae799bad13e631bccf733a34c3a8fd22e8dd48 100644 >> --- a/migration/savevm.c >> +++ b/migration/savevm.c >> @@ -1342,10 +1342,10 @@ void qemu_savevm_state_setup(QEMUFile *f) >> } >> 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, &local_err); >> save_section_footer(f, se); >> if (ret < 0) { >> - qemu_file_set_error(f, ret); >> + qemu_file_set_error_obj(f, ret, local_err); > > Should we set local_err = NULL? possibly, yes. > Because it is re-used a few lines after this, by precopy_notify(). I wonder why is precopy_notify(PRECOPY_NOTIFY_SETUP) even called when there was an error in one of the save_setup() handlers. It probably shouldn't and qemu_savevm_state_setup() should return at the first error in the loop. This is something that could have been overlooked by commit bd2270608fa0 "migration/ram.c: add a notifier chain for precopy" because qemu_savevm_state_setup() does not have a return value. Probably because the callers rely on qemu_file_get_error() to know if something wrong happened. Also, the only user of PRECOPY_NOTIFY_SETUP is virtio-balloon and nothing is done. PrecopyNotifyData has an errp attribute which is unused. > > BTW, I think that if we add Error** parameter to functions we must make sure all their error flows set errp as well. > According to Error API: > * - On success, the function should not touch *errp. On failure, it > * should set a new error, e.g. with error_setg(errp, ...), or > * propagate an existing one, e.g. with error_propagate(errp, ...). > > For example, a caller that handles errors by printing them with error_report_err() would crash when trying to access NULL error object (if some error path didn't set errp). One of the underlying goal is to avoid and remove all error_report_err() calls to propagate the error up the call stack. > If you agree, we should check it throughout the series. I do agree and this is a can of worms ! I haven't quite found my way around yet. Thanks for your inputs, C.
On 12/02/2024 16:49, Cédric Le Goater wrote: > External email: Use caution opening links or attachments > > > On 2/12/24 09:36, Avihai Horon wrote: >> Hi, Cedric >> >> On 07/02/2024 15:33, Cédric Le Goater wrote: >>> External email: Use caution opening links or attachments >>> >>> >>> 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(). The following patches >>> will introduce such changes for VFIO first. >>> >>> Signed-off-by: Cédric Le Goater <clg@redhat.com> >>> --- >>> include/migration/register.h | 2 +- >>> hw/ppc/spapr.c | 2 +- >>> hw/s390x/s390-stattrib.c | 2 +- >>> hw/vfio/migration.c | 2 +- >>> migration/block-dirty-bitmap.c | 2 +- >>> migration/block.c | 2 +- >>> migration/ram.c | 2 +- >>> migration/savevm.c | 4 ++-- >>> 8 files changed, 9 insertions(+), 9 deletions(-) >>> >>> diff --git a/include/migration/register.h >>> b/include/migration/register.h >>> index >>> 9ab1f79512c605f0c88a45b560c57486fa054441..831600a00eae4efd0464b60925d65de4d9dbcff8 >>> 100644 >>> --- a/include/migration/register.h >>> +++ b/include/migration/register.h >>> @@ -25,7 +25,7 @@ typedef struct SaveVMHandlers { >>> * used to perform early checks. >>> */ >>> int (*save_prepare)(void *opaque, Error **errp); >>> - int (*save_setup)(QEMUFile *f, void *opaque); >>> + int (*save_setup)(QEMUFile *f, void *opaque, Error **errp); >>> void (*save_cleanup)(void *opaque); >>> int (*save_live_complete_postcopy)(QEMUFile *f, void *opaque); >>> int (*save_live_complete_precopy)(QEMUFile *f, void *opaque); >>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c >>> index >>> 0d72d286d80f0435122593555f79fae4d90acf81..a1b0aa02582ad2d68a13476c1859b18143da7bb8 >>> 100644 >>> --- a/hw/ppc/spapr.c >>> +++ b/hw/ppc/spapr.c >>> @@ -2142,7 +2142,7 @@ static const VMStateDescription vmstate_spapr = { >>> } >>> }; >>> >>> -static int htab_save_setup(QEMUFile *f, void *opaque) >>> +static int htab_save_setup(QEMUFile *f, void *opaque, Error **errp) >>> { >>> SpaprMachineState *spapr = opaque; >>> >>> diff --git a/hw/s390x/s390-stattrib.c b/hw/s390x/s390-stattrib.c >>> index >>> c483b62a9b5f71772639fc180bdad15ecb6711cb..c934df424a555d83d2198f5ddfc0cbe0ea98e9ec >>> 100644 >>> --- a/hw/s390x/s390-stattrib.c >>> +++ b/hw/s390x/s390-stattrib.c >>> @@ -166,7 +166,7 @@ static int cmma_load(QEMUFile *f, void *opaque, >>> int version_id) >>> return ret; >>> } >>> >>> -static int cmma_save_setup(QEMUFile *f, void *opaque) >>> +static int cmma_save_setup(QEMUFile *f, void *opaque, Error **errp) >>> { >>> S390StAttribState *sas = S390_STATTRIB(opaque); >>> S390StAttribClass *sac = S390_STATTRIB_GET_CLASS(sas); >>> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c >>> index >>> 70e6b1a709f9b67e4c9eb41033d76347275cac42..8bcb4bc73cd5ba5338e3ffa4d907d0e6bfbb9485 >>> 100644 >>> --- a/hw/vfio/migration.c >>> +++ b/hw/vfio/migration.c >>> @@ -378,7 +378,7 @@ static int vfio_save_prepare(void *opaque, Error >>> **errp) >>> return 0; >>> } >>> >>> -static int vfio_save_setup(QEMUFile *f, void *opaque) >>> +static int vfio_save_setup(QEMUFile *f, void *opaque, Error **errp) >>> { >>> VFIODevice *vbasedev = opaque; >>> VFIOMigration *migration = vbasedev->migration; >>> diff --git a/migration/block-dirty-bitmap.c >>> b/migration/block-dirty-bitmap.c >>> index >>> 2708abf3d762de774ed294d3fdb8e56690d2974c..16f84e6c57c2403a8c2d6319f4e7b6360dade28c >>> 100644 >>> --- a/migration/block-dirty-bitmap.c >>> +++ b/migration/block-dirty-bitmap.c >>> @@ -1213,7 +1213,7 @@ fail: >>> return ret; >>> } >>> >>> -static int dirty_bitmap_save_setup(QEMUFile *f, void *opaque) >>> +static int dirty_bitmap_save_setup(QEMUFile *f, void *opaque, Error >>> **errp) >>> { >>> DBMSaveState *s = &((DBMState *)opaque)->save; >>> SaveBitmapState *dbms = NULL; >>> diff --git a/migration/block.c b/migration/block.c >>> index >>> 8c6ebafacc1ffe930d1d4f19d968817b14852c69..df15319ceab66201b043f15eac1b0a7d6522b60c >>> 100644 >>> --- a/migration/block.c >>> +++ b/migration/block.c >>> @@ -708,7 +708,7 @@ static void block_migration_cleanup(void *opaque) >>> blk_mig_unlock(); >>> } >>> >>> -static int block_save_setup(QEMUFile *f, void *opaque) >>> +static int block_save_setup(QEMUFile *f, void *opaque, Error **errp) >>> { >>> int ret; >>> >>> diff --git a/migration/ram.c b/migration/ram.c >>> index >>> d5b7cd5ac2f31aabf4a248b966153401c48912cf..136c237f4079f68d4e578cf1c72eec2efc815bc8 >>> 100644 >>> --- a/migration/ram.c >>> +++ b/migration/ram.c >>> @@ -2931,7 +2931,7 @@ void qemu_guest_free_page_hint(void *addr, >>> size_t len) >>> * @f: QEMUFile where to send the data >>> * @opaque: RAMState pointer >>> */ >>> -static int ram_save_setup(QEMUFile *f, void *opaque) >>> +static int ram_save_setup(QEMUFile *f, void *opaque, Error **errp) >>> { >>> RAMState **rsp = opaque; >>> RAMBlock *block; >>> diff --git a/migration/savevm.c b/migration/savevm.c >>> index >>> d612c8a9020b204d5d078d5df85f0e6449c27645..f2ae799bad13e631bccf733a34c3a8fd22e8dd48 >>> 100644 >>> --- a/migration/savevm.c >>> +++ b/migration/savevm.c >>> @@ -1342,10 +1342,10 @@ void qemu_savevm_state_setup(QEMUFile *f) >>> } >>> 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, &local_err); >>> save_section_footer(f, se); >>> if (ret < 0) { >>> - qemu_file_set_error(f, ret); >>> + qemu_file_set_error_obj(f, ret, local_err); >> >> Should we set local_err = NULL? > > possibly, yes. > >> Because it is re-used a few lines after this, by precopy_notify(). > > I wonder why is precopy_notify(PRECOPY_NOTIFY_SETUP) even called when > there was an error in one of the save_setup() handlers. It probably > shouldn't and qemu_savevm_state_setup() should return at the first > error in the loop. This is something that could have been overlooked > by commit bd2270608fa0 "migration/ram.c: add a notifier chain for > precopy" because qemu_savevm_state_setup() does not have a return > value. Probably because the callers rely on qemu_file_get_error() > to know if something wrong happened. Yes, I guess here we could return early and skip precopy_notify(). > > Also, the only user of PRECOPY_NOTIFY_SETUP is virtio-balloon and > nothing is done. PrecopyNotifyData has an errp attribute which is > unused. You are right, with current code there won't be any problem, but new code that will make use of the errp can be problematic. > >> >> BTW, I think that if we add Error** parameter to functions we must >> make sure all their error flows set errp as well. >> According to Error API: >> * - On success, the function should not touch *errp. On failure, it >> * should set a new error, e.g. with error_setg(errp, ...), or >> * propagate an existing one, e.g. with error_propagate(errp, ...). >> >> For example, a caller that handles errors by printing them with >> error_report_err() would crash when trying to access NULL error >> object (if some error path didn't set errp). > > One of the underlying goal is to avoid and remove all error_report_err() > calls to propagate the error up the call stack. Yes, I just gave an example of how this could go wrong. I think the general point here is that a caller that provides a valid Error** errp assumes he will get an Error object from the callee in case of an error, and the caller can operate on it as he wants. > >> If you agree, we should check it throughout the series. > > I do agree and this is a can of worms ! Indeed. > I haven't quite found my way > around yet. Briefly looking, it seems like .save_setup() / .load_setup() shouldn't be too hard. However, memory stuff seem to be more involved.
diff --git a/include/migration/register.h b/include/migration/register.h index 9ab1f79512c605f0c88a45b560c57486fa054441..831600a00eae4efd0464b60925d65de4d9dbcff8 100644 --- a/include/migration/register.h +++ b/include/migration/register.h @@ -25,7 +25,7 @@ typedef struct SaveVMHandlers { * used to perform early checks. */ int (*save_prepare)(void *opaque, Error **errp); - int (*save_setup)(QEMUFile *f, void *opaque); + int (*save_setup)(QEMUFile *f, void *opaque, Error **errp); void (*save_cleanup)(void *opaque); int (*save_live_complete_postcopy)(QEMUFile *f, void *opaque); int (*save_live_complete_precopy)(QEMUFile *f, void *opaque); diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index 0d72d286d80f0435122593555f79fae4d90acf81..a1b0aa02582ad2d68a13476c1859b18143da7bb8 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -2142,7 +2142,7 @@ static const VMStateDescription vmstate_spapr = { } }; -static int htab_save_setup(QEMUFile *f, void *opaque) +static int htab_save_setup(QEMUFile *f, void *opaque, Error **errp) { SpaprMachineState *spapr = opaque; diff --git a/hw/s390x/s390-stattrib.c b/hw/s390x/s390-stattrib.c index c483b62a9b5f71772639fc180bdad15ecb6711cb..c934df424a555d83d2198f5ddfc0cbe0ea98e9ec 100644 --- a/hw/s390x/s390-stattrib.c +++ b/hw/s390x/s390-stattrib.c @@ -166,7 +166,7 @@ static int cmma_load(QEMUFile *f, void *opaque, int version_id) return ret; } -static int cmma_save_setup(QEMUFile *f, void *opaque) +static int cmma_save_setup(QEMUFile *f, void *opaque, Error **errp) { S390StAttribState *sas = S390_STATTRIB(opaque); S390StAttribClass *sac = S390_STATTRIB_GET_CLASS(sas); diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c index 70e6b1a709f9b67e4c9eb41033d76347275cac42..8bcb4bc73cd5ba5338e3ffa4d907d0e6bfbb9485 100644 --- a/hw/vfio/migration.c +++ b/hw/vfio/migration.c @@ -378,7 +378,7 @@ static int vfio_save_prepare(void *opaque, Error **errp) return 0; } -static int vfio_save_setup(QEMUFile *f, void *opaque) +static int vfio_save_setup(QEMUFile *f, void *opaque, Error **errp) { VFIODevice *vbasedev = opaque; VFIOMigration *migration = vbasedev->migration; diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c index 2708abf3d762de774ed294d3fdb8e56690d2974c..16f84e6c57c2403a8c2d6319f4e7b6360dade28c 100644 --- a/migration/block-dirty-bitmap.c +++ b/migration/block-dirty-bitmap.c @@ -1213,7 +1213,7 @@ fail: return ret; } -static int dirty_bitmap_save_setup(QEMUFile *f, void *opaque) +static int dirty_bitmap_save_setup(QEMUFile *f, void *opaque, Error **errp) { DBMSaveState *s = &((DBMState *)opaque)->save; SaveBitmapState *dbms = NULL; diff --git a/migration/block.c b/migration/block.c index 8c6ebafacc1ffe930d1d4f19d968817b14852c69..df15319ceab66201b043f15eac1b0a7d6522b60c 100644 --- a/migration/block.c +++ b/migration/block.c @@ -708,7 +708,7 @@ static void block_migration_cleanup(void *opaque) blk_mig_unlock(); } -static int block_save_setup(QEMUFile *f, void *opaque) +static int block_save_setup(QEMUFile *f, void *opaque, Error **errp) { int ret; diff --git a/migration/ram.c b/migration/ram.c index d5b7cd5ac2f31aabf4a248b966153401c48912cf..136c237f4079f68d4e578cf1c72eec2efc815bc8 100644 --- a/migration/ram.c +++ b/migration/ram.c @@ -2931,7 +2931,7 @@ void qemu_guest_free_page_hint(void *addr, size_t len) * @f: QEMUFile where to send the data * @opaque: RAMState pointer */ -static int ram_save_setup(QEMUFile *f, void *opaque) +static int ram_save_setup(QEMUFile *f, void *opaque, Error **errp) { RAMState **rsp = opaque; RAMBlock *block; diff --git a/migration/savevm.c b/migration/savevm.c index d612c8a9020b204d5d078d5df85f0e6449c27645..f2ae799bad13e631bccf733a34c3a8fd22e8dd48 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -1342,10 +1342,10 @@ void qemu_savevm_state_setup(QEMUFile *f) } 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, &local_err); save_section_footer(f, se); if (ret < 0) { - qemu_file_set_error(f, ret); + qemu_file_set_error_obj(f, ret, local_err); break; } }
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(). The following patches will introduce such changes for VFIO first. Signed-off-by: Cédric Le Goater <clg@redhat.com> --- include/migration/register.h | 2 +- hw/ppc/spapr.c | 2 +- hw/s390x/s390-stattrib.c | 2 +- hw/vfio/migration.c | 2 +- migration/block-dirty-bitmap.c | 2 +- migration/block.c | 2 +- migration/ram.c | 2 +- migration/savevm.c | 4 ++-- 8 files changed, 9 insertions(+), 9 deletions(-)