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

Re: [libvirt] [PATCH] remove useless code



"Daniel P. Berrange" <berrange redhat com> wrote:

> On Thu, Feb 05, 2009 at 01:55:39PM +0100, Jim Meyering wrote:
>> "Daniel P. Berrange" <berrange redhat com> wrote:
>> ...
>> > These two are not safe to remove. Look at the whole code block in
>> > question:
>> >
>> >     if (diskVal->list == NULL)
>> >         VIR_FREE(diskVal);
>> >     else if (virConfSetValue(conf, "disk", diskVal) < 0) {
>> >         diskVal = NULL;
>> >         goto no_memory;
>> >     }
>> >     diskVal = NULL;
>> >
>> > In the case where virConfSetValue returned >= 0, we need to
>> > still set diskVal = NULL.
>>
>> Oops.  Thanks.
>>
>> However, the original code is ugly enough that I have rewritten
>> it to make it clear that regardless of the virConfSetValue outcome,
>> we don't free that variable:
>>
>>     if (diskVal->list != NULL) {
>>         bool err = (virConfSetValue(conf, "disk", diskVal) < 0);
>>         diskVal = NULL;
>>         if (err)
>>             goto no_memory;
>>     }
>>     VIR_FREE(diskVal);
>
> I prefer if it had the return value check separate from the assignment,
> eg
>
>          int ret = virConfSetValue(conf, "disk", diskVal);
>          diskVal = NULL;
>          if (ret < 0)
>              goto no_memory;

Ok. so do I.


>From 6f1b5bb867d58d5606986ec1c8f34f6d95007663 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering redhat com>
Date: Wed, 4 Feb 2009 17:44:24 +0100
Subject: [PATCH] remove useless code

* src/bridge.c (brAddTap): Remove redundant errno=ENOMEM assignment
after failed strdup.
* src/cgroup.c (virCgroupFree): Remove redundant *group=NULL assignment
after VIR_FREE(*group).
* src/lxc_driver.c (lxcShutdown): Likewise.
* src/xen_unified.c (xenUnifiedClose): Likewise.
* src/xm_internal.c (xenXMDomainConfigFormat): Rewrite disk-
and net-config-setting code to be clearer.
---
 src/bridge.c      |    6 ++----
 src/cgroup.c      |    1 -
 src/lxc_driver.c  |    1 -
 src/xen_unified.c |    1 -
 src/xm_internal.c |   21 ++++++++++-----------
 5 files changed, 12 insertions(+), 18 deletions(-)

...
diff --git a/src/xm_internal.c b/src/xm_internal.c
index a03263c..3a96058 100644
--- a/src/xm_internal.c
+++ b/src/xm_internal.c
@@ -2233,14 +2233,13 @@ virConfPtr xenXMDomainConfigFormat(virConnectPtr conn,
                                         hvm, priv->xendConfigVersion) < 0)
             goto cleanup;
     }
-    if (diskVal->list == NULL)
-        VIR_FREE(diskVal);
-    else if (virConfSetValue(conf, "disk", diskVal) < 0) {
+    if (diskVal->list != NULL) {
+        int ret = virConfSetValue(conf, "disk", diskVal);
         diskVal = NULL;
-        goto no_memory;
+        if (ret < 0)
+            goto no_memory;
     }
-    diskVal = NULL;
-
+    VIR_FREE(diskVal);

     if (VIR_ALLOC(netVal) < 0)
         goto no_memory;
@@ -2253,13 +2252,13 @@ virConfPtr xenXMDomainConfigFormat(virConnectPtr conn,
                                        hvm) < 0)
             goto cleanup;
     }
-    if (netVal->list == NULL)
-        VIR_FREE(netVal);
-    else if (virConfSetValue(conf, "vif", netVal) < 0) {
+    if (netVal->list != NULL) {
+        int ret = virConfSetValue(conf, "vif", netVal);
         netVal = NULL;
-        goto no_memory;
+        if (ret < 0)
+            goto no_memory;
     }
-    netVal = NULL;
+    VIR_FREE(netVal);

     if (hvm) {
         if (def->nparallels) {
--
1.6.1.2.511.gc5d3f


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