[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v12 16/16] machine: Make smp_parse return a boolean

From: wangyanan (Y)
Subject: Re: [PATCH v12 16/16] machine: Make smp_parse return a boolean
Date: Thu, 7 Oct 2021 11:44:42 +0800
User-agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.4.0

On 2021/10/2 19:27, Markus Armbruster wrote:
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,
-    smp_parse(ms, config, errp);
-    if (*errp) {
+    if (!smp_parse(ms, config, errp)) {

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]


Thanks for the fix, Paolo!

I notice that with Paolo's fix applied first and then Patch15 removing
the sanity checks out, machine_set_smp() at last simply becomes:

static void machine_set_smp(Object *obj, Visitor *v, const char *name,
                            void *opaque, Error **errp)
    MachineState *ms = MACHINE(obj);
    g_autoptr(SMPConfiguration) config = NULL;

    if (!visit_type_SMPConfiguration(v, name, &config, errp)) {

    smp_parse(ms, config, errp);

It looks good currently, neither the returned boolean nor the errp needs to
be checked here now, and smp_parse is only called here. So in this case,
we may avoid the boolean until we need to use it and honor the rule. :)


reply via email to

[Prev in Thread] Current Thread [Next in Thread]