On 29.02.24 16:21, Markus Armbruster wrote:
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.
In my understanding, unset errp on failure path is wrong enough.
For example, simple pattern
Error *local_err = NULL;
int ret = foo(&local_err);
if (ret < 0) {
error_report_err(local_err);
return;
}
will just crash.
When I write code, I expect that "errp is set" === "ret < 0", for all functions
returning negative integer on failure.
Also, we have enough code that relying on errp for failure checking:
$ git grep 'if (local_err)' | wc -l
373
Of course, most of this should be checking failure of functions that return
void, but who knows.
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.