Re: [PATCH 01/14] migration: Add Error** argument to .save_setup() handler

Mon, 12 Feb 2024 07:58:44 -0800 


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 <c...@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.

























					Reply via email to