| Message ID | 20181010203735.27918-4-aclindsa@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | More fully implement ARM PMUv3 | expand |
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
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.
"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
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 *
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(-)