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(-)
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 *
--
2.19.1
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.
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 *
>
* 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
On Oct 16 09:21, Dr. David Alan Gilbert wrote: > * 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. This patch calls post_save *even if the save operation fails*. My reasoning was that I didn't want a failed migration to leave a still-running original QEMU instance in an invalid state. Was this misguided? If it was not, which error do you prefer to be returned from vmstate_save_state_v() in the case that both the save operation itself and the post_save call returned errors? -Aaron
* Aaron Lindsay (aclindsa@gmail.com) wrote: > On Oct 16 09:21, Dr. David Alan Gilbert wrote: > > * 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. > > This patch calls post_save *even if the save operation fails*. My > reasoning was that I didn't want a failed migration to leave a > still-running original QEMU instance in an invalid state. Was this > misguided? That's fine - my only issue is that I want post_save to be able to fail itself even if pre_save failed. > If it was not, which error do you prefer to be returned from > vmstate_save_state_v() in the case that both the save operation itself > and the post_save call returned errors? The return value from the save operation. I did wonder about suggesting that you pass the return value from the save operation as a parameter to post_save. Dave > -Aaron -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Oct 16 15:06, Dr. David Alan Gilbert wrote: > I did wonder about suggesting that you pass the return value from the > save operation as a parameter to post_save. I feel like having that information in post_save may be useful, though I'm having trouble coming up with any concrete uses for it. But, let me know if you decide that you prefer it and I can add that for the next revision. -Aaron
* Aaron Lindsay (aclindsa@gmail.com) wrote: > On Oct 16 15:06, Dr. David Alan Gilbert wrote: > > I did wonder about suggesting that you pass the return value from the > > save operation as a parameter to post_save. > > I feel like having that information in post_save may be useful, though > I'm having trouble coming up with any concrete uses for it. But, let me > know if you decide that you prefer it and I can add that for the next > revision. I'm happy either way with the input value; it sounded useful to me but not enough to ask you for, but if you agree then throw it in. Dave > -Aaron -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: > * Aaron Lindsay (aclindsa@gmail.com) wrote: >> On Oct 16 09:21, Dr. David Alan Gilbert wrote: >> > * 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. >> >> This patch calls post_save *even if the save operation fails*. My >> reasoning was that I didn't want a failed migration to leave a >> still-running original QEMU instance in an invalid state. Was this >> misguided? > > That's fine - my only issue is that I want post_save to be able to fail > itself even if pre_save failed. > >> If it was not, which error do you prefer to be returned from >> vmstate_save_state_v() in the case that both the save operation itself >> and the post_save call returned errors? > > The return value from the save operation. > I did wonder about suggesting that you pass the return value from the > save operation as a parameter to post_save. The one from save. In general, this one shouldn't be called, and if it gets an error, we are really in big trouble, no? By big treauble I mean that we can basically only stop the guest? Later, Juan. > > Dave > >> -Aaron > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
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. Hi Originally we have that function. We removed it because we had not remaining uses for it on tree. I am not againt getting it if you need it. Once told that, I think you can add a return value as David saide. And once there, it ia good thing that you document it if it is called "before" or "after" the subsections_save. There are arguments for doing it either way, just document it. Thanks, Juan.
© 2016 - 2025 Red Hat, Inc.