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 <arme...@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-d0c721178...@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 :|
