On Tue, Oct 15, 2024 at 04:15:15PM +0200, Hanna Czenczek wrote:
> migration/savevm.c contains some calls to vmstate_save() that are
> followed by migrate_set_error() if the integer return value indicates an
> error. migrate_set_error() requires that the `Error *` object passed to
> it is set. Therefore, vmstate_save() is assumed to always set *errp on
> error.
>
> Right now, that assumption is not met: vmstate_save_state_v() (called
> internally by vmstate_save()) will not set *errp if
> vmstate_subsection_save() or vmsd->post_save() fail. Fix that by adding
> an *errp parameter to vmstate_subsection_save(), and by generating a
> generic error in case post_save() fails (as is already done for
> pre_save()).
>
> Without this patch, qemu will crash after vmstate_subsection_save() or
> post_save() have failed inside of a vmstate_save() call (unless
> migrate_set_error() then happen to discard the new error because
> s->error is already set). This happens e.g. when receiving the state
> from a virtio-fs back-end (virtiofsd) fails.
>
> Signed-off-by: Hanna Czenczek <hre...@redhat.com>
> ---
> migration/vmstate.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index ff5d589a6d..13532f2807 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -22,7 +22,8 @@
> #include "trace.h"
>
> static int vmstate_subsection_save(QEMUFile *f, const VMStateDescription
> *vmsd,
> - void *opaque, JSONWriter *vmdesc);
> + void *opaque, JSONWriter *vmdesc,
> + Error **errp);
> static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription
> *vmsd,
> void *opaque);
>
> @@ -441,12 +442,13 @@ int vmstate_save_state_v(QEMUFile *f, const
> VMStateDescription *vmsd,
> json_writer_end_array(vmdesc);
> }
>
> - ret = vmstate_subsection_save(f, vmsd, opaque, vmdesc);
> + ret = vmstate_subsection_save(f, vmsd, opaque, vmdesc, errp);
>
> if (vmsd->post_save) {
> int ps_ret = vmsd->post_save(opaque);
> if (!ret) {
Perhaps here it needs to be "if (!ret && ps_ret)" now, otherwise the error
will be attached even if no error for both retvals?
Other than that it looks good.
Thanks,
> ret = ps_ret;
> + error_setg(errp, "post-save failed: %s", vmsd->name);
> }
> }
> return ret;
> @@ -518,7 +520,8 @@ static int vmstate_subsection_load(QEMUFile *f, const
> VMStateDescription *vmsd,
> }
>
> static int vmstate_subsection_save(QEMUFile *f, const VMStateDescription
> *vmsd,
> - void *opaque, JSONWriter *vmdesc)
> + void *opaque, JSONWriter *vmdesc,
> + Error **errp)
> {
> const VMStateDescription * const *sub = vmsd->subsections;
> bool vmdesc_has_subsections = false;
> @@ -546,7 +549,7 @@ static int vmstate_subsection_save(QEMUFile *f, const
> VMStateDescription *vmsd,
> qemu_put_byte(f, len);
> qemu_put_buffer(f, (uint8_t *)vmsdsub->name, len);
> qemu_put_be32(f, vmsdsub->version_id);
> - ret = vmstate_save_state(f, vmsdsub, opaque, vmdesc);
> + ret = vmstate_save_state_with_err(f, vmsdsub, opaque, vmdesc,
> errp);
> if (ret) {
> return ret;
> }
> --
> 2.45.2
>
--
Peter Xu
