Re: [Qemu-devel] [PATCH v6 03/14] migration: Add post_save function to VMStateDescription

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Richard Henderson (richard.henderson@linaro.org) wrote:
> On 10/10/18 1:37 PM, Aaron Lindsay wrote:
> > In some cases it may be helpful to modify state before saving it for
> > migration, and then modify the state back after it has been saved. The
> > existing pre_save function provides half of this functionality. This
> > patch adds a post_save function to provide the second half.
> > 
> > Signed-off-by: Aaron Lindsay <aclindsa@gmail.com>
> > ---
> >  docs/devel/migration.rst    |  9 +++++++--
> >  include/migration/vmstate.h |  1 +
> >  migration/vmstate.c         | 10 +++++++++-
> >  3 files changed, 17 insertions(+), 3 deletions(-)
> 
> Hmm, maybe.  I believe the common practice is for pre_save to copy state into a
> separate member on the side, so that conversion back isn't necessary.
> 
> Ccing in the migration maintainers for a second opinion.

It is common to copy stuff into a separate member; however we do
occasionally think that post_save would be a useful addition; so I think
we should take it (if nothing else it actually makes stuff symmetric!).

Please make it return 'int' in the same way that pre_save/pre_load
does, so that it can fail and stop the migration.

Dave

> 
> 
> r~
> 
> > 
> > diff --git a/docs/devel/migration.rst b/docs/devel/migration.rst
> > index 687570754d..2a2533c9b3 100644
> > --- a/docs/devel/migration.rst
> > +++ b/docs/devel/migration.rst
> > @@ -419,8 +419,13 @@ The functions to do that are inside a vmstate definition, and are called:
> >  
> >    This function is called before we save the state of one device.
> >  
> > -Example: You can look at hpet.c, that uses the three function to
> > -massage the state that is transferred.
> > +- ``void (*post_save)(void *opaque);``
> > +
> > +  This function is called after we save the state of one device
> > +  (even upon failure, unless the call to pre_save returned and error).
> > +
> > +Example: You can look at hpet.c, that uses the first three functions
> > +to massage the state that is transferred.
> >  
> >  The ``VMSTATE_WITH_TMP`` macro may be useful when the migration
> >  data doesn't match the stored device data well; it allows an
> > diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> > index 2b501d0466..f6053b94e4 100644
> > --- a/include/migration/vmstate.h
> > +++ b/include/migration/vmstate.h
> > @@ -185,6 +185,7 @@ struct VMStateDescription {
> >      int (*pre_load)(void *opaque);
> >      int (*post_load)(void *opaque, int version_id);
> >      int (*pre_save)(void *opaque);
> > +    void (*post_save)(void *opaque);
> >      bool (*needed)(void *opaque);
> >      VMStateField *fields;
> >      const VMStateDescription **subsections;
> > diff --git a/migration/vmstate.c b/migration/vmstate.c
> > index 0bc240a317..9afc9298f3 100644
> > --- a/migration/vmstate.c
> > +++ b/migration/vmstate.c
> > @@ -387,6 +387,9 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
> >                  if (ret) {
> >                      error_report("Save of field %s/%s failed",
> >                                   vmsd->name, field->name);
> > +                    if (vmsd->post_save) {
> > +                        vmsd->post_save(opaque);
> > +                    }
> >                      return ret;
> >                  }
> >  
> > @@ -412,7 +415,12 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
> >          json_end_array(vmdesc);
> >      }
> >  
> > -    return vmstate_subsection_save(f, vmsd, opaque, vmdesc);
> > +    ret = vmstate_subsection_save(f, vmsd, opaque, vmdesc);
> > +
> > +    if (vmsd->post_save) {
> > +        vmsd->post_save(opaque);
> > +    }
> > +    return ret;
> >  }
> >  
> >  static const VMStateDescription *
> > 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK