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 :|