Hi Akihiko,
Thanks for the review.
On Wed, Aug 06, 2025 at 02:45:01PM +0900, Akihiko Odaki wrote:
> On 2025/08/06 3:25, Arun Menon wrote:
> > - We need to have good error reporting in the callbacks in
> > VMStateDescription struct. Specifically pre_save, post_save,
> > pre_load and post_load callbacks.
> > - It is not possible to change these functions everywhere in one
> > patch, therefore, we introduce a duplicate set of callbacks
> > with Error object passed to them.
> > - So, in this commit, we implement 'errp' variants of these callbacks,
> > introducing an explicit Error object parameter.
> > - This is a functional step towards transitioning the entire codebase
> > to the new error-parameterized functions.
> > - Deliberately called in mutual exclusion from their counterparts,
> > to prevent conflicts during the transition.
> > - New impls should preferentally use 'errp' variants of
> > these methods, and existing impls incrementally converted.
> > The variants without 'errp' are intended to be removed
> > once all usage is converted.
> >
> > Signed-off-by: Arun Menon <arme...@redhat.com>
> > ---
> > docs/devel/migration/main.rst | 24 ++++++++++++++++++++++++
> > include/migration/vmstate.h | 15 +++++++++++++++
> > migration/vmstate.c | 34 ++++++++++++++++++++++++++++++----
> > 3 files changed, 69 insertions(+), 4 deletions(-)
> >
> > diff --git a/docs/devel/migration/main.rst b/docs/devel/migration/main.rst
> > index
> > 6493c1d2bca48a2fa34d92f6c0979c215c56b8d5..35bf5ae26c87f3c82964eb15618be373ab5a41fc
> > 100644
> > --- a/docs/devel/migration/main.rst
> > +++ b/docs/devel/migration/main.rst
> > @@ -444,6 +444,30 @@ The functions to do that are inside a vmstate
> > definition, and are called:
> > This function is called after we save the state of one device
> > (even upon failure, unless the call to pre_save returned an error).
> > +Following are the errp variants of these functions.
> > +
> > +- ``int (*pre_load_errp)(void *opaque, Error **errp);``
> > +
> > + This function is called before we load the state of one device.
> > +
> > +- ``int (*post_load_errp)(void *opaque, int version_id, Error **errp);``
> > +
> > + This function is called after we load the state of one device.
> > +
> > +- ``int (*pre_save_errp)(void *opaque, Error **errp);``
> > +
> > + This function is called before we save the state of one device.
> > +
> > +- ``int (*post_save_errp)(void *opaque, Error **errp);``
> > +
> > + This function is called after we save the state of one device
> > + (even upon failure, unless the call to pre_save returned an error).
>
> Reviewing "[PATCH v9 24/27] migration: Propagate last encountered error in
> vmstate_save_state_v() function", I wondered why it has never been a problem
> post_load(), and this says only post_save_errp() is called upon failure.
>
> Now I suspect it may be better to clarify their differences and avoid
> introducing post_save_errp() instead.
>
> My guess is that post_save_errp() is being introduced for consistency with
> post_load(), but they cannot have "consistency" if post_load() and
> post_save() are not corresponding functions of save/load but are independent
> two functions. Perhaps the true problem here is that post_load() and
> post_save() look similar; if so, that can be solved by:
> - Renaming post_save() to e.g., cleanup_save()
> - Changing it to return void
>
That is insightful. I agree. It was introduced because of the nomenclature.
>From what I have seen, the functionality indeed differs, in the sense that,
currently post_load does some sanity checks after loading the device state,
and therefore it can fail and return an error; whereas post_save is not known
to fail until now.
It is therefore possible to implement your suggestion : renaming it to
cleanup_save and returning void. This will avoid us in having the extra
patch 24/27.
While I have deviated a bit from TPM backend to make these changes in the
migration module, I believe they are important and hope the maintainers
will agree.
> > +
> > +New impls should preferentally use 'errp' variants of these
> > +methods and existing impls incrementally converted.
> > +The variants without 'errp' are intended to be removed
> > +once all usage is converted.
> > +
> > Example: You can look at hpet.c, that uses the first three functions
> > to massage the state that is transferred.
> > diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> > index
> > 5fe9bbf39058d0cf97c1adab54cc516dbe8dc32a..51baf6c7f9d061ee33949d7e798f2184e50b4cbf
> > 100644
> > --- a/include/migration/vmstate.h
> > +++ b/include/migration/vmstate.h
> > @@ -200,15 +200,30 @@ struct VMStateDescription {
> > * exclusive. For this reason, also early_setup VMSDs are migrated in
> > a
> > * QEMU_VM_SECTION_FULL section, while save_setup() data is migrated
> > in
> > * a QEMU_VM_SECTION_START section.
> > + *
> > + * There are duplicate impls of the post/pre save/load hooks.
> > + * New impls should preferentally use 'errp' variants of these
> > + * methods and existing impls incrementally converted.
> > + * The variants without 'errp' are intended to be removed
> > + * once all usage is converted.
> > + *
> > + * For the errp variants,
> > + * Returns: 0 on success,
> > + * <0 on error where -value is an error number from errno.h
> > */
> > +
> > bool early_setup;
> > int version_id;
> > int minimum_version_id;
> > MigrationPriority priority;
> > int (*pre_load)(void *opaque);
> > + int (*pre_load_errp)(void *opaque, Error **errp);
> > int (*post_load)(void *opaque, int version_id);
> > + int (*post_load_errp)(void *opaque, int version_id, Error **errp);
> > int (*pre_save)(void *opaque);
> > + int (*pre_save_errp)(void *opaque, Error **errp);
> > int (*post_save)(void *opaque);
> > + int (*post_save_errp)(void *opaque, Error **errp);
> > bool (*needed)(void *opaque);
> > bool (*dev_unplug_pending)(void *opaque);
> > diff --git a/migration/vmstate.c b/migration/vmstate.c
> > index
> > 569e66ea983f833e6a0d651d2a751f34a64e8f5c..0acaa855cfec8ddac63e33d4562e39c345856213
> > 100644
> > --- a/migration/vmstate.c
> > +++ b/migration/vmstate.c
> > @@ -152,7 +152,16 @@ int vmstate_load_state(QEMUFile *f, const
> > VMStateDescription *vmsd,
> > trace_vmstate_load_state_end(vmsd->name, "too old", -EINVAL);
> > return -EINVAL;
> > }
> > - if (vmsd->pre_load) {
> > + if (vmsd->pre_load_errp) {
> > + ret = vmsd->pre_load_errp(opaque, errp);
> > + if (ret) {
> > + error_prepend(errp, "VM pre load failed for: '%s', "
> > + "version_id: %d, minimum version_id: %d, "
> > + "ret: %d: ", vmsd->name, vmsd->version_id,
> > + vmsd->minimum_version_id, ret);
> > + return ret;
> > + }
> > + } else if (vmsd->pre_load) {
> > ret = vmsd->pre_load(opaque);
> > if (ret) {
> > error_setg(errp, "VM pre load failed for: '%s', "
> > @@ -242,7 +251,14 @@ int vmstate_load_state(QEMUFile *f, const
> > VMStateDescription *vmsd,
> > qemu_file_set_error(f, ret);
> > return ret;
> > }
> > - if (vmsd->post_load) {
> > + if (vmsd->post_load_errp) {
> > + ret = vmsd->post_load_errp(opaque, version_id, errp);
> > + if (ret < 0) {
> > + error_prepend(errp, "VM Post load failed for: %s, version_id:
> > %d, "
> > + "minimum_version: %d, ret: %d: ", vmsd->name,
> > + vmsd->version_id, vmsd->minimum_version_id, ret);
> > + }
> > + } else if (vmsd->post_load) {
> > ret = vmsd->post_load(opaque, version_id);
> > if (ret < 0) {
> > error_setg(errp,
> > @@ -411,8 +427,16 @@ int vmstate_save_state(QEMUFile *f, const
> > VMStateDescription *vmsd,
> > static int pre_save_dispatch(const VMStateDescription *vmsd, void *opaque,
> > Error **errp)
> > {
> > + ERRP_GUARD();
> > int ret = 0;
> > - if (vmsd->pre_save) {
> > + if (vmsd->pre_save_errp) {
> > + ret = vmsd->pre_save_errp(opaque, errp);
> > + trace_vmstate_save_state_pre_save_res(vmsd->name, ret);
> > + if (ret) {
> > + error_prepend(errp, "pre-save for %s failed, ret: %d: ",
> > + vmsd->name, ret);
> > + }
> > + } else if (vmsd->pre_save) {
> > ret = vmsd->pre_save(opaque);
> > trace_vmstate_save_state_pre_save_res(vmsd->name, ret);
> > if (ret) {
> > @@ -427,7 +451,9 @@ static int post_save_dispatch(const VMStateDescription
> > *vmsd, void *opaque,
> > Error **errp)
> > {
> > int ret = 0;
> > - if (vmsd->post_save) {
> > + if (vmsd->post_save_errp) {
> > + ret = vmsd->post_save_errp(opaque, errp);
> > + } else if (vmsd->post_save) {
> > ret = vmsd->post_save(opaque);
> > error_setg(errp, "post-save for %s failed, ret: %d",
> > vmsd->name, ret);
> >
>
Regards,
Arun
