Re: [PATCH v4 05/12] migration: qemu_file_set_blocking(): add errp parameter

[Peter Xu <peterx@redhat.com>]

On Mon, Sep 15, 2025 at 10:30:57PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> qemu_file_set_blocking() is a wrapper on qio_channel_set_blocking(),
> so let's passthrough the errp.

This looks all reasonable in general.

Said that, using error_abort in migration code normally are not suggested
because it's too strong.

I did check all of below should be on the incoming side which is not as
severe (because killing dest qemu before switchover is normally
benign). Still, can we switch all below users to error_warn (including the
one below that may want to error_report_err(), IMHO a warn report is fine
even for such error)?

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  migration/colo.c         | 5 ++++-
>  migration/migration.c    | 8 +++++---
>  migration/postcopy-ram.c | 2 +-
>  migration/qemu-file.c    | 4 ++--
>  migration/qemu-file.h    | 2 +-
>  migration/savevm.c       | 4 ++--
>  6 files changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/migration/colo.c b/migration/colo.c
> index e0f713c837..cf4d71d9ed 100644
> --- a/migration/colo.c
> +++ b/migration/colo.c
> @@ -859,7 +859,10 @@ static void *colo_process_incoming_thread(void *opaque)
>       * coroutine, and here we are in the COLO incoming thread, so it is ok to
>       * set the fd back to blocked.
>       */
> -    qemu_file_set_blocking(mis->from_src_file, true);
> +    if (!qemu_file_set_blocking(mis->from_src_file, true, &local_err)) {
> +        error_report_err(local_err);
> +        goto out;
> +    }
>  
>      colo_incoming_start_dirty_log();
>  
> diff --git a/migration/migration.c b/migration/migration.c
> index 10c216d25d..e1ac4d73c2 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -951,7 +951,7 @@ static void migration_incoming_setup(QEMUFile *f)
>  
>      assert(!mis->from_src_file);
>      mis->from_src_file = f;
> -    qemu_file_set_blocking(f, false);
> +    qemu_file_set_blocking(f, false, &error_abort);
>  }
>  
>  void migration_incoming_process(void)
> @@ -971,7 +971,7 @@ static bool postcopy_try_recover(void)
>          /* This should be set already in migration_incoming_setup() */
>          assert(mis->from_src_file);
>          /* Postcopy has standalone thread to do vm load */
> -        qemu_file_set_blocking(mis->from_src_file, true);
> +        qemu_file_set_blocking(mis->from_src_file, true, &error_abort);
>  
>          /* Re-configure the return path */
>          mis->to_src_file = qemu_file_get_return_path(mis->from_src_file);
> @@ -4002,7 +4002,9 @@ void migration_connect(MigrationState *s, Error *error_in)
>      }
>  
>      migration_rate_set(rate_limit);
> -    qemu_file_set_blocking(s->to_dst_file, true);
> +    if (!qemu_file_set_blocking(s->to_dst_file, true, &local_err)) {
> +        goto fail;
> +    }
>  
>      /*
>       * Open the return path. For postcopy, it is used exclusively. For
> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> index 45af9a361e..0172172343 100644
> --- a/migration/postcopy-ram.c
> +++ b/migration/postcopy-ram.c
> @@ -1909,7 +1909,7 @@ void postcopy_preempt_new_channel(MigrationIncomingState *mis, QEMUFile *file)
>       * The new loading channel has its own threads, so it needs to be
>       * blocked too.  It's by default true, just be explicit.
>       */
> -    qemu_file_set_blocking(file, true);
> +    qemu_file_set_blocking(file, true, &error_abort);
>      mis->postcopy_qemufile_dst = file;
>      qemu_sem_post(&mis->postcopy_qemufile_dst_done);
>      trace_postcopy_preempt_new_channel();
> diff --git a/migration/qemu-file.c b/migration/qemu-file.c
> index d5c6e7ec61..0f4280df21 100644
> --- a/migration/qemu-file.c
> +++ b/migration/qemu-file.c
> @@ -888,9 +888,9 @@ void qemu_put_counted_string(QEMUFile *f, const char *str)
>   *       both directions, and thus changing the blocking on the main
>   *       QEMUFile can also affect the return path.
>   */
> -void qemu_file_set_blocking(QEMUFile *f, bool block)
> +bool qemu_file_set_blocking(QEMUFile *f, bool block, Error **errp)
>  {
> -    qio_channel_set_blocking(f->ioc, block, NULL);
> +    return qio_channel_set_blocking(f->ioc, block, errp);
>  }
>  
>  /*
> diff --git a/migration/qemu-file.h b/migration/qemu-file.h
> index f5b9f430e0..c13c967167 100644
> --- a/migration/qemu-file.h
> +++ b/migration/qemu-file.h
> @@ -71,7 +71,7 @@ void qemu_file_set_error(QEMUFile *f, int ret);
>  int qemu_file_shutdown(QEMUFile *f);
>  QEMUFile *qemu_file_get_return_path(QEMUFile *f);
>  int qemu_fflush(QEMUFile *f);
> -void qemu_file_set_blocking(QEMUFile *f, bool block);
> +bool qemu_file_set_blocking(QEMUFile *f, bool block, Error **errp);
>  int qemu_file_get_to_fd(QEMUFile *f, int fd, size_t size);
>  void qemu_set_offset(QEMUFile *f, off_t off, int whence);
>  off_t qemu_get_offset(QEMUFile *f);
> diff --git a/migration/savevm.c b/migration/savevm.c
> index fabbeb296a..abe0547f9b 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2095,7 +2095,7 @@ static void *postcopy_ram_listen_thread(void *opaque)
>       * Because we're a thread and not a coroutine we can't yield
>       * in qemu_file, and thus we must be blocking now.
>       */
> -    qemu_file_set_blocking(f, true);
> +    qemu_file_set_blocking(f, true, &error_fatal);
>  
>      /* TODO: sanity check that only postcopiable data will be loaded here */
>      load_res = qemu_loadvm_state_main(f, mis);
> @@ -2108,7 +2108,7 @@ static void *postcopy_ram_listen_thread(void *opaque)
>      f = mis->from_src_file;
>  
>      /* And non-blocking again so we don't block in any cleanup */
> -    qemu_file_set_blocking(f, false);
> +    qemu_file_set_blocking(f, false, &error_fatal);
>  
>      trace_postcopy_ram_listen_thread_exit();
>      if (load_res < 0) {
> -- 
> 2.48.1
> 

-- 
Peter Xu