Re: [PATCH v6 23/24] migration: Add error-parameterized function variants in VMSD struct

["Daniel P. Berrangé" <berrange@redhat.com>]

On Mon, Jul 21, 2025 at 10:14:30PM +0900, Akihiko Odaki wrote:
> On 2025/07/21 20:29, 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 <armenon@redhat.com>
> > ---
> >   include/migration/vmstate.h | 11 +++++++++++
> >   migration/vmstate.c         | 47 +++++++++++++++++++++++++++++++++++++++------
> >   2 files changed, 52 insertions(+), 6 deletions(-)
> > 
> > diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> > index 056781b1c21e737583f081594d9f88b32adfd674..53fa72c1bbde399be02c88fc8745fdbb79bfd7c8 100644
> > --- a/include/migration/vmstate.h
> > +++ b/include/migration/vmstate.h
> > @@ -200,15 +200,26 @@ 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.
> >        */
> > +
> >       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);
> 
> I think the new functions should have void as return value instead.
> 
> As I discussed before, I think having an integer return value is a source of
> confusion:
> https://lore.kernel.org/qemu-devel/0447e269-c242-4cd7-b68e-d0c7211784a7@rsg.ci.i.u-tokyo.ac.jp/
> 
> In the previous discussion, I suggested using bool, but void fits better in
> this particular case.
> 
> include/qapi/error.h says:
> > Whenever practical, also return a value that indicates success /
> > failure.  This can make the error checking more concise, and can avoid
> > useless error object creation and destruction.  Note that we still
> > have many functions returning void.
> 
> There will be more implementations of these function pointers than their
> callers, so it makes more sense to let return void and make implementations
> more concise while making the callers less so. There is also DeviceRealize,
> an example of function pointer type that takes errp but returns void.

No, please do NOT make these functions void. As that text you quote
says, we want functions to return a value indicating success/failure.
'void' return is a historical practice we don't want to continue
in QEMU.

Given that the existing methods all return 'int', we should remain
consistent with the new functions and return 'int', with -1 for
failure, 0 for success, and not use bool.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|