diff mbox series

[v2,4/4] migration: Allow postcopy_ram_supported_by_host() to report err

Message ID 20230419161739.1129988-5-peterx@redhat.com (mailing list archive)
State New, archived
Headers show
Series migration/hostmem: Allow to fail early for postcopy on specific fs type | expand

Commit Message

Peter Xu April 19, 2023, 4:17 p.m. UTC 
Instead of print it to STDERR, bring the error upwards so that it can be
reported via QMP responses.

E.g.:

{ "execute": "migrate-set-capabilities" ,
  "arguments": { "capabilities":
  [ { "capability": "postcopy-ram", "state": true } ] } }

{ "error":
  { "class": "GenericError",
    "desc": "Postcopy is not supported: Host backend files need to be TMPFS
    or HUGETLBFS only" } }

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.c    |  9 +++---
 migration/postcopy-ram.c | 61 ++++++++++++++++++++++------------------
 migration/postcopy-ram.h |  3 +-
 migration/savevm.c       |  3 +-
 4 files changed, 42 insertions(+), 34 deletions(-)

Comments

Juan Quintela April 19, 2023, 7:51 p.m. UTC | #1 
Peter Xu <peterx@redhat.com> wrote:
> Instead of print it to STDERR, bring the error upwards so that it can be
> reported via QMP responses.
>
> E.g.:
>
> { "execute": "migrate-set-capabilities" ,
>   "arguments": { "capabilities":
>   [ { "capability": "postcopy-ram", "state": true } ] } }
>
> { "error":
>   { "class": "GenericError",
>     "desc": "Postcopy is not supported: Host backend files need to be TMPFS
>     or HUGETLBFS only" } }
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/migration.c    |  9 +++---
>  migration/postcopy-ram.c | 61 ++++++++++++++++++++++------------------
>  migration/postcopy-ram.h |  3 +-
>  migration/savevm.c       |  3 +-
>  4 files changed, 42 insertions(+), 34 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index bda4789193..ac15fa6092 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1313,6 +1313,7 @@ static bool migrate_caps_check(bool *cap_list,
>      MigrationCapabilityStatusList *cap;
>      bool old_postcopy_cap;
>      MigrationIncomingState *mis = migration_incoming_get_current();
> +    Error *local_err = NULL;

This variable can be declared in the only block that uses it.

> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> index bbb8af61ae..0713ddeeef 100644
> --- a/migration/postcopy-ram.c
> +++ b/migration/postcopy-ram.c
> @@ -282,11 +282,14 @@ static bool request_ufd_features(int ufd, uint64_t features)
>      return true;
>  }
>  
> -static bool ufd_check_and_apply(int ufd, MigrationIncomingState *mis)
> +static bool ufd_check_and_apply(int ufd, MigrationIncomingState *mis,
> +                                Error **errp)
>  {
>      uint64_t asked_features = 0;
>      static uint64_t supported_features;
>  
> +    assert(errp);
> +

Is this right?  My impression was that you have to live with errp being NULL.

error_setg() knows how to handle it being NULL:

error_setg() -> error_setg_internal() -> error_setv()

static void error_setv(Error **errp,
                       const char *src, int line, const char *func,
                       ErrorClass err_class, const char *fmt, va_list ap,
                       const char *suffix)
{
    ....
    if (errp == NULL) {
        return;
    }


> -static int test_ramblock_postcopiable(RAMBlock *rb)
> +static int test_ramblock_postcopiable(RAMBlock *rb, Error **errp)

In patch 3 you do this other change:

-static int test_ramblock_postcopiable(RAMBlock *rb, void *opaque)
+static int test_ramblock_postcopiable(RAMBlock *rb)

Can you do with a single change?

The idea of the patch is right.

Later, Juan.
Peter Xu April 26, 2023, 1:10 a.m. UTC | #2 
On Wed, Apr 19, 2023 at 09:51:24PM +0200, Juan Quintela wrote:
> Peter Xu <peterx@redhat.com> wrote:
> > Instead of print it to STDERR, bring the error upwards so that it can be
> > reported via QMP responses.
> >
> > E.g.:
> >
> > { "execute": "migrate-set-capabilities" ,
> >   "arguments": { "capabilities":
> >   [ { "capability": "postcopy-ram", "state": true } ] } }
> >
> > { "error":
> >   { "class": "GenericError",
> >     "desc": "Postcopy is not supported: Host backend files need to be TMPFS
> >     or HUGETLBFS only" } }
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  migration/migration.c    |  9 +++---
> >  migration/postcopy-ram.c | 61 ++++++++++++++++++++++------------------
> >  migration/postcopy-ram.h |  3 +-
> >  migration/savevm.c       |  3 +-
> >  4 files changed, 42 insertions(+), 34 deletions(-)
> >
> > diff --git a/migration/migration.c b/migration/migration.c
> > index bda4789193..ac15fa6092 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -1313,6 +1313,7 @@ static bool migrate_caps_check(bool *cap_list,
> >      MigrationCapabilityStatusList *cap;
> >      bool old_postcopy_cap;
> >      MigrationIncomingState *mis = migration_incoming_get_current();
> > +    Error *local_err = NULL;
> 
> This variable can be declared in the only block that uses it.

Sure, though I just remembered I can use ERRP_GUARD(), hence I'll go with
that.

> 
> > diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> > index bbb8af61ae..0713ddeeef 100644
> > --- a/migration/postcopy-ram.c
> > +++ b/migration/postcopy-ram.c
> > @@ -282,11 +282,14 @@ static bool request_ufd_features(int ufd, uint64_t features)
> >      return true;
> >  }
> >  
> > -static bool ufd_check_and_apply(int ufd, MigrationIncomingState *mis)
> > +static bool ufd_check_and_apply(int ufd, MigrationIncomingState *mis,
> > +                                Error **errp)
> >  {
> >      uint64_t asked_features = 0;
> >      static uint64_t supported_features;
> >  
> > +    assert(errp);
> > +
> 
> Is this right?  My impression was that you have to live with errp being NULL.

Right it knows, I just wanted to make sure error msg got captured, but we
don't really need to worrry here, all callers are in this case I believe.

Let me also switch to ERRP_GUARD() just to still avoid error_abort being
passed in, so we can still abort with a full message.

> 
> error_setg() knows how to handle it being NULL:
> 
> error_setg() -> error_setg_internal() -> error_setv()
> 
> static void error_setv(Error **errp,
>                        const char *src, int line, const char *func,
>                        ErrorClass err_class, const char *fmt, va_list ap,
>                        const char *suffix)
> {
>     ....
>     if (errp == NULL) {
>         return;
>     }
> 
> 
> > -static int test_ramblock_postcopiable(RAMBlock *rb)
> > +static int test_ramblock_postcopiable(RAMBlock *rb, Error **errp)
> 
> In patch 3 you do this other change:
> 
> -static int test_ramblock_postcopiable(RAMBlock *rb, void *opaque)
> +static int test_ramblock_postcopiable(RAMBlock *rb)
> 
> Can you do with a single change?
> 
> The idea of the patch is right.

I saw that patch 3 already merged.  I'll just spin this one, thanks!
diff mbox series

Patch

diff --git a/migration/migration.c b/migration/migration.c
index bda4789193..ac15fa6092 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1313,6 +1313,7 @@  static bool migrate_caps_check(bool *cap_list,
     MigrationCapabilityStatusList *cap;
     bool old_postcopy_cap;
     MigrationIncomingState *mis = migration_incoming_get_current();
+    Error *local_err = NULL;
 
     old_postcopy_cap = cap_list[MIGRATION_CAPABILITY_POSTCOPY_RAM];
 
@@ -1344,11 +1345,9 @@  static bool migrate_caps_check(bool *cap_list,
          * special support.
          */
         if (!old_postcopy_cap && runstate_check(RUN_STATE_INMIGRATE) &&
-            !postcopy_ram_supported_by_host(mis)) {
-            /* postcopy_ram_supported_by_host will have emitted a more
-             * detailed message
-             */
-            error_setg(errp, "Postcopy is not supported");
+            !postcopy_ram_supported_by_host(mis, &local_err)) {
+            error_propagate_prepend(errp, local_err,
+                                    "Postcopy is not supported: ");
             return false;
         }
 
diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
index bbb8af61ae..0713ddeeef 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
@@ -282,11 +282,14 @@  static bool request_ufd_features(int ufd, uint64_t features)
     return true;
 }
 
-static bool ufd_check_and_apply(int ufd, MigrationIncomingState *mis)
+static bool ufd_check_and_apply(int ufd, MigrationIncomingState *mis,
+                                Error **errp)
 {
     uint64_t asked_features = 0;
     static uint64_t supported_features;
 
+    assert(errp);
+
     /*
      * it's not possible to
      * request UFFD_API twice per one fd
@@ -294,7 +297,7 @@  static bool ufd_check_and_apply(int ufd, MigrationIncomingState *mis)
      */
     if (!supported_features) {
         if (!receive_ufd_features(&supported_features)) {
-            error_report("%s failed", __func__);
+            error_setg(errp, "Userfault feature detection failed");
             return false;
         }
     }
@@ -316,8 +319,7 @@  static bool ufd_check_and_apply(int ufd, MigrationIncomingState *mis)
      * userfault file descriptor
      */
     if (!request_ufd_features(ufd, asked_features)) {
-        error_report("%s failed: features %" PRIu64, __func__,
-                     asked_features);
+        error_setg(errp, "Failed features %" PRIu64, asked_features);
         return false;
     }
 
@@ -328,7 +330,8 @@  static bool ufd_check_and_apply(int ufd, MigrationIncomingState *mis)
         have_hp = supported_features & UFFD_FEATURE_MISSING_HUGETLBFS;
 #endif
         if (!have_hp) {
-            error_report("Userfault on this host does not support huge pages");
+            error_setg(errp,
+                       "Userfault on this host does not support huge pages");
             return false;
         }
     }
@@ -337,7 +340,7 @@  static bool ufd_check_and_apply(int ufd, MigrationIncomingState *mis)
 
 /* Callback from postcopy_ram_supported_by_host block iterator.
  */
-static int test_ramblock_postcopiable(RAMBlock *rb)
+static int test_ramblock_postcopiable(RAMBlock *rb, Error **errp)
 {
     const char *block_name = qemu_ram_get_idstr(rb);
     ram_addr_t length = qemu_ram_get_used_length(rb);
@@ -345,16 +348,18 @@  static int test_ramblock_postcopiable(RAMBlock *rb)
     QemuFsType fs;
 
     if (length % pagesize) {
-        error_report("Postcopy requires RAM blocks to be a page size multiple,"
-                     " block %s is 0x" RAM_ADDR_FMT " bytes with a "
-                     "page size of 0x%zx", block_name, length, pagesize);
+        error_setg(errp,
+                   "Postcopy requires RAM blocks to be a page size multiple,"
+                   " block %s is 0x" RAM_ADDR_FMT " bytes with a "
+                   "page size of 0x%zx", block_name, length, pagesize);
         return 1;
     }
 
     if (rb->fd >= 0) {
         fs = qemu_fd_getfs(rb->fd);
         if (fs != QEMU_FS_TYPE_TMPFS && fs != QEMU_FS_TYPE_HUGETLBFS) {
-            error_report("Host backend files need to be TMPFS or HUGETLBFS only");
+            error_setg(errp,
+                       "Host backend files need to be TMPFS or HUGETLBFS only");
             return 1;
         }
     }
@@ -367,7 +372,8 @@  static int test_ramblock_postcopiable(RAMBlock *rb)
  * normally fine since if the postcopy succeeds it gets turned back on at the
  * end.
  */
-bool postcopy_ram_supported_by_host(MigrationIncomingState *mis)
+bool postcopy_ram_supported_by_host(MigrationIncomingState *mis,
+                                    Error **errp)
 {
     long pagesize = qemu_real_host_page_size();
     int ufd = -1;
@@ -376,29 +382,28 @@  bool postcopy_ram_supported_by_host(MigrationIncomingState *mis)
     struct uffdio_register reg_struct;
     struct uffdio_range range_struct;
     uint64_t feature_mask;
-    Error *local_err = NULL;
     RAMBlock *block;
 
+    assert(errp);
+
     if (qemu_target_page_size() > pagesize) {
-        error_report("Target page size bigger than host page size");
+        error_setg(errp, "Target page size bigger than host page size");
         goto out;
     }
 
     ufd = uffd_open(O_CLOEXEC);
     if (ufd == -1) {
-        error_report("%s: userfaultfd not available: %s", __func__,
-                     strerror(errno));
+        error_setg(errp, "Userfaultfd not available: %s", strerror(errno));
         goto out;
     }
 
     /* Give devices a chance to object */
-    if (postcopy_notify(POSTCOPY_NOTIFY_PROBE, &local_err)) {
-        error_report_err(local_err);
+    if (postcopy_notify(POSTCOPY_NOTIFY_PROBE, errp)) {
         goto out;
     }
 
     /* Version and features check */
-    if (!ufd_check_and_apply(ufd, mis)) {
+    if (!ufd_check_and_apply(ufd, mis, errp)) {
         goto out;
     }
 
@@ -416,7 +421,7 @@  bool postcopy_ram_supported_by_host(MigrationIncomingState *mis)
      * affect in reality, or we can revisit.
      */
     RAMBLOCK_FOREACH(block) {
-        if (test_ramblock_postcopiable(block)) {
+        if (test_ramblock_postcopiable(block, errp)) {
             goto out;
         }
     }
@@ -426,7 +431,7 @@  bool postcopy_ram_supported_by_host(MigrationIncomingState *mis)
      * it was enabled.
      */
     if (munlockall()) {
-        error_report("%s: munlockall: %s", __func__,  strerror(errno));
+        error_setg(errp, "munlockall() failed: %s", strerror(errno));
         goto out;
     }
 
@@ -438,8 +443,7 @@  bool postcopy_ram_supported_by_host(MigrationIncomingState *mis)
     testarea = mmap(NULL, pagesize, PROT_READ | PROT_WRITE, MAP_PRIVATE |
                                     MAP_ANONYMOUS, -1, 0);
     if (testarea == MAP_FAILED) {
-        error_report("%s: Failed to map test area: %s", __func__,
-                     strerror(errno));
+        error_setg(errp, "Failed to map test area: %s", strerror(errno));
         goto out;
     }
     g_assert(QEMU_PTR_IS_ALIGNED(testarea, pagesize));
@@ -449,14 +453,14 @@  bool postcopy_ram_supported_by_host(MigrationIncomingState *mis)
     reg_struct.mode = UFFDIO_REGISTER_MODE_MISSING;
 
     if (ioctl(ufd, UFFDIO_REGISTER, &reg_struct)) {
-        error_report("%s userfault register: %s", __func__, strerror(errno));
+        error_setg(errp, "UFFDIO_REGISTER failed: %s", strerror(errno));
         goto out;
     }
 
     range_struct.start = (uintptr_t)testarea;
     range_struct.len = pagesize;
     if (ioctl(ufd, UFFDIO_UNREGISTER, &range_struct)) {
-        error_report("%s userfault unregister: %s", __func__, strerror(errno));
+        error_setg(errp, "UFFDIO_UNREGISTER failed: %s", strerror(errno));
         goto out;
     }
 
@@ -464,8 +468,8 @@  bool postcopy_ram_supported_by_host(MigrationIncomingState *mis)
                    (__u64)1 << _UFFDIO_COPY |
                    (__u64)1 << _UFFDIO_ZEROPAGE;
     if ((reg_struct.ioctls & feature_mask) != feature_mask) {
-        error_report("Missing userfault map features: %" PRIx64,
-                     (uint64_t)(~reg_struct.ioctls & feature_mask));
+        error_setg(errp, "Missing userfault map features: %" PRIx64,
+                   (uint64_t)(~reg_struct.ioctls & feature_mask));
         goto out;
     }
 
@@ -1187,6 +1191,8 @@  static int postcopy_temp_pages_setup(MigrationIncomingState *mis)
 
 int postcopy_ram_incoming_setup(MigrationIncomingState *mis)
 {
+    Error *local_err = NULL;
+
     /* Open the fd for the kernel to give us userfaults */
     mis->userfault_fd = uffd_open(O_CLOEXEC | O_NONBLOCK);
     if (mis->userfault_fd == -1) {
@@ -1199,7 +1205,8 @@  int postcopy_ram_incoming_setup(MigrationIncomingState *mis)
      * Although the host check already tested the API, we need to
      * do the check again as an ABI handshake on the new fd.
      */
-    if (!ufd_check_and_apply(mis->userfault_fd, mis)) {
+    if (!ufd_check_and_apply(mis->userfault_fd, mis, &local_err)) {
+        error_report_err(local_err);
         return -1;
     }
 
diff --git a/migration/postcopy-ram.h b/migration/postcopy-ram.h
index b4867a32d5..442ab89752 100644
--- a/migration/postcopy-ram.h
+++ b/migration/postcopy-ram.h
@@ -14,7 +14,8 @@ 
 #define QEMU_POSTCOPY_RAM_H
 
 /* Return true if the host supports everything we need to do postcopy-ram */
-bool postcopy_ram_supported_by_host(MigrationIncomingState *mis);
+bool postcopy_ram_supported_by_host(MigrationIncomingState *mis,
+                                    Error **errp);
 
 /*
  * Make all of RAM sensitive to accesses to areas that haven't yet been written
diff --git a/migration/savevm.c b/migration/savevm.c
index aa54a67fda..0d61ab6c19 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1752,7 +1752,8 @@  static int loadvm_postcopy_handle_advise(MigrationIncomingState *mis,
         return -EINVAL;
     }
 
-    if (!postcopy_ram_supported_by_host(mis)) {
+    if (!postcopy_ram_supported_by_host(mis, &local_err)) {
+        error_report_err(local_err);
         postcopy_state_set(POSTCOPY_INCOMING_NONE);
         return -1;
     }