[PATCH 0/2] migration: Only pass negative values to qemu_file_set_error()

[PATCH 0/2] migration: Only pass negative values to qemu_file_set_error()

[Fabiano Rosas]

qemu_file_set_error() sets f->last_error, which is later used by
functions in qemu-file.c to return the error to callers.

The following functions expect f->last_error to be -errno:

qemu_file_get_error_obj()
qemu_file_get_error()
qemu_fclose()

Make sure qemu_file_set_error() always receives a negative number and
document the assumption.

Fabiano Rosas (2):
  target/arm: Return negative value on power state migration error
  migration: Make it clear that qemu_file_set_error() needs a negative
    value

 migration/qemu-file.c | 2 ++
 migration/savevm.c    | 6 +++---
 target/arm/machine.c  | 2 +-
 3 files changed, 6 insertions(+), 4 deletions(-)

-- 
2.35.3

[PATCH 1/2] target/arm: Return negative value on power state migration error

[Fabiano Rosas]

All migration hooks, except this one, return -1 on error and 0 on
success.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 target/arm/machine.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/arm/machine.c b/target/arm/machine.c
index fc4a4a4064..db3fcd5ae4 100644
--- a/target/arm/machine.c
+++ b/target/arm/machine.c
@@ -702,7 +702,7 @@ static int put_power(QEMUFile *f, void *opaque, size_t size,
         qemu_put_byte(f, powered_off);
         return 0;
     } else {
-        return 1;
+        return -1;
     }
 }
 
-- 
2.35.3

[PATCH 2/2] migration: Make it clear that qemu_file_set_error() needs a negative value

[Fabiano Rosas]

The convention in qemu-file.c is to return a negative value on
error.

The only place that could use qemu_file_set_error() to store a
positive value to f->last_error was vmstate_save() which has been
fixed in the previous patch.

bdrv_inactivate_all() already returns a negative value on error.

Document that qemu_file_set_error() needs -errno and alter the callers
to check ret < 0.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/qemu-file.c | 2 ++
 migration/savevm.c    | 6 +++---
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index acc282654a..8276bac248 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -222,6 +222,8 @@ int qemu_file_get_error(QEMUFile *f)
 
 /*
  * Set the last error for stream f
+ *
+ * The error ('ret') should be in -errno format.
  */
 void qemu_file_set_error(QEMUFile *f, int ret)
 {
diff --git a/migration/savevm.c b/migration/savevm.c
index 95c2abf47c..f3c303ab74 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1249,7 +1249,7 @@ void qemu_savevm_state_setup(QEMUFile *f)
     QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
         if (se->vmsd && se->vmsd->early_setup) {
             ret = vmstate_save(f, se, ms->vmdesc);
-            if (ret) {
+            if (ret < 0) {
                 qemu_file_set_error(f, ret);
                 break;
             }
@@ -1464,7 +1464,7 @@ int qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f,
         }
 
         ret = vmstate_save(f, se, vmdesc);
-        if (ret) {
+        if (ret < 0) {
             qemu_file_set_error(f, ret);
             return ret;
         }
@@ -1474,7 +1474,7 @@ int qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f,
         /* Inactivate before sending QEMU_VM_EOF so that the
          * bdrv_activate_all() on the other end won't fail. */
         ret = bdrv_inactivate_all();
-        if (ret) {
+        if (ret < 0) {
             error_report("%s: bdrv_inactivate_all() failed (%d)",
                          __func__, ret);
             qemu_file_set_error(f, ret);
-- 
2.35.3

Re: [PATCH 2/2] migration: Make it clear that qemu_file_set_error() needs a negative value

[Peter Maydell]

On Thu, 6 Jul 2023 at 20:52, Fabiano Rosas <farosas@suse.de> wrote:
>
> The convention in qemu-file.c is to return a negative value on
> error.
>
> The only place that could use qemu_file_set_error() to store a
> positive value to f->last_error was vmstate_save() which has been
> fixed in the previous patch.
>
> bdrv_inactivate_all() already returns a negative value on error.
>
> Document that qemu_file_set_error() needs -errno and alter the callers
> to check ret < 0.
>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  migration/qemu-file.c | 2 ++
>  migration/savevm.c    | 6 +++---
>  2 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/migration/qemu-file.c b/migration/qemu-file.c
> index acc282654a..8276bac248 100644
> --- a/migration/qemu-file.c
> +++ b/migration/qemu-file.c
> @@ -222,6 +222,8 @@ int qemu_file_get_error(QEMUFile *f)
>
>  /*
>   * Set the last error for stream f
> + *
> + * The error ('ret') should be in -errno format.
>   */
>  void qemu_file_set_error(QEMUFile *f, int ret)
>  {
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 95c2abf47c..f3c303ab74 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1249,7 +1249,7 @@ void qemu_savevm_state_setup(QEMUFile *f)
>      QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
>          if (se->vmsd && se->vmsd->early_setup) {
>              ret = vmstate_save(f, se, ms->vmdesc);
> -            if (ret) {
> +            if (ret < 0) {
>                  qemu_file_set_error(f, ret);

You say qemu_file_set_error() should take an errno,
but vmstate_save() doesn't return one. It will directly
return whatever the VMStateInfo put, pre_save, etc hooks
return, which isn't necessarily an errno. (Specifically,
patch 1 in this series makes a .put hook return -1,
rather than an errno. I'm guessing other implementations
might too, though it's a bit hard to find them. A
coccinelle script could probably locate them.)

The de-facto API for all these methods in the VMStateInfo
is "0 on success, non-0 on failure", because
 (a) we don't document what the actual API for these is
 (b) the code which calls the methods and tests their
     return result is just doing "if (ret)" tests

If we want to tighten this up to "return an errno"
I think we would need to
 (a) audit all the implementations
 (b) document the updated API in vmstate.h and perhaps
     also docs/devel/migration.rst

thanks
-- PMM

Re: [PATCH 2/2] migration: Make it clear that qemu_file_set_error() needs a negative value

[Fabiano Rosas]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Thu, 6 Jul 2023 at 20:52, Fabiano Rosas <farosas@suse.de> wrote:
>>
>> The convention in qemu-file.c is to return a negative value on
>> error.
>>
>> The only place that could use qemu_file_set_error() to store a
>> positive value to f->last_error was vmstate_save() which has been
>> fixed in the previous patch.
>>
>> bdrv_inactivate_all() already returns a negative value on error.
>>
>> Document that qemu_file_set_error() needs -errno and alter the callers
>> to check ret < 0.
>>
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>  migration/qemu-file.c | 2 ++
>>  migration/savevm.c    | 6 +++---
>>  2 files changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/migration/qemu-file.c b/migration/qemu-file.c
>> index acc282654a..8276bac248 100644
>> --- a/migration/qemu-file.c
>> +++ b/migration/qemu-file.c
>> @@ -222,6 +222,8 @@ int qemu_file_get_error(QEMUFile *f)
>>
>>  /*
>>   * Set the last error for stream f
>> + *
>> + * The error ('ret') should be in -errno format.
>>   */
>>  void qemu_file_set_error(QEMUFile *f, int ret)
>>  {
>> diff --git a/migration/savevm.c b/migration/savevm.c
>> index 95c2abf47c..f3c303ab74 100644
>> --- a/migration/savevm.c
>> +++ b/migration/savevm.c
>> @@ -1249,7 +1249,7 @@ void qemu_savevm_state_setup(QEMUFile *f)
>>      QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
>>          if (se->vmsd && se->vmsd->early_setup) {
>>              ret = vmstate_save(f, se, ms->vmdesc);
>> -            if (ret) {
>> +            if (ret < 0) {
>>                  qemu_file_set_error(f, ret);
>
> You say qemu_file_set_error() should take an errno,
> but vmstate_save() doesn't return one. It will directly
> return whatever the VMStateInfo put, pre_save, etc hooks
> return, which isn't necessarily an errno. (Specifically,
> patch 1 in this series makes a .put hook return -1,
> rather than an errno. I'm guessing other implementations
> might too, though it's a bit hard to find them. A
> coccinelle script could probably locate them.)
>

All implementations return either 0, -1 or some errno; that one instance
from patch 1 returns 1. But you're right, those -1 are not really errno,
they are just "some negative value".

Since qemu-file.c puts the error through the error.c functions and those
call strerror(), all values that will go into qemu_file_set_error()
should be proper errnos.

I should probably audit users of qemu_file_set_error() instead and stop
using it for errors that have nothing to do with the actual migration
stream/QEMUFile. Currently it seems to have morphed into a mechanism to
record generic migration errors.