All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v6 0/5] migration: do not exit on incoming failure
@ 2024-04-30  8:56 Vladimir Sementsov-Ogievskiy
  2024-04-30  8:56 ` [PATCH v6 1/5] migration: move trace-point from migrate_fd_error to migrate_set_error Vladimir Sementsov-Ogievskiy
                   ` (5 more replies)
  0 siblings, 6 replies; 11+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2024-04-30  8:56 UTC (permalink / raw)
  To: peterx, farosas; +Cc: eblake, armbru, pbonzini, qemu-devel, vsementsov, yc-core

Hi all!

The series brings an option to not immediately exit on incoming
migration failure, giving a possibility to orchestrator to get the error
through QAPI and shutdown QEMU by "quit".

v6:
01,02: add r-b by Peter
03: only fix potential use-after-free
04: rework error handling, drop r-b


v5:
- add "migration: process_incoming_migration_co(): fix reporting s->error"

v4:
- add r-b and a-b by Fabiano and Markus
- improve wording in 04 as Markus suggested

v3:
- don't refactor the whole code around setting migration error, it seems
  too much and necessary for the new feature itself
- add constant
- change behavior for HMP command
- split some things to separate patches
- and more, by Peter's suggestions


New behavior can be demonstrated like this:

bash:

(
cat <<EOF
{'execute': 'qmp_capabilities'}
{'execute': 'migrate-set-capabilities', 'arguments': {'capabilities': [{'capability': 'events', 'state': true}]}}
{'execute': 'migrate-incoming', 'arguments': {'uri': 'exec:echo x', 'exit-on-error': false}}
EOF
sleep 1
cat <<EOF
{'execute': 'query-migrate'}
{'execute': 'quit'}
EOF
) | ./build/qemu-system-x86_64 -incoming 'defer' -qmp stdio -nographic -nodefaults

output:

{"QMP": {"version": {"qemu": {"micro": 50, "minor": 0, "major": 9}, "package": "v9.0.0-149-gb6295ad58c"}, "capabilities": ["oob"]}}
{"return": {}}
{"return": {}}
{"timestamp": {"seconds": 1714068847, "microseconds": 263907}, "event": "MIGRATION", "data": {"status": "setup"}}
{"return": {}}
{"timestamp": {"seconds": 1714068847, "microseconds": 266696}, "event": "MIGRATION", "data": {"status": "active"}}
qemu-system-x86_64: Not a migration stream
{"timestamp": {"seconds": 1714068847, "microseconds": 266766}, "event": "MIGRATION", "data": {"status": "failed"}}
{"return": {"status": "failed", "error-desc": "load of migration failed: Invalid argument"}}
{"timestamp": {"seconds": 1714068848, "microseconds": 237187}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}}
{"return": {}}

Vladimir Sementsov-Ogievskiy (5):
  migration: move trace-point from migrate_fd_error to migrate_set_error
  migration: process_incoming_migration_co(): complete cleanup on
    failure
  migration: process_incoming_migration_co(): fix reporting s->error
  migration: process_incoming_migration_co(): rework error reporting
  qapi: introduce exit-on-error parameter for migrate-incoming

 migration/migration-hmp-cmds.c |  2 +-
 migration/migration.c          | 54 ++++++++++++++++++++++++----------
 migration/migration.h          |  3 ++
 migration/trace-events         |  2 +-
 qapi/migration.json            |  7 ++++-
 system/vl.c                    |  3 +-
 6 files changed, 52 insertions(+), 19 deletions(-)

-- 
2.34.1



^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v6 1/5] migration: move trace-point from migrate_fd_error to migrate_set_error
  2024-04-30  8:56 [PATCH v6 0/5] migration: do not exit on incoming failure Vladimir Sementsov-Ogievskiy
@ 2024-04-30  8:56 ` Vladimir Sementsov-Ogievskiy
  2024-04-30  8:56 ` [PATCH v6 2/5] migration: process_incoming_migration_co(): complete cleanup on failure Vladimir Sementsov-Ogievskiy
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 11+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2024-04-30  8:56 UTC (permalink / raw)
  To: peterx, farosas; +Cc: eblake, armbru, pbonzini, qemu-devel, vsementsov, yc-core

Cover more cases by trace-point.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Fabiano Rosas <farosas@suse.de>
Reviewed-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.c  | 4 +++-
 migration/trace-events | 2 +-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index b5af6b5105..2dc6a063e9 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1421,6 +1421,9 @@ static void migrate_fd_cleanup_bh(void *opaque)
 void migrate_set_error(MigrationState *s, const Error *error)
 {
     QEMU_LOCK_GUARD(&s->error_mutex);
+
+    trace_migrate_error(error_get_pretty(error));
+
     if (!s->error) {
         s->error = error_copy(error);
     }
@@ -1444,7 +1447,6 @@ static void migrate_error_free(MigrationState *s)
 
 static void migrate_fd_error(MigrationState *s, const Error *error)
 {
-    trace_migrate_fd_error(error_get_pretty(error));
     assert(s->to_dst_file == NULL);
     migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
                       MIGRATION_STATUS_FAILED);
diff --git a/migration/trace-events b/migration/trace-events
index f0e1cb80c7..d0c44c3853 100644
--- a/migration/trace-events
+++ b/migration/trace-events
@@ -152,7 +152,7 @@ multifd_set_outgoing_channel(void *ioc, const char *ioctype, const char *hostnam
 # migration.c
 migrate_set_state(const char *new_state) "new state %s"
 migrate_fd_cleanup(void) ""
-migrate_fd_error(const char *error_desc) "error=%s"
+migrate_error(const char *error_desc) "error=%s"
 migrate_fd_cancel(void) ""
 migrate_handle_rp_req_pages(const char *rbname, size_t start, size_t len) "in %s at 0x%zx len 0x%zx"
 migrate_pending_exact(uint64_t size, uint64_t pre, uint64_t post) "exact pending size %" PRIu64 " (pre = %" PRIu64 " post=%" PRIu64 ")"
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v6 2/5] migration: process_incoming_migration_co(): complete cleanup on failure
  2024-04-30  8:56 [PATCH v6 0/5] migration: do not exit on incoming failure Vladimir Sementsov-Ogievskiy
  2024-04-30  8:56 ` [PATCH v6 1/5] migration: move trace-point from migrate_fd_error to migrate_set_error Vladimir Sementsov-Ogievskiy
@ 2024-04-30  8:56 ` Vladimir Sementsov-Ogievskiy
  2024-04-30  8:56 ` [PATCH v6 3/5] migration: process_incoming_migration_co(): fix reporting s->error Vladimir Sementsov-Ogievskiy
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 11+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2024-04-30  8:56 UTC (permalink / raw)
  To: peterx, farosas; +Cc: eblake, armbru, pbonzini, qemu-devel, vsementsov, yc-core

Make call to migration_incoming_state_destroy(), instead of doing only
partial of it.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Fabiano Rosas <farosas@suse.de>
Reviewed-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 2dc6a063e9..0d26db47f7 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -799,10 +799,7 @@ process_incoming_migration_co(void *opaque)
 fail:
     migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE,
                       MIGRATION_STATUS_FAILED);
-    qemu_fclose(mis->from_src_file);
-
-    multifd_recv_cleanup();
-    compress_threads_load_cleanup();
+    migration_incoming_state_destroy();
 
     exit(EXIT_FAILURE);
 }
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v6 3/5] migration: process_incoming_migration_co(): fix reporting s->error
  2024-04-30  8:56 [PATCH v6 0/5] migration: do not exit on incoming failure Vladimir Sementsov-Ogievskiy
  2024-04-30  8:56 ` [PATCH v6 1/5] migration: move trace-point from migrate_fd_error to migrate_set_error Vladimir Sementsov-Ogievskiy
  2024-04-30  8:56 ` [PATCH v6 2/5] migration: process_incoming_migration_co(): complete cleanup on failure Vladimir Sementsov-Ogievskiy
@ 2024-04-30  8:56 ` Vladimir Sementsov-Ogievskiy
  2024-04-30 14:29   ` Fabiano Rosas
  2024-04-30  8:56 ` [PATCH v6 4/5] migration: process_incoming_migration_co(): rework error reporting Vladimir Sementsov-Ogievskiy
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 11+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2024-04-30  8:56 UTC (permalink / raw)
  To: peterx, farosas; +Cc: eblake, armbru, pbonzini, qemu-devel, vsementsov, yc-core

It's bad idea to leave critical section with error object freed, but
s->error still set, this theoretically may lead to use-after-free
crash. Let's avoid it.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 migration/migration.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/migration/migration.c b/migration/migration.c
index 0d26db47f7..b307a4bc59 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -784,6 +784,7 @@ process_incoming_migration_co(void *opaque)
         if (migrate_has_error(s)) {
             WITH_QEMU_LOCK_GUARD(&s->error_mutex) {
                 error_report_err(s->error);
+                s->error = NULL;
             }
         }
         error_report("load of migration failed: %s", strerror(-ret));
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v6 4/5] migration: process_incoming_migration_co(): rework error reporting
  2024-04-30  8:56 [PATCH v6 0/5] migration: do not exit on incoming failure Vladimir Sementsov-Ogievskiy
                   ` (2 preceding siblings ...)
  2024-04-30  8:56 ` [PATCH v6 3/5] migration: process_incoming_migration_co(): fix reporting s->error Vladimir Sementsov-Ogievskiy
@ 2024-04-30  8:56 ` Vladimir Sementsov-Ogievskiy
  2024-04-30  9:16   ` Philippe Mathieu-Daudé
  2024-04-30  8:56 ` [PATCH v6 5/5] qapi: introduce exit-on-error parameter for migrate-incoming Vladimir Sementsov-Ogievskiy
  2024-05-01 16:00 ` [PATCH v6 0/5] migration: do not exit on incoming failure Peter Xu
  5 siblings, 1 reply; 11+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2024-04-30  8:56 UTC (permalink / raw)
  To: peterx, farosas; +Cc: eblake, armbru, pbonzini, qemu-devel, vsementsov, yc-core

Unify error reporting in the function. This simplifies the following
commit, which will not-exit-on-error behavior variant to the function.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 migration/migration.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index b307a4bc59..a9599838e6 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -735,14 +735,16 @@ static void process_incoming_migration_bh(void *opaque)
 static void coroutine_fn
 process_incoming_migration_co(void *opaque)
 {
+    MigrationState *s = migrate_get_current();
     MigrationIncomingState *mis = migration_incoming_get_current();
     PostcopyState ps;
     int ret;
+    Error *local_err = NULL;
 
     assert(mis->from_src_file);
 
     if (compress_threads_load_setup(mis->from_src_file)) {
-        error_report("Failed to setup decompress threads");
+        error_setg(&local_err, "Failed to setup decompress threads");
         goto fail;
     }
 
@@ -779,19 +781,12 @@ process_incoming_migration_co(void *opaque)
     }
 
     if (ret < 0) {
-        MigrationState *s = migrate_get_current();
-
-        if (migrate_has_error(s)) {
-            WITH_QEMU_LOCK_GUARD(&s->error_mutex) {
-                error_report_err(s->error);
-                s->error = NULL;
-            }
-        }
-        error_report("load of migration failed: %s", strerror(-ret));
+        error_setg(&local_err, "load of migration failed: %s", strerror(-ret));
         goto fail;
     }
 
     if (colo_incoming_co() < 0) {
+        error_setg(&local_err, "colo incoming failed");
         goto fail;
     }
 
@@ -800,8 +795,16 @@ process_incoming_migration_co(void *opaque)
 fail:
     migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE,
                       MIGRATION_STATUS_FAILED);
+    migrate_set_error(s, local_err);
+    error_free(local_err);
+
     migration_incoming_state_destroy();
 
+    WITH_QEMU_LOCK_GUARD(&s->error_mutex) {
+        error_report_err(s->error);
+        s->error = NULL;
+    }
+
     exit(EXIT_FAILURE);
 }
 
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v6 5/5] qapi: introduce exit-on-error parameter for migrate-incoming
  2024-04-30  8:56 [PATCH v6 0/5] migration: do not exit on incoming failure Vladimir Sementsov-Ogievskiy
                   ` (3 preceding siblings ...)
  2024-04-30  8:56 ` [PATCH v6 4/5] migration: process_incoming_migration_co(): rework error reporting Vladimir Sementsov-Ogievskiy
@ 2024-04-30  8:56 ` Vladimir Sementsov-Ogievskiy
  2024-04-30 14:40   ` Fabiano Rosas
  2024-05-01 16:00 ` [PATCH v6 0/5] migration: do not exit on incoming failure Peter Xu
  5 siblings, 1 reply; 11+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2024-04-30  8:56 UTC (permalink / raw)
  To: peterx, farosas; +Cc: eblake, armbru, pbonzini, qemu-devel, vsementsov, yc-core

Now we do set MIGRATION_FAILED state, but don't give a chance to
orchestrator to query migration state and get the error.

Let's provide a possibility for QMP-based orchestrators to get an error
like with outgoing migration.

For hmp_migrate_incoming(), let's enable the new behavior: HMP is not
and ABI, it's mostly intended to use by developer and it makes sense
not to stop the process.

For x-exit-preconfig, let's keep the old behavior:
 - it's called from init(), so here we want to keep current behavior by
   default
 - it does exit on error by itself as well
So, if we want to change the behavior of x-exit-preconfig, it should be
another patch.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Acked-by: Markus Armbruster <armbru@redhat.com>
---
 migration/migration-hmp-cmds.c |  2 +-
 migration/migration.c          | 33 +++++++++++++++++++++++++++------
 migration/migration.h          |  3 +++
 qapi/migration.json            |  7 ++++++-
 system/vl.c                    |  3 ++-
 5 files changed, 39 insertions(+), 9 deletions(-)

diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
index 7e96ae6ffd..23181bbee1 100644
--- a/migration/migration-hmp-cmds.c
+++ b/migration/migration-hmp-cmds.c
@@ -466,7 +466,7 @@ void hmp_migrate_incoming(Monitor *mon, const QDict *qdict)
     }
     QAPI_LIST_PREPEND(caps, g_steal_pointer(&channel));
 
-    qmp_migrate_incoming(NULL, true, caps, &err);
+    qmp_migrate_incoming(NULL, true, caps, true, false, &err);
     qapi_free_MigrationChannelList(caps);
 
 end:
diff --git a/migration/migration.c b/migration/migration.c
index a9599838e6..289afa8d85 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -72,6 +72,8 @@
 #define NOTIFIER_ELEM_INIT(array, elem)    \
     [elem] = NOTIFIER_WITH_RETURN_LIST_INITIALIZER((array)[elem])
 
+#define INMIGRATE_DEFAULT_EXIT_ON_ERROR true
+
 static NotifierWithReturnList migration_state_notifiers[] = {
     NOTIFIER_ELEM_INIT(migration_state_notifiers, MIG_MODE_NORMAL),
     NOTIFIER_ELEM_INIT(migration_state_notifiers, MIG_MODE_CPR_REBOOT),
@@ -234,6 +236,8 @@ void migration_object_init(void)
     qemu_cond_init(&current_incoming->page_request_cond);
     current_incoming->page_requested = g_tree_new(page_request_addr_cmp);
 
+    current_incoming->exit_on_error = INMIGRATE_DEFAULT_EXIT_ON_ERROR;
+
     migration_object_check(current_migration, &error_fatal);
 
     blk_mig_init();
@@ -800,12 +804,14 @@ fail:
 
     migration_incoming_state_destroy();
 
-    WITH_QEMU_LOCK_GUARD(&s->error_mutex) {
-        error_report_err(s->error);
-        s->error = NULL;
-    }
+    if (mis->exit_on_error) {
+        WITH_QEMU_LOCK_GUARD(&s->error_mutex) {
+            error_report_err(s->error);
+            s->error = NULL;
+        }
 
-    exit(EXIT_FAILURE);
+        exit(EXIT_FAILURE);
+    }
 }
 
 /**
@@ -1314,6 +1320,15 @@ static void fill_destination_migration_info(MigrationInfo *info)
         break;
     }
     info->status = mis->state;
+
+    if (!info->error_desc) {
+        MigrationState *s = migrate_get_current();
+        QEMU_LOCK_GUARD(&s->error_mutex);
+
+        if (s->error) {
+            info->error_desc = g_strdup(error_get_pretty(s->error));
+        }
+    }
 }
 
 MigrationInfo *qmp_query_migrate(Error **errp)
@@ -1797,10 +1812,13 @@ void migrate_del_blocker(Error **reasonp)
 }
 
 void qmp_migrate_incoming(const char *uri, bool has_channels,
-                          MigrationChannelList *channels, Error **errp)
+                          MigrationChannelList *channels,
+                          bool has_exit_on_error, bool exit_on_error,
+                          Error **errp)
 {
     Error *local_err = NULL;
     static bool once = true;
+    MigrationIncomingState *mis = migration_incoming_get_current();
 
     if (!once) {
         error_setg(errp, "The incoming migration has already been started");
@@ -1815,6 +1833,9 @@ void qmp_migrate_incoming(const char *uri, bool has_channels,
         return;
     }
 
+    mis->exit_on_error =
+        has_exit_on_error ? exit_on_error : INMIGRATE_DEFAULT_EXIT_ON_ERROR;
+
     qemu_start_incoming_migration(uri, has_channels, channels, &local_err);
 
     if (local_err) {
diff --git a/migration/migration.h b/migration/migration.h
index 8045e39c26..95995a818e 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -227,6 +227,9 @@ struct MigrationIncomingState {
      * is needed as this field is updated serially.
      */
     unsigned int switchover_ack_pending_num;
+
+    /* Do exit on incoming migration failure */
+    bool exit_on_error;
 };
 
 MigrationIncomingState *migration_incoming_get_current(void);
diff --git a/qapi/migration.json b/qapi/migration.json
index 8c65b90328..9feed413b5 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1837,6 +1837,10 @@
 # @channels: list of migration stream channels with each stream in the
 #     list connected to a destination interface endpoint.
 #
+# @exit-on-error: Exit on incoming migration failure.  Default true.
+#     When set to false, the failure triggers a MIGRATION event, and
+#     error details could be retrieved with query-migrate.  (since 9.1)
+#
 # Since: 2.3
 #
 # Notes:
@@ -1889,7 +1893,8 @@
 ##
 { 'command': 'migrate-incoming',
              'data': {'*uri': 'str',
-                      '*channels': [ 'MigrationChannel' ] } }
+                      '*channels': [ 'MigrationChannel' ],
+                      '*exit-on-error': 'bool' } }
 
 ##
 # @xen-save-devices-state:
diff --git a/system/vl.c b/system/vl.c
index 7756eac81e..79cd498395 100644
--- a/system/vl.c
+++ b/system/vl.c
@@ -2723,7 +2723,8 @@ void qmp_x_exit_preconfig(Error **errp)
     if (incoming) {
         Error *local_err = NULL;
         if (strcmp(incoming, "defer") != 0) {
-            qmp_migrate_incoming(incoming, false, NULL, &local_err);
+            qmp_migrate_incoming(incoming, false, NULL, true, true,
+                                 &local_err);
             if (local_err) {
                 error_reportf_err(local_err, "-incoming %s: ", incoming);
                 exit(1);
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v6 4/5] migration: process_incoming_migration_co(): rework error reporting
  2024-04-30  8:56 ` [PATCH v6 4/5] migration: process_incoming_migration_co(): rework error reporting Vladimir Sementsov-Ogievskiy
@ 2024-04-30  9:16   ` Philippe Mathieu-Daudé
  2024-05-01  7:19     ` Vladimir Sementsov-Ogievskiy
  0 siblings, 1 reply; 11+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-04-30  9:16 UTC (permalink / raw)
  To: Vladimir Sementsov-Ogievskiy, peterx, farosas
  Cc: eblake, armbru, pbonzini, qemu-devel, yc-core

On 30/4/24 10:56, Vladimir Sementsov-Ogievskiy wrote:
> Unify error reporting in the function. This simplifies the following
> commit, which will not-exit-on-error behavior variant to the function.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>   migration/migration.c | 23 +++++++++++++----------
>   1 file changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index b307a4bc59..a9599838e6 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -735,14 +735,16 @@ static void process_incoming_migration_bh(void *opaque)
>   static void coroutine_fn
>   process_incoming_migration_co(void *opaque)
>   {
> +    MigrationState *s = migrate_get_current();

(see below)

>       MigrationIncomingState *mis = migration_incoming_get_current();
>       PostcopyState ps;
>       int ret;
> +    Error *local_err = NULL;
>   
>       assert(mis->from_src_file);
>   
>       if (compress_threads_load_setup(mis->from_src_file)) {
> -        error_report("Failed to setup decompress threads");
> +        error_setg(&local_err, "Failed to setup decompress threads");
>           goto fail;
>       }
>   
> @@ -779,19 +781,12 @@ process_incoming_migration_co(void *opaque)
>       }
>   
>       if (ret < 0) {
> -        MigrationState *s = migrate_get_current();
> -
> -        if (migrate_has_error(s)) {
> -            WITH_QEMU_LOCK_GUARD(&s->error_mutex) {
> -                error_report_err(s->error);
> -                s->error = NULL;
> -            }
> -        }
> -        error_report("load of migration failed: %s", strerror(-ret));
> +        error_setg(&local_err, "load of migration failed: %s", strerror(-ret));
>           goto fail;
>       }
>   
>       if (colo_incoming_co() < 0) {
> +        error_setg(&local_err, "colo incoming failed");
>           goto fail;
>       }
>   
> @@ -800,8 +795,16 @@ process_incoming_migration_co(void *opaque)
>   fail:

Maybe just assign @s in error path here?

        s = migrate_get_current();

>       migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE,
>                         MIGRATION_STATUS_FAILED);
> +    migrate_set_error(s, local_err);
> +    error_free(local_err);
> +
>       migration_incoming_state_destroy();
>   
> +    WITH_QEMU_LOCK_GUARD(&s->error_mutex) {
> +        error_report_err(s->error);
> +        s->error = NULL;
> +    }
> +
>       exit(EXIT_FAILURE);
>   }
>   

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v6 3/5] migration: process_incoming_migration_co(): fix reporting s->error
  2024-04-30  8:56 ` [PATCH v6 3/5] migration: process_incoming_migration_co(): fix reporting s->error Vladimir Sementsov-Ogievskiy
@ 2024-04-30 14:29   ` Fabiano Rosas
  0 siblings, 0 replies; 11+ messages in thread
From: Fabiano Rosas @ 2024-04-30 14:29 UTC (permalink / raw)
  To: Vladimir Sementsov-Ogievskiy, peterx
  Cc: eblake, armbru, pbonzini, qemu-devel, vsementsov, yc-core

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:

> It's bad idea to leave critical section with error object freed, but
> s->error still set, this theoretically may lead to use-after-free
> crash. Let's avoid it.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

Reviewed-by: Fabiano Rosas <farosas@suse.de>


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v6 5/5] qapi: introduce exit-on-error parameter for migrate-incoming
  2024-04-30  8:56 ` [PATCH v6 5/5] qapi: introduce exit-on-error parameter for migrate-incoming Vladimir Sementsov-Ogievskiy
@ 2024-04-30 14:40   ` Fabiano Rosas
  0 siblings, 0 replies; 11+ messages in thread
From: Fabiano Rosas @ 2024-04-30 14:40 UTC (permalink / raw)
  To: Vladimir Sementsov-Ogievskiy, peterx
  Cc: eblake, armbru, pbonzini, qemu-devel, vsementsov, yc-core

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:

> Now we do set MIGRATION_FAILED state, but don't give a chance to
> orchestrator to query migration state and get the error.
>
> Let's provide a possibility for QMP-based orchestrators to get an error
> like with outgoing migration.
>
> For hmp_migrate_incoming(), let's enable the new behavior: HMP is not
> and ABI, it's mostly intended to use by developer and it makes sense
> not to stop the process.
>
> For x-exit-preconfig, let's keep the old behavior:
>  - it's called from init(), so here we want to keep current behavior by
>    default
>  - it does exit on error by itself as well
> So, if we want to change the behavior of x-exit-preconfig, it should be
> another patch.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> Acked-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Fabiano Rosas <farosas@suse.de>


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v6 4/5] migration: process_incoming_migration_co(): rework error reporting
  2024-04-30  9:16   ` Philippe Mathieu-Daudé
@ 2024-05-01  7:19     ` Vladimir Sementsov-Ogievskiy
  0 siblings, 0 replies; 11+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2024-05-01  7:19 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, peterx, farosas
  Cc: eblake, armbru, pbonzini, qemu-devel, yc-core

On 30.04.24 12:16, Philippe Mathieu-Daudé wrote:
> On 30/4/24 10:56, Vladimir Sementsov-Ogievskiy wrote:
>> Unify error reporting in the function. This simplifies the following
>> commit, which will not-exit-on-error behavior variant to the function.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> ---
>>   migration/migration.c | 23 +++++++++++++----------
>>   1 file changed, 13 insertions(+), 10 deletions(-)
>>
>> diff --git a/migration/migration.c b/migration/migration.c
>> index b307a4bc59..a9599838e6 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -735,14 +735,16 @@ static void process_incoming_migration_bh(void *opaque)
>>   static void coroutine_fn
>>   process_incoming_migration_co(void *opaque)
>>   {
>> +    MigrationState *s = migrate_get_current();
> 
> (see below)
> 
>>       MigrationIncomingState *mis = migration_incoming_get_current();
>>       PostcopyState ps;
>>       int ret;
>> +    Error *local_err = NULL;
>>       assert(mis->from_src_file);
>>       if (compress_threads_load_setup(mis->from_src_file)) {
>> -        error_report("Failed to setup decompress threads");
>> +        error_setg(&local_err, "Failed to setup decompress threads");
>>           goto fail;
>>       }
>> @@ -779,19 +781,12 @@ process_incoming_migration_co(void *opaque)
>>       }
>>       if (ret < 0) {
>> -        MigrationState *s = migrate_get_current();
>> -
>> -        if (migrate_has_error(s)) {
>> -            WITH_QEMU_LOCK_GUARD(&s->error_mutex) {
>> -                error_report_err(s->error);
>> -                s->error = NULL;
>> -            }
>> -        }
>> -        error_report("load of migration failed: %s", strerror(-ret));
>> +        error_setg(&local_err, "load of migration failed: %s", strerror(-ret));
>>           goto fail;
>>       }
>>       if (colo_incoming_co() < 0) {
>> +        error_setg(&local_err, "colo incoming failed");
>>           goto fail;
>>       }
>> @@ -800,8 +795,16 @@ process_incoming_migration_co(void *opaque)
>>   fail:
> 
> Maybe just assign @s in error path here?
> 
>         s = migrate_get_current();

I'd keep as is. If continue improving the function, I'd better split the logic to seperate function with classic "Error **errp" argument. And keep reporting error in caller.

> 
>>       migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE,
>>                         MIGRATION_STATUS_FAILED);
>> +    migrate_set_error(s, local_err);
>> +    error_free(local_err);
>> +
>>       migration_incoming_state_destroy();
>> +    WITH_QEMU_LOCK_GUARD(&s->error_mutex) {
>> +        error_report_err(s->error);
>> +        s->error = NULL;
>> +    }
>> +
>>       exit(EXIT_FAILURE);
>>   }
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 

-- 
Best regards,
Vladimir



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v6 0/5] migration: do not exit on incoming failure
  2024-04-30  8:56 [PATCH v6 0/5] migration: do not exit on incoming failure Vladimir Sementsov-Ogievskiy
                   ` (4 preceding siblings ...)
  2024-04-30  8:56 ` [PATCH v6 5/5] qapi: introduce exit-on-error parameter for migrate-incoming Vladimir Sementsov-Ogievskiy
@ 2024-05-01 16:00 ` Peter Xu
  5 siblings, 0 replies; 11+ messages in thread
From: Peter Xu @ 2024-05-01 16:00 UTC (permalink / raw)
  To: Vladimir Sementsov-Ogievskiy
  Cc: farosas, eblake, armbru, pbonzini, qemu-devel, yc-core

On Tue, Apr 30, 2024 at 11:56:41AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
> 
> The series brings an option to not immediately exit on incoming
> migration failure, giving a possibility to orchestrator to get the error
> through QAPI and shutdown QEMU by "quit".

There seem to have one comment overlooked in the last patch I left before
(on a probably useless error_desc pre-check), but doesn't look a huge
deal..

Reviewed-by: Peter Xu <peterx@redhat.com>

Thanks,

-- 
Peter Xu



^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2024-05-01 16:01 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-04-30  8:56 [PATCH v6 0/5] migration: do not exit on incoming failure Vladimir Sementsov-Ogievskiy
2024-04-30  8:56 ` [PATCH v6 1/5] migration: move trace-point from migrate_fd_error to migrate_set_error Vladimir Sementsov-Ogievskiy
2024-04-30  8:56 ` [PATCH v6 2/5] migration: process_incoming_migration_co(): complete cleanup on failure Vladimir Sementsov-Ogievskiy
2024-04-30  8:56 ` [PATCH v6 3/5] migration: process_incoming_migration_co(): fix reporting s->error Vladimir Sementsov-Ogievskiy
2024-04-30 14:29   ` Fabiano Rosas
2024-04-30  8:56 ` [PATCH v6 4/5] migration: process_incoming_migration_co(): rework error reporting Vladimir Sementsov-Ogievskiy
2024-04-30  9:16   ` Philippe Mathieu-Daudé
2024-05-01  7:19     ` Vladimir Sementsov-Ogievskiy
2024-04-30  8:56 ` [PATCH v6 5/5] qapi: introduce exit-on-error parameter for migrate-incoming Vladimir Sementsov-Ogievskiy
2024-04-30 14:40   ` Fabiano Rosas
2024-05-01 16:00 ` [PATCH v6 0/5] migration: do not exit on incoming failure Peter Xu

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.