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

[Fabiano Rosas <farosas@suse.de>]

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.