Hi
On Tue, Aug 5, 2025 at 10:31 PM Arun Menon <arme...@redhat.com> wrote:
> This is an incremental step in converting vmstate loading
> code to report error via Error objects instead of directly
> printing it to console/monitor.
> It is ensured that loadvm_process_enable_colo() must report an error
> in errp, in case of failure.
>
> Signed-off-by: Arun Menon <arme...@redhat.com>
>
Reviewed-by: Marc-André Lureau <marcandre.lur...@redhat.com>
> ---
> include/migration/colo.h | 2 +-
> migration/migration.c | 12 ++++++------
> migration/ram.c | 8 ++++----
> migration/ram.h | 2 +-
> migration/savevm.c | 26 ++++++++++++++------------
> 5 files changed, 26 insertions(+), 24 deletions(-)
>
> diff --git a/include/migration/colo.h b/include/migration/colo.h
> index
> 43222ef5ae6adc3f7d8aa6a48bef79af33d09208..d4fe422e4d335d3bef4f860f56400fcd73287a0e
> 100644
> --- a/include/migration/colo.h
> +++ b/include/migration/colo.h
> @@ -25,7 +25,7 @@ void migrate_start_colo_process(MigrationState *s);
> bool migration_in_colo_state(void);
>
> /* loadvm */
> -int migration_incoming_enable_colo(void);
> +int migration_incoming_enable_colo(Error **errp);
> void migration_incoming_disable_colo(void);
> bool migration_incoming_colo_enabled(void);
> bool migration_incoming_in_colo_state(void);
> diff --git a/migration/migration.c b/migration/migration.c
> index
> 6962dc7d7f3e0121d28994c98f12f9f2258343d7..4a76d7ed730589bae87115368b0bf4819f8b161e
> 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -623,22 +623,22 @@ void migration_incoming_disable_colo(void)
> migration_colo_enabled = false;
> }
>
> -int migration_incoming_enable_colo(void)
> +int migration_incoming_enable_colo(Error **errp)
> {
> #ifndef CONFIG_REPLICATION
> - error_report("ENABLE_COLO command come in migration stream, but the "
> - "replication module is not built in");
> + error_setg(errp, "ENABLE_COLO command come in migration stream, but
> the "
> + "replication module is not built in");
> return -ENOTSUP;
> #endif
>
> if (!migrate_colo()) {
> - error_report("ENABLE_COLO command come in migration stream, but
> x-colo "
> - "capability is not set");
> + error_setg(errp, "ENABLE_COLO command come in migration stream"
> + ", but x-colo capability is not set");
> return -EINVAL;
> }
>
> if (ram_block_discard_disable(true)) {
> - error_report("COLO: cannot disable RAM discard");
> + error_setg(errp, "COLO: cannot disable RAM discard");
> return -EBUSY;
> }
> migration_colo_enabled = true;
> diff --git a/migration/ram.c b/migration/ram.c
> index
> 6a0dcc04f436524a37672c41c38f201f06773374..995431c9e320f443c385c29d664d62e18c1afd90
> 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -3576,7 +3576,7 @@ static void colo_init_ram_state(void)
> * memory of the secondary VM, it is need to hold the global lock
> * to call this helper.
> */
> -int colo_init_ram_cache(void)
> +int colo_init_ram_cache(Error **errp)
> {
> RAMBlock *block;
>
> @@ -3585,9 +3585,9 @@ int colo_init_ram_cache(void)
> block->colo_cache = qemu_anon_ram_alloc(block->used_length,
> NULL, false, false);
> if (!block->colo_cache) {
> - error_report("%s: Can't alloc memory for COLO cache of
> block %s,"
> - "size 0x" RAM_ADDR_FMT, __func__,
> block->idstr,
> - block->used_length);
> + error_setg(errp, "Can't alloc memory for COLO cache of "
> + "block %s, size 0x" RAM_ADDR_FMT,
> + block->idstr, block->used_length);
> RAMBLOCK_FOREACH_NOT_IGNORED(block) {
> if (block->colo_cache) {
> qemu_anon_ram_free(block->colo_cache,
> block->used_length);
> diff --git a/migration/ram.h b/migration/ram.h
> index
> 275709a99187f9429ccb4111e05281ec268ba0db..24cd0bf585762cfa1e86834dc03c6baeea2f0627
> 100644
> --- a/migration/ram.h
> +++ b/migration/ram.h
> @@ -109,7 +109,7 @@ void ramblock_set_file_bmap_atomic(RAMBlock *block,
> ram_addr_t offset,
> bool set);
>
> /* ram cache */
> -int colo_init_ram_cache(void);
> +int colo_init_ram_cache(Error **errp);
> void colo_flush_ram_cache(void);
> void colo_release_ram_cache(void);
> void colo_incoming_start_dirty_log(void);
> diff --git a/migration/savevm.c b/migration/savevm.c
> index
> 938adb20270adbf9546b0884d0877c25c3f0f816..a6b71a958aeda31e89043f8103bfe2fc89542fb5
> 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2519,15 +2519,21 @@ static int
> loadvm_handle_recv_bitmap(MigrationIncomingState *mis,
> return 0;
> }
>
> -static int loadvm_process_enable_colo(MigrationIncomingState *mis)
> +static int loadvm_process_enable_colo(MigrationIncomingState *mis,
> + Error **errp)
> {
> - int ret = migration_incoming_enable_colo();
> + ERRP_GUARD();
> + int ret;
>
> - if (!ret) {
> - ret = colo_init_ram_cache();
> - if (ret) {
> - migration_incoming_disable_colo();
> - }
> + ret = migration_incoming_enable_colo(errp);
> + if (ret < 0) {
> + return ret;
> + }
> +
> + ret = colo_init_ram_cache(errp);
> + if (ret) {
>
Note: here you keep the "ret" error condition !=0, ok.
colo_init_ram_cache(), returns -errno on error. Although errno should
remain unchanged on success (during qemu_anon_ram_free etc), I think it
would be safer to convert the function to follow the recommended
bool-valued function for true on success / false on failure instead.
> + error_prepend(errp, "failed to init colo RAM cache: %d: ", ret);
> + migration_incoming_disable_colo();
>
}
> return ret;
> }
> @@ -2646,11 +2652,7 @@ static int loadvm_process_command(QEMUFile *f,
> Error **errp)
> return loadvm_handle_recv_bitmap(mis, len, errp);
>
> case MIG_CMD_ENABLE_COLO:
> - ret = loadvm_process_enable_colo(mis);
> - if (ret < 0) {
> - error_setg(errp, "Failed to load device state command: %d",
> ret);
> - }
> - return ret;
> + return loadvm_process_enable_colo(mis, errp);
>
> case MIG_CMD_SWITCHOVER_START:
> ret = loadvm_postcopy_handle_switchover_start();
>
> --
> 2.50.1
>
>
