Re: [PATCH QEMU HCI-8.1 v2 10/24] 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 prepends for any ret
--

commit 8108694e711b23e4f49933352e133e7c3dec993c
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 when a nested
vmstate_save_state() or vmstate_save_state_v() call fails. It sets the Error
only for leaf put() failures, and it changes the field-level error_setg()
into 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
> --- 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");
> +                    }
>                  }

[Severity: Medium]
Can a positive return from put() leave *errp unset here?

The new error_setg() runs only for ret < 0, but the failure check further
down in vmstate_save_state_v() is a plain "if (ret)", which also runs for
positive values.

In-tree, put_power() in target/arm/machine.c (vmstate_powered_off) returns 1
when cpu->power_state is neither PSCI_ON nor PSCI_OFF:

    } else {
        return 1;
    }

With that return value, no Error is created at this point.

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

Is it safe to call error_prepend() on this path when *errp is NULL?

error_vprepend() in util/error.c checks only for a NULL errp, then reads the
message:

    if (!errp) {
        return;
    }
    ...
    g_string_append(newmsg, (*errp)->msg);

If a caller passed a non-NULL errp, such as a local Error pointer or
&error_fatal, and put_power() returned 1, would this dereference a NULL
*errp and crash QEMU during migration save?

Before this patch, error_setg() ran for every non-zero ret, so this case was
handled. Checking the series up to its final commit, I found no change to
migration/vmstate.c or migration/vmstate-types.c that addresses it.

Would it work to make the leaf-put condition "if (ret)" instead of
"if (ret < 0)", or otherwise ensure an Error is set before error_prepend()
runs?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261013-vfio_v1-v2-0-433455c0a294@virtuozzo.com?part=10