Hi
On Wed, Aug 6, 2025 at 1:58 PM Arun Menon <arme...@redhat.com> wrote:
>
> 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.
Ok, but the usage of errno in that function seems fragile as other
functions could clear it before it is returned. Can you make another
patch to change the return value to -1 and document it?
thanks
>
> >
> >
> > > + 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
>
>
--
Marc-André Lureau
