Peter Maydell <peter.mayd...@linaro.org> writes: > On Thu, 6 Jul 2023 at 20:52, Fabiano Rosas <faro...@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 <faro...@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.