Paolo Bonzini <pbonzini@redhat.com> writes:
On 01/10/21 19:15, Daniel P. Berrangé wrote:
On Fri, Oct 01, 2021 at 07:08:51PM +0200, Paolo Bonzini wrote:
On 29/09/21 04:58, Yanan Wang wrote:
@@ -933,8 +935,7 @@ static void machine_set_smp(Object *obj, Visitor *v, const
char *name,
return;
}
- smp_parse(ms, config, errp);
- if (*errp) {
+ if (!smp_parse(ms, config, errp)) {
qapi_free_SMPConfiguration(config);
}
}
This is actually a leak, so I'm replacing this patch with
This patch isn't adding a leak, as there's no change in
control flow / exit paths. AFAICT, the leak was introduced
in patch 15 instead, so the code below shoudl be squashed
into that, and this patch left as-is.
Yes, even better make it a separate patch and fix the conflict in patch
15. But I'm still not sure of the wisdom of this patch.
At this point smp_parse has exactly one caller and it doesn't care about
the return value. The "return a boolean" rule adds some complexity (and
a possibility for things to be wrong/inconsistent) to the function for
the benefit of the callers.
Yes, but returning something is only a minor burden. It also makes
success vs. failure obvious at a glance.
I'm not worrying about inconsistency anymore. In a way, void functions
are an exception. Many non-void functions return a distinct error value
on failure, like NULL. The only kind of inconsistency I can remember
seeing in these functions is forgetting to set an error. Can be screwed
up in a void function just as easily.
If there is only one caller, as is the case
here or for virtual functions, the benefit can well be zero (this case)
or negative (virtual functions).
Two small benefits here:
1. No need for ERRP_GUARD().
2. Conforms to the rules. Rules are not laws, but let's stick to them
when it's as easy as it is here.
For what it's worth, GLib always advised users of GError to return a
value. We chose to deviate for our Error, then spent nine years
learning how that leads to cumbersome code, leading us to:
commit e3fe3988d7851cac30abffae06d2f555ff7bee62
Author: Markus Armbruster <armbru@redhat.com>
Date: Tue Jul 7 18:05:31 2020 +0200
error: Document Error API usage rules
This merely codifies existing practice, with one exception: the rule
advising against returning void, where existing practice is mixed.
When the Error API was created, we adopted the (unwritten) rule to
return void when the function returns no useful value on success,
unlike GError, which recommends to return true on success and false on
error then.
When a function returns a distinct error value, say false, a checked
call that passes the error up looks like
if (!frobnicate(..., errp)) {
handle the error...
}
When it returns void, we need
Error *err = NULL;
frobnicate(..., &err);
if (err) {
handle the error...
error_propagate(errp, err);
}
Not only is this more verbose, it also creates an Error object even
when @errp is null, &error_abort or &error_fatal.
People got tired of the additional boilerplate, and started to ignore
the unwritten rule. The result is confusion among developers about
the preferred usage.
Make the rule advising against returning void official by putting it
in writing. This will hopefully reduce confusion.
Update the examples accordingly.
The remainder of this series will update a substantial amount of code
to honor the rule.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Greg Kurz <groug@kaod.org>
Message-Id: <20200707160613.848843-4-armbru@redhat.com>
[Tweak prose as per advice from Eric]