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);
break;
}
}
--
2.43.0
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.
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>
--
Peter Xu
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 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>
>
© 2016 - 2024 Red Hat, Inc.