Hi On Tue, Aug 5, 2025 at 10:31 PM Arun Menon <[email protected]> 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 <[email protected]> > --- > 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 > + } > + 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 ? What do you do of local_err ? > 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. It should probably call error_propagate() instead. > + } > + > return ret; > } > > > -- > 2.50.1 > >
