Re: [PATCH v3 01/26] s390/stattrib: Add Error** argument to set_migrationmode() handler

[Avihai Horon]

On 04/03/2024 14:28, Cédric Le Goater wrote:

External email: Use caution opening links or attachments


This will prepare ground for futur changes adding an Error** argument
to the save_setup() handler. We need to make sure that on failure,
set_migrationmode() always sets a new error. See the Rules section in
qapi/error.h.

Cc: Halil Pasic <pa...@linux.ibm.com>
Cc: Christian Borntraeger <borntrae...@linux.ibm.com>
Cc: Thomas Huth <th...@redhat.com>
Signed-off-by: Cédric Le Goater <c...@redhat.com>
---
  include/hw/s390x/storage-attributes.h |  2 +-
  hw/s390x/s390-stattrib-kvm.c          | 12 ++++++++++--
  hw/s390x/s390-stattrib.c              | 14 +++++++++-----
  3 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/include/hw/s390x/storage-attributes.h 
b/include/hw/s390x/storage-attributes.h
index 
5239eb538c1b087797867a247abfc14551af6a4d..8921a04d514bf64a3113255ee10ed33fc598ae06
 100644
--- a/include/hw/s390x/storage-attributes.h
+++ b/include/hw/s390x/storage-attributes.h
@@ -39,7 +39,7 @@ struct S390StAttribClass {
      int (*set_stattr)(S390StAttribState *sa, uint64_t start_gfn,
                        uint32_t count, uint8_t *values);
      void (*synchronize)(S390StAttribState *sa);
-    int (*set_migrationmode)(S390StAttribState *sa, bool value);
+    int (*set_migrationmode)(S390StAttribState *sa, bool value, Error **errp);
      int (*get_active)(S390StAttribState *sa);
      long long (*get_dirtycount)(S390StAttribState *sa);
  };
diff --git a/hw/s390x/s390-stattrib-kvm.c b/hw/s390x/s390-stattrib-kvm.c
index 
24cd01382e2d74d62c2d7e980eb6aca1077d893d..357cea2c987213b867c81b0e258f7d0c293fe665
 100644
--- a/hw/s390x/s390-stattrib-kvm.c
+++ b/hw/s390x/s390-stattrib-kvm.c
@@ -17,6 +17,7 @@
  #include "sysemu/kvm.h"
  #include "exec/ram_addr.h"
  #include "kvm/kvm_s390x.h"
+#include "qapi/error.h"

  Object *kvm_s390_stattrib_create(void)
  {
@@ -137,14 +138,21 @@ static void 
kvm_s390_stattrib_synchronize(S390StAttribState *sa)
      }
  }

-static int kvm_s390_stattrib_set_migrationmode(S390StAttribState *sa, bool val)
+static int kvm_s390_stattrib_set_migrationmode(S390StAttribState *sa, bool val,
+                                               Error **errp)
  {
      struct kvm_device_attr attr = {
          .group = KVM_S390_VM_MIGRATION,
          .attr = val,
          .addr = 0,
      };
-    return kvm_vm_ioctl(kvm_state, KVM_SET_DEVICE_ATTR, &attr);
+    int r;
+
+    r = kvm_vm_ioctl(kvm_state, KVM_SET_DEVICE_ATTR, &attr);
+    if (r) {
+        error_setg_errno(errp, -r, "KVM_S390_SET_CMMA_BITS failed");
+    }
+    return r;
  }

  static long long kvm_s390_stattrib_get_dirtycount(S390StAttribState *sa)
diff --git a/hw/s390x/s390-stattrib.c b/hw/s390x/s390-stattrib.c
index 
c483b62a9b5f71772639fc180bdad15ecb6711cb..e99de190332a82363b1388bbc450013149295bc0
 100644
--- a/hw/s390x/s390-stattrib.c
+++ b/hw/s390x/s390-stattrib.c
@@ -60,11 +60,12 @@ void hmp_migrationmode(Monitor *mon, const QDict *qdict)
      S390StAttribState *sas = s390_get_stattrib_device();
      S390StAttribClass *sac = S390_STATTRIB_GET_CLASS(sas);
      uint64_t what = qdict_get_int(qdict, "mode");
+    Error *local_err = NULL;
      int r;

-    r = sac->set_migrationmode(sas, what);
+    r = sac->set_migrationmode(sas, what, &local_err);
      if (r < 0) {
-        monitor_printf(mon, "Error: %s", strerror(-r));
+        monitor_printf(mon, "Error: %s", error_get_pretty(local_err));

I think we need to free the error here:
error_free(local_err);

Thanks.

      }
  }

@@ -170,13 +171,15 @@ static int cmma_save_setup(QEMUFile *f, void *opaque)
  {
      S390StAttribState *sas = S390_STATTRIB(opaque);
      S390StAttribClass *sac = S390_STATTRIB_GET_CLASS(sas);
+    Error *local_err = NULL;
      int res;
      /*
       * Signal that we want to start a migration, thus needing PGSTE dirty
       * tracking.
       */
-    res = sac->set_migrationmode(sas, 1);
+    res = sac->set_migrationmode(sas, true, &local_err);
      if (res) {
+        error_report_err(local_err);
          return res;
      }
      qemu_put_be64(f, STATTR_FLAG_EOS);
@@ -260,7 +263,7 @@ static void cmma_save_cleanup(void *opaque)
  {
      S390StAttribState *sas = S390_STATTRIB(opaque);
      S390StAttribClass *sac = S390_STATTRIB_GET_CLASS(sas);
-    sac->set_migrationmode(sas, 0);
+    sac->set_migrationmode(sas, false, NULL);
  }

  static bool cmma_active(void *opaque)
@@ -293,7 +296,8 @@ static long long 
qemu_s390_get_dirtycount_stub(S390StAttribState *sa)
  {
      return 0;
  }
-static int qemu_s390_set_migrationmode_stub(S390StAttribState *sa, bool value)
+static int qemu_s390_set_migrationmode_stub(S390StAttribState *sa, bool value,
+                                            Error **errp)
  {
      return 0;
  }
--
2.44.0