Hi Marc-André,
Thanks for the review and the Reviewed-by tag.
On Wed, Aug 06, 2025 at 12:07:25PM +0400, Marc-André Lureau wrote:
> 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.
There has been some discussion on this here:
https://lore.kernel.org/all/ah5atucji3hyx...@redhat.com/
It has been advised not to use bool return.
>
>
> > + 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
> >
> >
Regards,
Arun
