Re: [PATCH v2 06/21] migration: Add Error** argument to .save_setup() handler

[Markus Armbruster <armbru@redhat.com>]

Thomas Huth <thuth@redhat.com> writes:

> On 29/02/2024 08.20, Vladimir Sementsov-Ogievskiy wrote:
>> On 29.02.24 09:32, Markus Armbruster wrote:

[...]

>>> Anti-pattern: fail without setting an error.  There might be more
>>> elsewhere in the series.
>>>
>>> qapi/error.h's big comment:
>>>
>>>   * - On success, the function should not touch *errp.  On failure, it
>>>   *   should set a new error, e.g. with error_setg(errp, ...), or
>>>   *   propagate an existing one, e.g. with error_propagate(errp, ...).
>>>   *
>>>   * - 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.  We recommend
>>>   *   • bool-valued functions return true on success / false on failure,
>>>   *   • pointer-valued functions return non-null / null pointer, and
>>>   *   • integer-valued functions return non-negative / negative.
>>>
>>>         }
>>>         qemu_put_be64(f, STATTR_FLAG_EOS);
>>>         return 0;
>>>     }
>>>
>>> When adding Error **errp to a function, you must also add code to set an
>>> error on failure to every failure path.  Adding it in a later patch in
>>> the same series can be okay,
>>
>> Personally, I'd prefer not doing so. Creating wrong commits and fixing them in same series - better to merge all fixes into bad commit:)
>
> I agree - that might create issues with bisecting later. Please fix it in this patch here already!

Depends on the wrongness, really.

We don't want broken intermediate states, no argument.

But intermediate states that are merely unclean can be acceptable.

For instance, my commit a30ecde6e79 (net: Permit incremental conversion
of init functions to Error) added such Error ** parameters to a somewhat
tangled nest of functions, along with FIXME comments where errors
weren't set.  The next few commits fixed most, but not all of them.
Later commits fixed some more.  The one in tap-win32.c is still there
today.

This was acceptable, because it improved things from "bad error
reporting" to "okay error reporting in most cases, unclean and bad error
reporting in a few cases marked FIXME", with "a few" over time going
down to the one I can't test, and nobody else seems to care about.