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

[Paolo Bonzini <pbonzini@redhat.com>]

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.  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).

Paolo

---------------- 8< ----------------
 From e7f944bb94a375e8ee7469ffa535ea6e11ce59e1 Mon Sep 17 00:00:00 2001
From: Paolo Bonzini <pbonzini@redhat.com>
Date: Fri, 1 Oct 2021 19:04:03 +0200
Subject: [PATCH] machine: Use g_autoptr in machine_set_smp

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
  hw/core/machine.c | 7 ++-----
  1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 54f04a5ac6..d49ebc24e2 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -897,7 +897,7 @@ static void machine_set_smp(Object *obj, Visitor *v, const char *name,
  {
      MachineClass *mc = MACHINE_GET_CLASS(obj);
      MachineState *ms = MACHINE(obj);
-    SMPConfiguration *config;
+    g_autoptr(SMPConfiguration) config = NULL;
      ERRP_GUARD();
  
      if (!visit_type_SMPConfiguration(v, name, &config, errp)) {
@@ -920,7 +920,7 @@ static void machine_set_smp(Object *obj, Visitor *v, const char *name,
  
      smp_parse(ms, config, errp);
      if (*errp) {
-        goto out_free;
+        return;
      }
  
      /* sanity-check smp_cpus and max_cpus against mc */
@@ -935,9 +935,6 @@ static void machine_set_smp(Object *obj, Visitor *v, const char *name,
                     ms->smp.max_cpus,
                     mc->name, mc->max_cpus);
      }
-
-out_free:
-    qapi_free_SMPConfiguration(config);
  }
  
  static void machine_class_init(ObjectClass *oc, void *data)
-- 
2.31.1