[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: Paolo Bonzini
Subject: Re: [PATCH v12 16/16] machine: Make smp_parse return a boolean
Date: Sat, 2 Oct 2021 08:40:46 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.1.0

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


---------------- 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;
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,
                    mc->name, mc->max_cpus);
-    qapi_free_SMPConfiguration(config);
static void machine_class_init(ObjectClass *oc, void *data)

reply via email to

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