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

[Markus Armbruster <armbru@redhat.com>]

Please excuse the delayed response, I was on vacation.

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

> On 2025/08/09 23:30, Markus Armbruster wrote:
>> Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp> writes:

[...]

>>> There were three options on the table: bool, int, and void.
>>>
>>> The previous discussion you referred explains why void should be avoided, and include/qapi/error.h also says void should be avoided.
>>>
>>> There is pre_load() that does not use Error returns int, but now we are adding pre_load_errp() that uses Error.
>>>
>>> Then what pre_load_errp() should return: bool or int?
>>
>> I like bool when it's all we need.
>>
>> When we need to return a non-negative int on success, use int and return
>> -1 or a negative error code on failure.
>>
>> Another reason to pick int is local consistency: related functions
>> already use int.
>>
>> Changing working code from int to bool doesn't seem worth the bother.
>>
>> Questions?
>
> I at least see what "[PATCH v9 26/27] migration: Add error-parameterized function variants in VMSD struct" says is undesirable. It says:
>
>> For the errp variants,
>> Returns: 0 on success,
>>          <0 on error where -value is an error number from errno.h

Does any caller use errno codes to discriminate between failures?

> There are already non-errp variant implementations that return -1 (e.g., dbus_vmstate_post_load). Adding errp variants that require to return errno is degradation.

However, the non-errp variants are intended to be removed.  Any
inconsistency with them would hopefully be temporary.  We can accept
that when other considerations call for it.

> I'm still not sure what conclusion will be drawn. I can make two opposite conclusions:
>
> - Non-errp variants return int, so errp variants should also return int
>   for local consistency.
>
> - bool is all we need, but changing non-errp variants doesn't seem worth
>   the bother, so only errp variants should return bool.

Here's my recommendation.

If any caller needs to use errno codes, use int and return -errno on
failure.  This is inconsistent with the old callbacks, but callers'
needs trump that.

Else if we want consistency with the old callbacks even though we intend
to remove them, use int and return -1 on failure.

Else use bool and return false on failure.