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.
In general I would consider it bad practice to have an method
with "Error **errp" that also returns an errno. 95% of the time
the errno is just there as further info on the error scenario,
which "errp" obsoletes. Only in the rare cases where the caller
needs to take functional action based on errno values, is it
appropriate to contine returning '-errno'.
IOW, I'd consider this appropriate for the patch as is.
> 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.
That doesn't guarantee avoidance of bugs, as bool values can be
assigned to & compared against integers.
> > 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;
> > }
This is a definite anti-pattern though as it treats positive return
values as errors, contrary to the documented API for both the old
and new code.
So change this to "< 0" while we're here.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
