Re: [PATCH QEMU HCI-8.1 v6 17/34] 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() sets Error only for ret < 0 but error_prepend() runs for any ret != 0
--

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

migration: vmstate_save_state_v: fix double error_setg

This patch avoids calling error_setg() twice on the same errp when a nested
vmstate_save_state() or vmstate_save_state_v() fails. It sets an Error only
when put() returns a negative value, and it changes the common failure
branch to use error_prepend().

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");
> +                    }
>                  }

[ ... ]

> @@ -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);

[Severity: Medium]
Can this error_prepend() run with *errp still NULL? The put() branch sets
an Error only when ret < 0, but this branch is taken for any non-zero ret:

    ret = inner_field->info->put(f, curr_elem, size,
                                 inner_field, vmdesc_loop);
    if (ret < 0) {
        error_setg(errp, "put failed");
    }
    ...
    if (ret) {
        error_prepend(errp, "Save of field %s/%s failed: ", ...);

If put() returns a positive value, error_setg() is skipped. error_prepend()
then calls error_vprepend(), which checks only for a NULL errp and not a
NULL *errp:

    if (!errp) {
        return;
    }
    newmsg = g_string_new(NULL);
    g_string_vprintf(newmsg, fmt, ap);
    g_string_append(newmsg, (*errp)->msg);

That would dereference a NULL Error and crash the migration source.

The nested paths look similar. put_tmp() in vmstate-types.c reports the
inner failure through a local_err and returns the nested ret unchanged:

    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() behave the same way. If the
nested ret is positive, for example from a pre_save callback, it reaches
the outer branch with ret < 0 false and *errp unset.

I could not find an in-tree put() callback that returns a positive value.
The ones I read return 0 or a negative errno. The VMStateInfo.put contract
in include/migration/vmstate.h doesn't say that only negative values are
allowed.

Before this patch, error_setg() ran for any non-zero ret, so this mismatch
is new. The pre_save and post_save paths are not affected, because both
call error_setg() for any non-zero return.

Would it be better to use "if (ret)" instead of "if (ret < 0)" for the
put() case, or to normalise ret to a negative value?

>                      if (vmsd->post_save) {
>                          vmsd->post_save(opaque);
>                      }

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261102-vfio_v1-v6-0-9bc7cabc850d@virtuozzo.com?part=17