Hi Marc-André,
Thanks for the review.
On Wed, Aug 06, 2025 at 12:19:29PM +0400, Marc-André Lureau wrote:
> Hi
>
> On Tue, Aug 5, 2025 at 10:31 PM Arun Menon <armenon@redhat.com> wrote:
>
> > The original vmstate_save_state_v() function combined multiple
> > responsibilities like calling pre-save hooks, saving the state of
> > the device, handling subsection saves and invoking post-save hooks.
> > This led to a lengthy and less readable function.
> >
> > To address this, introduce wrapper functions for pre-save,
> > post-save and the device-state save functionalities.
> >
> > Signed-off-by: Arun Menon <armenon@redhat.com>
> > ---
> > migration/vmstate.c | 78
> > ++++++++++++++++++++++++++++++++++++++++-------------
> > 1 file changed, 60 insertions(+), 18 deletions(-)
> >
> > diff --git a/migration/vmstate.c b/migration/vmstate.c
> > index
> > 60ff38858cf54277992fa5eddeadb6f3d70edec3..fbc59caadbbcc75fe6de27b459aa9aa25e76aa0a
> > 100644
> > --- a/migration/vmstate.c
> > +++ b/migration/vmstate.c
> > @@ -414,22 +414,43 @@ int vmstate_save_state_with_err(QEMUFile *f, const
> > VMStateDescription *vmsd,
> > return vmstate_save_state_v(f, vmsd, opaque, vmdesc_id,
> > vmsd->version_id, errp);
> > }
> >
> > -int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
> > - void *opaque, JSONWriter *vmdesc, int
> > version_id, Error **errp)
> > +static int pre_save_dispatch(const VMStateDescription *vmsd, void *opaque,
> > + Error **errp)
> > {
> > int ret = 0;
> > - const VMStateField *field = vmsd->fields;
> > -
> > - trace_vmstate_save_state_top(vmsd->name);
> > -
> > if (vmsd->pre_save) {
> > ret = vmsd->pre_save(opaque);
> > trace_vmstate_save_state_pre_save_res(vmsd->name, ret);
> > if (ret) {
> > - error_setg(errp, "pre-save failed: %s", vmsd->name);
> > - return ret;
> > + error_setg(errp, "pre-save for %s failed, ret: %d",
> > + vmsd->name, ret);
> > }
> > }
> > + return ret;
> > +}
> > +
> > +static int post_save_dispatch(const VMStateDescription *vmsd, void
> > *opaque,
> > + Error **errp)
> > +{
> > + int ret = 0;
> > + if (vmsd->post_save) {
> > + ret = vmsd->post_save(opaque);
>
> + error_setg(errp, "post-save for %s failed, ret: %d",
> > + vmsd->name, ret);
> >
>
> Only set errp if ret != 0
>
Yes, missed this one.
>
> > + }
> > + return ret;
> > +}
> > +
> > +static int vmstate_save_dispatch(QEMUFile *f,
> > + const VMStateDescription *vmsd,
> > + void *opaque, JSONWriter *vmdesc,
> > + int version_id, Error **errp)
> > +{
> > + ERRP_GUARD();
> > + int ret = 0;
> > + int ps_ret = 0;
> > + Error *local_err = NULL;
> > + const VMStateField *field = vmsd->fields;
> >
> > if (vmdesc) {
> > json_writer_str(vmdesc, "vmsd_name", vmsd->name);
> > @@ -532,9 +553,7 @@ 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);
> > - if (vmsd->post_save) {
> > - vmsd->post_save(opaque);
> > - }
> > + ps_ret = post_save_dispatch(vmsd, opaque, &local_err);
> >
>
> why keep ps_ret ?
It is not required.
>
> What do you do of local_err ?
Should be freed. We do nothing with this. In the next patch it is propagated, and
previous error is dismissed. But that will no longer be the case if the behaviour of
post_save is is kept the same (from the discussion on changeing post_save() to cleanup_save())
>
>
> > return ret;
> > }
> >
> > @@ -557,16 +576,39 @@ int vmstate_save_state_v(QEMUFile *f, const
> > VMStateDescription *vmsd,
> > if (vmdesc) {
> > json_writer_end_array(vmdesc);
> > }
> > + return ret;
> > +}
> >
> > - ret = vmstate_subsection_save(f, vmsd, opaque, vmdesc, errp);
> >
> > - if (vmsd->post_save) {
> > - int ps_ret = vmsd->post_save(opaque);
> > - if (!ret && ps_ret) {
> > - ret = ps_ret;
> > - error_setg(errp, "post-save failed: %s", vmsd->name);
> > - }
> > +int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
> > + void *opaque, JSONWriter *vmdesc, int version_id,
> > + Error **errp)
> > +{
> > + ERRP_GUARD();
> > + int ret = 0;
> > + Error *local_err = NULL;
> > + int ps_ret = 0;
> > +
> > + trace_vmstate_save_state_top(vmsd->name);
> > +
> > + ret = pre_save_dispatch(vmsd, opaque, errp);
> > + if (ret) {
> > + return ret;
> > + }
> > +
> > + ret = vmstate_save_dispatch(f, vmsd, opaque, vmdesc,
> > + version_id, errp);
> > + if (ret) {
> > + return ret
>
>
> post_save_dispatch() should be called on failure.
>
> >
> > }
> > +
> > + ret = vmstate_subsection_save(f, vmsd, opaque, vmdesc, errp);
> >
>
> Imho this should be moved back to the vmstate_save_dispatch()
>
> + ps_ret = post_save_dispatch(vmsd, opaque, &local_err);
> > + if (!ret && ps_ret) {
> > + ret = ps_ret;
> > + error_setg(errp, "post-save failed: %s", vmsd->name);
> >
>
> And then you can have a single place to call post_save_dispatch() - remove
> it from vmstate_subsection_save -> you mean vmstate_save_state_v()?
>
> It should probably call error_propagate() instead.
Yes, we can move vmstate_subsection_save() and the subsequent call
to post_save() here.
Shall do, if we are planning to keep this behavior for post_save.
Otherwise we can just keep things as is, and just change post_save() return
value and naming.
>
>
>
> > + }
> > +
> > return ret;
> > }
> >
> >
> > --
> > 2.50.1
> >
> >
Regards,
Arun