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 <armenon@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
*
* @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;
}
--
2.50.0On 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 <armenon@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.
> *
> * @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;
> }
>
>
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 <armenon@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 :|
On 2025/07/18 2:02, Daniel P. Berrangé wrote:
> 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 <armenon@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.
Returning -errno with "Error **errp" is indeed a bad practice, but
someone may write a return statement with -errno by mistake as long as
the type is int. For callers, it is a good defensive coding practice to
assume any negative value indicates an error as it will dismiss the
difference of -1 or -errno, and it is what the existing documentation
implicitly suggests.
On the other hand, a function can never have a bug to return -errno if
its return value type is bool.
Regards,
Akihiko Odaki
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 <armenon@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
© 2016 - 2025 Red Hat, Inc.