[PATCH 00/11] migration: Misc cleanups and fixes

[PATCH 00/11] migration: Misc cleanups and fixes

[Avihai Horon]

Hi,

This series contains misc cleanups and fixes in migration code that I
noticed while going over the code.

Thanks.

Avihai Horon (11):
  migration: Remove migrate_max_downtime() declaration
  migration: Remove nulling of hostname in migrate_init()
  migration: Refactor migration_incoming_setup()
  migration: Remove errp parameter in migration_fd_process_incoming()
  migration/multifd: Fix error message in multifd_recv_initial_packet()
  migration/multifd: Simplify multifd_channel_connect() if else
    statement
  migration/multifd: Fix leaking of Error in TLS error flow
  migration/multifd: Remove error_setg() in
    migration_ioc_process_incoming()
  migration: Fix migration_channel_read_peek() error path
  migration: Remove unnecessary usage of local Error
  migration/multifd: Remove unnecessary usage of local Error

 migration/migration.h |  4 +---
 migration/channel.c   |  9 ++++++---
 migration/migration.c | 27 ++++++---------------------
 migration/multifd.c   | 27 ++++++++++++---------------
 migration/rdma.c      |  6 +-----
 5 files changed, 26 insertions(+), 47 deletions(-)

-- 
2.26.3

[PATCH 01/11] migration: Remove migrate_max_downtime() declaration

[Avihai Horon]

migrate_max_downtime() has been removed long ago, but its declaration
was mistakenly left. Remove it.

Signed-off-by: Avihai Horon <avihaih@nvidia.com>
---
 migration/migration.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/migration/migration.h b/migration/migration.h
index cf2c9c88e0..b3c9288c38 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -480,8 +480,6 @@ void migration_incoming_process(void);
 
 bool  migration_has_all_channels(void);
 
-uint64_t migrate_max_downtime(void);
-
 void migrate_set_error(MigrationState *s, const Error *error);
 bool migrate_has_error(MigrationState *s);
 
-- 
2.26.3

[PATCH 02/11] migration: Remove nulling of hostname in migrate_init()

[Avihai Horon]

MigrationState->hostname is set to NULL in migrate_init(). This is
redundant because it is already freed and set to NULL in
migrade_fd_cleanup(). Remove it.

Signed-off-by: Avihai Horon <avihaih@nvidia.com>
---
 migration/migration.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/migration/migration.c b/migration/migration.c
index 3ce04b2aaf..1e25d7966f 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1588,7 +1588,6 @@ int migrate_init(MigrationState *s, Error **errp)
     s->migration_thread_running = false;
     error_free(s->error);
     s->error = NULL;
-    s->hostname = NULL;
     s->vmdesc = NULL;
 
     migrate_set_state(&s->state, MIGRATION_STATUS_NONE, MIGRATION_STATUS_SETUP);
-- 
2.26.3

[PATCH 03/11] migration: Refactor migration_incoming_setup()

[Avihai Horon]

Commit 6720c2b32725 ("migration: check magic value for deciding the
mapping of channels") extracted the only code that could fail in
migration_incoming_setup().

Now migration_incoming_setup() can't fail, so refactor it to return void
and remove errp parameter.

Signed-off-by: Avihai Horon <avihaih@nvidia.com>
---
 migration/migration.c | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 1e25d7966f..55d77572d8 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -724,11 +724,8 @@ fail:
 /**
  * migration_incoming_setup: Setup incoming migration
  * @f: file for main migration channel
- * @errp: where to put errors
- *
- * Returns: %true on success, %false on error.
  */
-static bool migration_incoming_setup(QEMUFile *f, Error **errp)
+static void migration_incoming_setup(QEMUFile *f)
 {
     MigrationIncomingState *mis = migration_incoming_get_current();
 
@@ -736,7 +733,6 @@ static bool migration_incoming_setup(QEMUFile *f, Error **errp)
         mis->from_src_file = f;
     }
     qemu_file_set_blocking(f, false);
-    return true;
 }
 
 void migration_incoming_process(void)
@@ -780,9 +776,7 @@ static bool postcopy_try_recover(void)
 
 void migration_fd_process_incoming(QEMUFile *f, Error **errp)
 {
-    if (!migration_incoming_setup(f, errp)) {
-        return;
-    }
+    migration_incoming_setup(f);
     if (postcopy_try_recover()) {
         return;
     }
@@ -855,10 +849,7 @@ void migration_ioc_process_incoming(QIOChannel *ioc, Error **errp)
 
     if (default_channel) {
         f = qemu_file_new_input(ioc);
-
-        if (!migration_incoming_setup(f, errp)) {
-            return;
-        }
+        migration_incoming_setup(f);
     } else {
         /* Multiple connections */
         assert(migration_needs_multiple_sockets());
-- 
2.26.3

[PATCH 04/11] migration: Remove errp parameter in migration_fd_process_incoming()

[Avihai Horon]

Errp parameter in migration_fd_process_incoming() is unused.
Remove it.

Signed-off-by: Avihai Horon <avihaih@nvidia.com>
---
 migration/migration.h | 2 +-
 migration/migration.c | 2 +-
 migration/rdma.c      | 6 +-----
 3 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/migration/migration.h b/migration/migration.h
index b3c9288c38..17972dac34 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -474,7 +474,7 @@ struct MigrationState {
 
 void migrate_set_state(int *state, int old_state, int new_state);
 
-void migration_fd_process_incoming(QEMUFile *f, Error **errp);
+void migration_fd_process_incoming(QEMUFile *f);
 void migration_ioc_process_incoming(QIOChannel *ioc, Error **errp);
 void migration_incoming_process(void);
 
diff --git a/migration/migration.c b/migration/migration.c
index 55d77572d8..732e3dadd9 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -774,7 +774,7 @@ static bool postcopy_try_recover(void)
     return false;
 }
 
-void migration_fd_process_incoming(QEMUFile *f, Error **errp)
+void migration_fd_process_incoming(QEMUFile *f)
 {
     migration_incoming_setup(f);
     if (postcopy_try_recover()) {
diff --git a/migration/rdma.c b/migration/rdma.c
index 04debab5d9..94c0f871f0 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -4035,7 +4035,6 @@ static void rdma_accept_incoming_migration(void *opaque)
 {
     RDMAContext *rdma = opaque;
     QEMUFile *f;
-    Error *local_err = NULL;
 
     trace_qemu_rdma_accept_incoming_migration();
     if (qemu_rdma_accept(rdma) < 0) {
@@ -4057,10 +4056,7 @@ static void rdma_accept_incoming_migration(void *opaque)
     }
 
     rdma->migration_started_on_destination = 1;
-    migration_fd_process_incoming(f, &local_err);
-    if (local_err) {
-        error_reportf_err(local_err, "RDMA ERROR:");
-    }
+    migration_fd_process_incoming(f);
 }
 
 void rdma_start_incoming_migration(InetSocketAddress *host_port,
-- 
2.26.3

[PATCH 05/11] migration/multifd: Fix error message in multifd_recv_initial_packet()

[Avihai Horon]

In multifd_recv_initial_packet(), if MultiFDInit_t->id is greater than
the configured number of multifd channels, an irrelevant error message
about multifd version is printed.

Change the error message to a relevant one about the channel id.

Signed-off-by: Avihai Horon <avihaih@nvidia.com>
---
 migration/multifd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/migration/multifd.c b/migration/multifd.c
index 409460684f..a6204fc72f 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -228,8 +228,8 @@ static int multifd_recv_initial_packet(QIOChannel *c, Error **errp)
     }
 
     if (msg.id > migrate_multifd_channels()) {
-        error_setg(errp, "multifd: received channel version %u "
-                   "expected %u", msg.version, MULTIFD_VERSION);
+        error_setg(errp, "multifd: received channel id %u is greater than "
+                   "number of channels %u", msg.id, migrate_multifd_channels());
         return -1;
     }
 
-- 
2.26.3

[PATCH 06/11] migration/multifd: Simplify multifd_channel_connect() if else statement

[Avihai Horon]

The else branch in multifd_channel_connect() is redundant because when
the if branch is taken the function returns.

Simplify the code by removing the else branch.

Signed-off-by: Avihai Horon <avihaih@nvidia.com>
---
 migration/multifd.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/migration/multifd.c b/migration/multifd.c
index a6204fc72f..55d5fd55f8 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -847,14 +847,13 @@ static bool multifd_channel_connect(MultiFDSendParams *p,
          * so we mustn't call multifd_send_thread until then
          */
         return multifd_tls_channel_connect(p, ioc, errp);
-
-    } else {
-        migration_ioc_register_yank(ioc);
-        p->registered_yank = true;
-        p->c = ioc;
-        qemu_thread_create(&p->thread, p->name, multifd_send_thread, p,
-                           QEMU_THREAD_JOINABLE);
     }
+
+    migration_ioc_register_yank(ioc);
+    p->registered_yank = true;
+    p->c = ioc;
+    qemu_thread_create(&p->thread, p->name, multifd_send_thread, p,
+                       QEMU_THREAD_JOINABLE);
     return true;
 }
 
-- 
2.26.3

[PATCH 07/11] migration/multifd: Fix leaking of Error in TLS error flow

[Avihai Horon]

If there is an error in multifd TLS handshake task,
multifd_tls_outgoing_handshake() retrieves the error with
qio_task_propagate_error() but never frees it.

Fix it by freeing the obtained Error.

In addition, the error is not reported at all, so report it with
migrate_set_error().

Fixes: 29647140157a ("migration/tls: add support for multifd tls-handshake")
Signed-off-by: Avihai Horon <avihaih@nvidia.com>
---
 migration/multifd.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/migration/multifd.c b/migration/multifd.c
index 55d5fd55f8..9ac24866ad 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -787,6 +787,7 @@ static void multifd_tls_outgoing_handshake(QIOTask *task,
 
     trace_multifd_tls_outgoing_handshake_error(ioc, error_get_pretty(err));
 
+    migrate_set_error(migrate_get_current(), err);
     /*
      * Error happen, mark multifd_send_thread status as 'quit' although it
      * is not created, and then tell who pay attention to me.
@@ -794,6 +795,7 @@ static void multifd_tls_outgoing_handshake(QIOTask *task,
     p->quit = true;
     qemu_sem_post(&multifd_send_state->channels_ready);
     qemu_sem_post(&p->sem_sync);
+    error_free(err);
 }
 
 static void *multifd_tls_handshake_thread(void *opaque)
-- 
2.26.3

[PATCH 08/11] migration/multifd: Remove error_setg() in migration_ioc_process_incoming()

[Avihai Horon]

If multifd_load_setup() fails in migration_ioc_process_incoming(),
error_setg() is called with errp. This will lead to an assert because in
that case errp already contains an error.

Fix it by removing the redundant error_setg().

Fixes: 6720c2b32725 ("migration: check magic value for deciding the mapping of channels")
Signed-off-by: Avihai Horon <avihaih@nvidia.com>
---
 migration/migration.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/migration/migration.c b/migration/migration.c
index 732e3dadd9..b0c3b4c23a 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -843,7 +843,6 @@ void migration_ioc_process_incoming(QIOChannel *ioc, Error **errp)
     }
 
     if (multifd_load_setup(errp) != 0) {
-        error_setg(errp, "Failed to setup multifd channels");
         return;
     }
 
-- 
2.26.3

[PATCH 09/11] migration: Fix migration_channel_read_peek() error path

[Avihai Horon]

migration_channel_read_peek() calls qio_channel_readv_full() and handles
both cases of return value == 0 and return value < 0 the same way, by
calling error_setg() with errp. However, if return value < 0, errp is
already set, so calling error_setg() with errp will lead to an assert.

Fix it by handling these cases separately, calling error_setg() with
errp only in return value == 0 case.

Fixes: 6720c2b32725 ("migration: check magic value for deciding the mapping of channels")
Signed-off-by: Avihai Horon <avihaih@nvidia.com>
---
 migration/channel.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/migration/channel.c b/migration/channel.c
index ca3319a309..f9de064f3b 100644
--- a/migration/channel.c
+++ b/migration/channel.c
@@ -117,9 +117,12 @@ int migration_channel_read_peek(QIOChannel *ioc,
         len = qio_channel_readv_full(ioc, &iov, 1, NULL, NULL,
                                      QIO_CHANNEL_READ_FLAG_MSG_PEEK, errp);
 
-        if (len <= 0 && len != QIO_CHANNEL_ERR_BLOCK) {
-            error_setg(errp,
-                       "Failed to peek at channel");
+        if (len < 0 && len != QIO_CHANNEL_ERR_BLOCK) {
+            return -1;
+        }
+
+        if (len == 0) {
+            error_setg(errp, "Failed to peek at channel");
             return -1;
         }
 
-- 
2.26.3

[PATCH 10/11] migration: Remove unnecessary usage of local Error

[Avihai Horon]

According to Error API, usage of ERRP_GUARD() or a local Error instead
of errp is needed if errp is passed to void functions, where it is later
dereferenced to see if an error occurred.

There are several places in migration.c that use local Error although it
is not needed. Change these places to use errp directly.

Signed-off-by: Avihai Horon <avihaih@nvidia.com>
---
 migration/migration.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index b0c3b4c23a..fc7f4fd263 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -830,10 +830,9 @@ void migration_ioc_process_incoming(QIOChannel *ioc, Error **errp)
          * issue is not possible.
          */
         ret = migration_channel_read_peek(ioc, (void *)&channel_magic,
-                                          sizeof(channel_magic), &local_err);
+                                          sizeof(channel_magic), errp);
 
         if (ret != 0) {
-            error_propagate(errp, local_err);
             return;
         }
 
@@ -1825,8 +1824,6 @@ bool migration_is_blocked(Error **errp)
 static bool migrate_prepare(MigrationState *s, bool blk, bool blk_inc,
                             bool resume, Error **errp)
 {
-    Error *local_err = NULL;
-
     if (blk_inc) {
         warn_report("parameter 'inc' is deprecated;"
                     " use blockdev-mirror with NBD instead");
@@ -1896,8 +1893,7 @@ static bool migrate_prepare(MigrationState *s, bool blk, bool blk_inc,
                        "current migration capabilities");
             return false;
         }
-        if (!migrate_cap_set(MIGRATION_CAPABILITY_BLOCK, true, &local_err)) {
-            error_propagate(errp, local_err);
+        if (!migrate_cap_set(MIGRATION_CAPABILITY_BLOCK, true, errp)) {
             return false;
         }
         s->must_remove_block_options = true;
-- 
2.26.3

[PATCH 11/11] migration/multifd: Remove unnecessary usage of local Error

[Avihai Horon]

According to Error API, usage of ERRP_GUARD() or a local Error instead
of errp is needed if errp is passed to void functions, where it is later
dereferenced to see if an error occurred.

There are several places in multifd.c that use local Error although it
is not needed. Change these places to use errp directly.

Signed-off-by: Avihai Horon <avihaih@nvidia.com>
---
 migration/multifd.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/migration/multifd.c b/migration/multifd.c
index 9ac24866ad..9f353aecfa 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -951,12 +951,10 @@ int multifd_save_setup(Error **errp)
 
     for (i = 0; i < thread_count; i++) {
         MultiFDSendParams *p = &multifd_send_state->params[i];
-        Error *local_err = NULL;
         int ret;
 
-        ret = multifd_send_state->ops->send_setup(p, &local_err);
+        ret = multifd_send_state->ops->send_setup(p, errp);
         if (ret) {
-            error_propagate(errp, local_err);
             return ret;
         }
     }
@@ -1195,12 +1193,10 @@ int multifd_load_setup(Error **errp)
 
     for (i = 0; i < thread_count; i++) {
         MultiFDRecvParams *p = &multifd_recv_state->params[i];
-        Error *local_err = NULL;
         int ret;
 
-        ret = multifd_recv_state->ops->recv_setup(p, &local_err);
+        ret = multifd_recv_state->ops->recv_setup(p, errp);
         if (ret) {
-            error_propagate(errp, local_err);
             return ret;
         }
     }
-- 
2.26.3

Re: [PATCH 03/11] migration: Refactor migration_incoming_setup()

[Philippe Mathieu-Daudé]

On 31/12/23 10:30, Avihai Horon wrote:
> Commit 6720c2b32725 ("migration: check magic value for deciding the
> mapping of channels") extracted the only code that could fail in
> migration_incoming_setup().
> 
> Now migration_incoming_setup() can't fail, so refactor it to return void
> and remove errp parameter.
> 
> Signed-off-by: Avihai Horon <avihaih@nvidia.com>
> ---
>   migration/migration.c | 15 +++------------
>   1 file changed, 3 insertions(+), 12 deletions(-)

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

Re: [PATCH 06/11] migration/multifd: Simplify multifd_channel_connect() if else statement

[Philippe Mathieu-Daudé]

On 31/12/23 10:30, Avihai Horon wrote:
> The else branch in multifd_channel_connect() is redundant because when
> the if branch is taken the function returns.
> 
> Simplify the code by removing the else branch.
> 
> Signed-off-by: Avihai Horon <avihaih@nvidia.com>
> ---
>   migration/multifd.c | 13 ++++++-------
>   1 file changed, 6 insertions(+), 7 deletions(-)

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

Re: [PATCH 09/11] migration: Fix migration_channel_read_peek() error path

[Philippe Mathieu-Daudé]

+Markus

On 31/12/23 10:30, Avihai Horon wrote:
> migration_channel_read_peek() calls qio_channel_readv_full() and handles
> both cases of return value == 0 and return value < 0 the same way, by
> calling error_setg() with errp. However, if return value < 0, errp is
> already set, so calling error_setg() with errp will lead to an assert.

I suppose the API would be safer by passing &len as argument and
return a boolean.

> Fix it by handling these cases separately, calling error_setg() with
> errp only in return value == 0 case.
> 
> Fixes: 6720c2b32725 ("migration: check magic value for deciding the mapping of channels")
> Signed-off-by: Avihai Horon <avihaih@nvidia.com>
> ---
>   migration/channel.c | 9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/migration/channel.c b/migration/channel.c
> index ca3319a309..f9de064f3b 100644
> --- a/migration/channel.c
> +++ b/migration/channel.c
> @@ -117,9 +117,12 @@ int migration_channel_read_peek(QIOChannel *ioc,
>           len = qio_channel_readv_full(ioc, &iov, 1, NULL, NULL,
>                                        QIO_CHANNEL_READ_FLAG_MSG_PEEK, errp);
>   
> -        if (len <= 0 && len != QIO_CHANNEL_ERR_BLOCK) {
> -            error_setg(errp,
> -                       "Failed to peek at channel");
> +        if (len < 0 && len != QIO_CHANNEL_ERR_BLOCK) {
> +            return -1;
> +        }
> +
> +        if (len == 0) {
> +            error_setg(errp, "Failed to peek at channel");
>               return -1;
>           }

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

Re: [PATCH 11/11] migration/multifd: Remove unnecessary usage of local Error

[Philippe Mathieu-Daudé]

On 31/12/23 10:30, Avihai Horon wrote:
> According to Error API, usage of ERRP_GUARD() or a local Error instead
> of errp is needed if errp is passed to void functions, where it is later
> dereferenced to see if an error occurred.
> 
> There are several places in multifd.c that use local Error although it
> is not needed. Change these places to use errp directly.
> 
> Signed-off-by: Avihai Horon <avihaih@nvidia.com>
> ---
>   migration/multifd.c | 8 ++------
>   1 file changed, 2 insertions(+), 6 deletions(-)

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

Re: [PATCH 01/11] migration: Remove migrate_max_downtime() declaration

[Fabiano Rosas]

Avihai Horon <avihaih@nvidia.com> writes:

> migrate_max_downtime() has been removed long ago, but its declaration
> was mistakenly left. Remove it.
>
> Signed-off-by: Avihai Horon <avihaih@nvidia.com>

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

Re: [PATCH 02/11] migration: Remove nulling of hostname in migrate_init()

[Fabiano Rosas]

Avihai Horon <avihaih@nvidia.com> writes:

> MigrationState->hostname is set to NULL in migrate_init(). This is
> redundant because it is already freed and set to NULL in
> migrade_fd_cleanup(). Remove it.
>
> Signed-off-by: Avihai Horon <avihaih@nvidia.com>

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

Re: [PATCH 03/11] migration: Refactor migration_incoming_setup()

[Fabiano Rosas]

Avihai Horon <avihaih@nvidia.com> writes:

> Commit 6720c2b32725 ("migration: check magic value for deciding the
> mapping of channels") extracted the only code that could fail in
> migration_incoming_setup().
>
> Now migration_incoming_setup() can't fail, so refactor it to return void
> and remove errp parameter.
>
> Signed-off-by: Avihai Horon <avihaih@nvidia.com>

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

Re: [PATCH 04/11] migration: Remove errp parameter in migration_fd_process_incoming()

[Fabiano Rosas]

Avihai Horon <avihaih@nvidia.com> writes:

> Errp parameter in migration_fd_process_incoming() is unused.
> Remove it.
>
> Signed-off-by: Avihai Horon <avihaih@nvidia.com>

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

Re: [PATCH 05/11] migration/multifd: Fix error message in multifd_recv_initial_packet()

[Fabiano Rosas]

Avihai Horon <avihaih@nvidia.com> writes:

> In multifd_recv_initial_packet(), if MultiFDInit_t->id is greater than
> the configured number of multifd channels, an irrelevant error message
> about multifd version is printed.
>
> Change the error message to a relevant one about the channel id.
>
> Signed-off-by: Avihai Horon <avihaih@nvidia.com>

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

Re: [PATCH 07/11] migration/multifd: Fix leaking of Error in TLS error flow

[Fabiano Rosas]

Avihai Horon <avihaih@nvidia.com> writes:

> If there is an error in multifd TLS handshake task,
> multifd_tls_outgoing_handshake() retrieves the error with
> qio_task_propagate_error() but never frees it.
>
> Fix it by freeing the obtained Error.
>
> In addition, the error is not reported at all, so report it with
> migrate_set_error().
>
> Fixes: 29647140157a ("migration/tls: add support for multifd tls-handshake")
> Signed-off-by: Avihai Horon <avihaih@nvidia.com>

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

Re: [PATCH 08/11] migration/multifd: Remove error_setg() in migration_ioc_process_incoming()

[Fabiano Rosas]

Avihai Horon <avihaih@nvidia.com> writes:

> If multifd_load_setup() fails in migration_ioc_process_incoming(),
> error_setg() is called with errp. This will lead to an assert because in
> that case errp already contains an error.
>
> Fix it by removing the redundant error_setg().
>
> Fixes: 6720c2b32725 ("migration: check magic value for deciding the mapping of channels")
> Signed-off-by: Avihai Horon <avihaih@nvidia.com>

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

Re: [PATCH 09/11] migration: Fix migration_channel_read_peek() error path

[Fabiano Rosas]

Avihai Horon <avihaih@nvidia.com> writes:

> migration_channel_read_peek() calls qio_channel_readv_full() and handles
> both cases of return value == 0 and return value < 0 the same way, by
> calling error_setg() with errp. However, if return value < 0, errp is
> already set, so calling error_setg() with errp will lead to an assert.
>
> Fix it by handling these cases separately, calling error_setg() with
> errp only in return value == 0 case.
>
> Fixes: 6720c2b32725 ("migration: check magic value for deciding the mapping of channels")
> Signed-off-by: Avihai Horon <avihaih@nvidia.com>

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

Re: [PATCH 10/11] migration: Remove unnecessary usage of local Error

[Fabiano Rosas]

Avihai Horon <avihaih@nvidia.com> writes:

> According to Error API, usage of ERRP_GUARD() or a local Error instead
> of errp is needed if errp is passed to void functions, where it is later
> dereferenced to see if an error occurred.
>
> There are several places in migration.c that use local Error although it
> is not needed. Change these places to use errp directly.
>
> Signed-off-by: Avihai Horon <avihaih@nvidia.com>

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

Re: [PATCH 00/11] migration: Misc cleanups and fixes

[Peter Xu]

On Sun, Dec 31, 2023 at 11:30:05AM +0200, Avihai Horon wrote:
> Hi,
> 
> This series contains misc cleanups and fixes in migration code that I
> noticed while going over the code.

queued.

-- 
Peter Xu

Re: [PATCH 09/11] migration: Fix migration_channel_read_peek() error path

[Markus Armbruster]

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

> +Markus
>
> On 31/12/23 10:30, Avihai Horon wrote:
>> migration_channel_read_peek() calls qio_channel_readv_full() and handles
>> both cases of return value == 0 and return value < 0 the same way, by
>> calling error_setg() with errp. However, if return value < 0, errp is
>> already set, so calling error_setg() with errp will lead to an assert.
>
> I suppose the API would be safer by passing &len as argument and
> return a boolean.

Doubtful, unless I'm misunderstanding something.

Function comment:

 * Returns: the number of bytes read, or -1 on error,
 * or QIO_CHANNEL_ERR_BLOCK if no data is available
 * and the channel is non-blocking

I understand this as:

* Success case: return #bytes read

* Error case: return -1, @errp is set

* Would block case: return QIO_CHANNEL_ERR_BLOCK

A zero return value must be the success case.  I figure this can happen
only at EOF.  A caller might need to treat unexpected EOF as an error.

>> Fix it by handling these cases separately, calling error_setg() with
>> errp only in return value == 0 case.
>> Fixes: 6720c2b32725 ("migration: check magic value for deciding the mapping of channels")
>> Signed-off-by: Avihai Horon <avihaih@nvidia.com>
>> ---
>>   migration/channel.c | 9 ++++++---
>>   1 file changed, 6 insertions(+), 3 deletions(-)
>> diff --git a/migration/channel.c b/migration/channel.c
>> index ca3319a309..f9de064f3b 100644
>> --- a/migration/channel.c
>> +++ b/migration/channel.c
>> @@ -117,9 +117,12 @@ int migration_channel_read_peek(QIOChannel *ioc,
>>          len = qio_channel_readv_full(ioc, &iov, 1, NULL, NULL,
>>                                       QIO_CHANNEL_READ_FLAG_MSG_PEEK, errp);
>> -        if (len <= 0 && len != QIO_CHANNEL_ERR_BLOCK) {
>> -            error_setg(errp,
>> -                       "Failed to peek at channel");
>> +        if (len < 0 && len != QIO_CHANNEL_ERR_BLOCK) {
>> +            return -1;
>> +        }
>> +
>> +        if (len == 0) {
>> +            error_setg(errp, "Failed to peek at channel");
>>              return -1;
>>           }
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>