Re: [PATCH QEMU HCI-8.1 v5 17/36] migration: vmstate_save_state_v: fix double error_setg

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] migration: vmstate_save_state_v() error_prepend() on unset errp if put() returns >0
--

commit 02288718e44ad995a2dd7f4f453ddcf9d949e513
Author: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

migration: vmstate_save_state_v: fix double error_setg

This commit avoids calling error_setg() twice on the same errp in
vmstate_save_state_v(). It does this by setting the Error only when
info->put() fails with a negative return, and by changing the later
failure path to use error_prepend() instead of error_setg().

Link: https://lore.kernel.org/qemu-devel/20260304212303.667141-2-vsementsov@yandex-team.ru

> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index aea41c099463..04903131257b 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c

[ ... ]

> @@ -523,6 +523,9 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
>                  } else {
>                      ret = inner_field->info->put(f, curr_elem, size,
>                                                   inner_field, vmdesc_loop);
> +                    if (ret < 0) {
> +                        error_setg(errp, "put failed");
> +                    }
>                  }
>  
>                  written_bytes = qemu_file_transferred(f) - old_offset;

[ ... ]

> @@ -535,8 +538,8 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
>                  }
>  
>                  if (ret) {
> -                    error_setg(errp, "Save of field %s/%s failed",
> -                                vmsd->name, field->name);
> +                    error_prepend(errp, "Save of field %s/%s failed: ",
> +                                  vmsd->name, field->name);
>                      if (vmsd->post_save) {
>                          vmsd->post_save(opaque);
>                      }

[Severity: Medium]
Can error_prepend() run here with *errp still unset?

The Error is only set after put() when ret < 0, but this failure block
runs for any non-zero ret. If info->put() returns a positive value,
nothing sets *errp, and error_prepend() ends up in error_vprepend(),
which checks only !errp before reading (*errp)->msg:

util/error.c:error_vprepend() {
    ...
    if (!errp) {
        return;
    }
    ...
    g_string_append(newmsg, (*errp)->msg);
}

Before this patch, error_setg() ran for every non-zero ret, so a
positive return was handled.

A positive ret can also come from the nested path. put_tmp() in
migration/vmstate-types.c calls vmstate_save_state() with a private
local_err and returns the raw inner ret:

    ret = vmstate_save_state(f, vmsd, tmp, vmdesc, &local_err);
    if (ret) {
        error_report_err(local_err);
    }
    g_free(tmp);

    return ret;

put_qtailq(), put_qlist() and put_gtree() follow the same pattern. If a
pre_save or post_save hook in such a nested vmsd returns a positive
value, vmstate_save_state_v() treats it as failure and the outer errp
stays unset.

Positive pre_save returns do exist in the tree, for example
qxl_pre_save() in hw/display/qxl.c returns 1. That hook is only on a
top-level vmsd today, where the pre-save failure path sets errp. I did
not find an in-tree put() or nested vmsd that currently triggers this,
so the problem looks latent.

If errp were &error_warn, error_setg() would report and free the Error
without storing it, and error_prepend() would then crash the same way.
I found no caller that passes it.

Would it be safer to set the Error for any non-zero ret from put(), for
example with if (ret), or to only call error_prepend() when *errp is
set?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261023-vfio_v1-v5-0-ba0bf3c6d122@virtuozzo.com?part=17