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

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(-)

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

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.