Re: [PATCH v3 12/18] migration: VMStateInfo: introduce new handlers with errp

[Markus Armbruster <armbru@redhat.com>]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 4/28/26 10:17, Vladimir Sementsov-Ogievskiy wrote:
>> [add Markus]
>> On 27.04.26 22:11, Paolo Bonzini wrote:
>>>
>>>
>>> Il lun 27 apr 2026, 15:19 Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru <mailto:vsementsov@yandex-team.ru>> ha scritto:
>>>>
>>>> On 26.04.26 11:33, Paolo Bonzini wrote:
>>>>> On 3/4/26 21:22, Vladimir Sementsov-Ogievskiy wrote:
>>>>>> +    bool coroutine_mixed_fn (*load)(QEMUFile *f, void *pv, size_t size,
>>>>>> +                                    const VMStateField *field,
>>>>>> +                                    Error **errp);
>>>>>> +    bool coroutine_mixed_fn (*save)(QEMUFile *f, void *pv, size_t size,
>>>>>> +                                    const VMStateField *field,
>>>>>> +                                    JSONWriter *vmdesc,
>>>>>> +                                    Error **errp);
>>>>> Use void functions for callbacks, with a wrapper adding the bool type. While bool/errp is preferred in general, for callbacks it provides N opportunities to get it wrong instead of just one.
>>>>>
>>>>
>>>> Hmm, for now, the series is already merged.
>>>>
>>>> Next, I'm unsure, how callbacks are different from other functions around errors?
>>>
>>> As I wrote above, confusing the *error == NULL and bool return value is avoided without much extra effort for callbacks (or other kinds of virtual functions) l, because there is a single caller. So the balance between "avoiding the possibility of getting the return value wrong" and "convenience of having a return value" tilts more towards the former.

Bug patterns:

1. Set an error, immediately return success.  I can't remember seeing
this.

2. Set an error, forget to return, somehow survive all the way to the
normal success return.  I've seen and fixed this.  Returning void does
not help at all here.

3. Set an error, goto error exit without flipping the return value
variable from success to failure.  I've seen and fixed this, too.  Best
to initialize the return value variable to failure, and flip it to
success only when success is ensured.  Of course, that flip can be
forgotten as well if it's needed in multiple places.  That's rare.
Anyway, returning eliminates this bug pattern.

The last one does some tilting.

Tilting the other way: keeping the rules simple and consistent with
GError's.

>> Understand. Hm.
>>
>> An alternative for this single-caller case, is just add an assertion into this caller:
>>
>> bool wrapper(..., errp) {
>>     ERRP_GUARD();
>>     bool ok = x->foo(..., errp);
>>     assert(ok == !(*errp));
>>     return ok;
>> }
>
> I don't think this makes sense, the assertion moves a compile-time guarantee to a run-time guarantee which is strictly worse.
>
> There are many more examples of void+errp callbacks than bool+errp, the most important one being .realize for devices.  A quick grep, which is certainly not fully accurate, shows 63 vs. 27:
>
> $ git grep -w 'void.*(\*.*errp' | wc -l
> 63
> $ git grep -w 'bool.*(\*.*errp' | wc -l
> 27

Yes.  At some point, I briefly considered updating them to return bool
for rules conformance, but decided it wasn't worth it.

> In many cases there are just two or three implementation so I wouldn't go to great lengths to change it even if I disagree.  But for vmstate (e.g. pre_load_errp and friends) this is not going to be the case in the long run, and it's more similar to the device case.

We should always be willing to accept reasoned exceptions to rules.
However...

>>>> I'm not sure that making wrappers, which will have to use ERRP_GUARD and check
>>>> *errp for the error is a good practice. If it is, it would be good to start by
>>>> modifying the documentation in include/qapi/error.h to mention the exclusion
>>>> for callbacks.

... explaining them in documentation is desirable.