Hi Akihiko,
Thanks for the review.
On Thu, Jul 17, 2025 at 12:34:21PM +0900, Akihiko Odaki wrote:
> On 2025/07/17 9:37, Arun Menon 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 ram_postcopy_incoming_init() must report an error
> > in errp, in case of failure.
> >
> > Signed-off-by: Arun Menon <arme...@redhat.com>
> > ---
> > migration/postcopy-ram.c | 9 ++++++---
> > migration/postcopy-ram.h | 2 +-
> > migration/ram.c | 6 +++---
> > migration/ram.h | 2 +-
> > migration/savevm.c | 2 +-
> > 5 files changed, 12 insertions(+), 9 deletions(-)
> >
> > diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> > index
> > 45af9a361e8eacaad0fb217a5da2c5004416c1da..05617e5fbcad62226a54fe17d9f7d9a316baf1e4
> > 100644
> > --- a/migration/postcopy-ram.c
> > +++ b/migration/postcopy-ram.c
> > @@ -681,6 +681,7 @@ out:
> > */
> > static int init_range(RAMBlock *rb, void *opaque)
> > {
> > + Error **errp = opaque;
> > const char *block_name = qemu_ram_get_idstr(rb);
> > void *host_addr = qemu_ram_get_host_addr(rb);
> > ram_addr_t offset = qemu_ram_get_offset(rb);
> > @@ -701,6 +702,8 @@ static int init_range(RAMBlock *rb, void *opaque)
> > * (Precopy will just overwrite this data, so doesn't need the
> > discard)
> > */
> > if (ram_discard_range(block_name, 0, length)) {
> > + error_setg(errp, "failed to discard RAM block %s len=%zu",
> > + block_name, length);
> > return -1;
> > }
> > @@ -749,9 +752,9 @@ static int cleanup_range(RAMBlock *rb, void *opaque)
> > * postcopy later; must be called prior to any precopy.
> > * called from arch_init's similarly named ram_postcopy_incoming_init
> > */
> > -int postcopy_ram_incoming_init(MigrationIncomingState *mis)
> > +int postcopy_ram_incoming_init(MigrationIncomingState *mis, Error **errp)
> > {
> > - if (foreach_not_ignored_block(init_range, NULL)) {
> > + if (foreach_not_ignored_block(init_range, errp)) {
> > return -1;
> > }
> > @@ -1703,7 +1706,7 @@ bool
> > postcopy_ram_supported_by_host(MigrationIncomingState *mis, Error **errp)
> > return false;
> > }
> > -int postcopy_ram_incoming_init(MigrationIncomingState *mis)
> > +int postcopy_ram_incoming_init(MigrationIncomingState *mis, Error **errp)
> > {
> > error_report("postcopy_ram_incoming_init: No OS support");
> > return -1;
> > diff --git a/migration/postcopy-ram.h b/migration/postcopy-ram.h
> > index
> > 3852141d7e37ab18bada4b46c137fef0969d0070..ca19433b246893fa5105bcebffb442c58a9a4f48
> > 100644
> > --- a/migration/postcopy-ram.h
> > +++ b/migration/postcopy-ram.h
> > @@ -30,7 +30,7 @@ int postcopy_ram_incoming_setup(MigrationIncomingState
> > *mis);
> > * postcopy later; must be called prior to any precopy.
> > * called from ram.c's similarly named ram_postcopy_incoming_init
> > */
> > -int postcopy_ram_incoming_init(MigrationIncomingState *mis);
> > +int postcopy_ram_incoming_init(MigrationIncomingState *mis, Error **errp);
> > /*
> > * At the end of a migration where postcopy_ram_incoming_init was called.
> > diff --git a/migration/ram.c b/migration/ram.c
> > index
> > 7208bc114fb5c366740db380ee6956a91b3871a0..8223183132dc0f558f45fbae3f4f832845730bd3
> > 100644
> > --- a/migration/ram.c
> > +++ b/migration/ram.c
> > @@ -3708,7 +3708,7 @@ static int ram_load_cleanup(void *opaque)
> > /**
> > * ram_postcopy_incoming_init: allocate postcopy data structures
> > *
> > - * Returns 0 for success and negative if there was one error
> > + * Returns 0 for success and -1 if there was one error
>
> This is true but not relevant in this patch's goal.
>
> Besides, I'm not in favor of letting callers make an assumption on integer
> return values (i.e., let callers assume a particular integer value in the
> error conditions). It is subtle to make a mistake to return -errno while the
> documentation says it returns -1.
>
> I think a proper way to avoid bugs due to return values here is to change
> the type to bool, which ensures there are two possible values; that is a
> nice improvement but something that can be done later.
I agree, I shall remove the unnecessary documentation change as it is not in
the scope.
>
> > *
> > * @mis: current migration incoming state
> > *
> > @@ -3716,9 +3716,9 @@ static int ram_load_cleanup(void *opaque)
> > * postcopy-ram. postcopy-ram's similarly names
> > * postcopy_ram_incoming_init does the work.
> > */
> > -int ram_postcopy_incoming_init(MigrationIncomingState *mis)
> > +int ram_postcopy_incoming_init(MigrationIncomingState *mis, Error **errp)
> > {
> > - return postcopy_ram_incoming_init(mis);
> > + return postcopy_ram_incoming_init(mis, errp);
> > }
> > /**
> > diff --git a/migration/ram.h b/migration/ram.h
> > index
> > 921c39a2c5c45bc2344be80854c46e4c10c09aeb..275709a99187f9429ccb4111e05281ec268ba0db
> > 100644
> > --- a/migration/ram.h
> > +++ b/migration/ram.h
> > @@ -86,7 +86,7 @@ void ram_postcopy_migrated_memory_release(MigrationState
> > *ms);
> > void ram_postcopy_send_discard_bitmap(MigrationState *ms);
> > /* For incoming postcopy discard */
> > int ram_discard_range(const char *block_name, uint64_t start, size_t
> > length);
> > -int ram_postcopy_incoming_init(MigrationIncomingState *mis);
> > +int ram_postcopy_incoming_init(MigrationIncomingState *mis, Error **errp);
> > int ram_load_postcopy(QEMUFile *f, int channel);
> > void ram_handle_zero(void *host, uint64_t size);
> > diff --git a/migration/savevm.c b/migration/savevm.c
> > index
> > d1edeaac5f2a5df2f6d94357388be807a938b2ef..8eba151a693b7f2dc58853292c92024288eae81e
> > 100644
> > --- a/migration/savevm.c
> > +++ b/migration/savevm.c
> > @@ -1983,7 +1983,7 @@ static int
> > loadvm_postcopy_handle_advise(MigrationIncomingState *mis,
> > return -1;
> > }
> > - if (ram_postcopy_incoming_init(mis)) {
> > + if (ram_postcopy_incoming_init(mis, NULL)) {
> > return -1;
> > }
> >
>
Regards,
Arun
