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>
>