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

[Markus Armbruster <armbru@redhat.com>]

Almost missed this, sorry.

Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp> writes:

> On 2025/07/21 22:29, Daniel P. Berrangé wrote:
>> 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/

I disagree.

We've discussed this a few times.  Here's a recent instance:
https://lore.kernel.org/qemu-devel/87jz5tbbqx.fsf@pond.sub.org/

>>> 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.
>
> Markus, I'd also like to hear your opinion since you are the maintainer of the error reporting facility.

I'm with Daniel.

New code should stick to the rules.

Changing existing code from "sticks to the rules" to not requires pretty
compelling justification.

The other direction is more welcome, but whether the juice is worth the
squeeze still needs to be decided case by case.