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