11:44 a.m.
Barely worth posting for review, but who knows...
From a7cae36a3d966e80d82859d63c60a303b63d6720 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering(a)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/bridge.c | 6 ++----
src/cgroup.c | 1 -
2 files changed, 2 insertions(+), 5 deletions(-)
diff --git a/src/bridge.c b/src/bridge.c
index 9c4ca74..990a567 100644
--- a/src/bridge.c
+++ b/src/bridge.c
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2007 Red Hat, Inc.
+ * Copyright (C) 2007, 2009 Red Hat, Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -539,10 +539,8 @@ brAddTap(brControl *ctl,
if ((errno = brSetInterfaceUp(ctl, try.ifr_name, 1)))
goto error;
VIR_FREE(*ifname);
- if (!(*ifname = strdup(try.ifr_name))) {
- errno = ENOMEM;
+ if (!(*ifname = strdup(try.ifr_name)))
goto error;
- }
*tapfd = fd;
return 0;
}
diff --git a/src/cgroup.c b/src/cgroup.c
index 1b4c27f..8bd6e87 100644
--- a/src/cgroup.c
+++ b/src/cgroup.c
@@ -51,7 +51,6 @@ void virCgroupFree(virCgroupPtr *group)
if (*group != NULL) {
VIR_FREE((*group)->path);
VIR_FREE(*group);
- *group = NULL;
}
}
--
1.6.1.2.467.g081e7
3:24 p.m.
Jim Meyering <jim(a)meyering.net> wrote:
>From a7cae36a3d966e80d82859d63c60a303b63d6720 Mon Sep 17 00:00:00
2001
From: Jim Meyering <meyering(a)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).
I found a few more.
Each of the last two are ok because the other
branch of the if stmt (out of context)
sets the same variable to NULL via VIR_FREE.
From afdb7b085d276c76d24468492045a7fc006022aa Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering(a)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): Likewise, twice.
---
src/bridge.c | 6 ++----
src/cgroup.c | 1 -
src/lxc_driver.c | 1 -
src/xen_unified.c | 1 -
src/xm_internal.c | 3 ---
5 files changed, 2 insertions(+), 10 deletions(-)
diff --git a/src/bridge.c b/src/bridge.c
index 9c4ca74..990a567 100644
--- a/src/bridge.c
+++ b/src/bridge.c
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2007 Red Hat, Inc.
+ * Copyright (C) 2007, 2009 Red Hat, Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -539,10 +539,8 @@ brAddTap(brControl *ctl,
if ((errno = brSetInterfaceUp(ctl, try.ifr_name, 1)))
goto error;
VIR_FREE(*ifname);
- if (!(*ifname = strdup(try.ifr_name))) {
- errno = ENOMEM;
+ if (!(*ifname = strdup(try.ifr_name)))
goto error;
- }
*tapfd = fd;
return 0;
}
diff --git a/src/cgroup.c b/src/cgroup.c
index 1b4c27f..8bd6e87 100644
--- a/src/cgroup.c
+++ b/src/cgroup.c
@@ -51,7 +51,6 @@ void virCgroupFree(virCgroupPtr *group)
if (*group != NULL) {
VIR_FREE((*group)->path);
VIR_FREE(*group);
- *group = NULL;
}
}
diff --git a/src/lxc_driver.c b/src/lxc_driver.c
index 8786505..aa417a9 100644
--- a/src/lxc_driver.c
+++ b/src/lxc_driver.c
@@ -1238,7 +1238,6 @@ static int lxcShutdown(void)
lxcDriverUnlock(lxc_driver);
virMutexDestroy(&lxc_driver->lock);
VIR_FREE(lxc_driver);
- lxc_driver = NULL;
return 0;
}
diff --git a/src/xen_unified.c b/src/xen_unified.c
index eefdb6c..8e64f51 100644
--- a/src/xen_unified.c
+++ b/src/xen_unified.c
@@ -399,7 +399,6 @@ xenUnifiedClose (virConnectPtr conn)
virMutexDestroy(&priv->lock);
VIR_FREE(conn->privateData);
- conn->privateData = NULL;
return 0;
}
diff --git a/src/xm_internal.c b/src/xm_internal.c
index a03263c..c1e9e99 100644
--- a/src/xm_internal.c
+++ b/src/xm_internal.c
@@ -2239,8 +2239,6 @@ virConfPtr xenXMDomainConfigFormat(virConnectPtr conn,
diskVal = NULL;
goto no_memory;
}
- diskVal = NULL;
-
if (VIR_ALLOC(netVal) < 0)
goto no_memory;
@@ -2259,7 +2257,6 @@ virConfPtr xenXMDomainConfigFormat(virConnectPtr conn,
netVal = NULL;
goto no_memory;
}
- netVal = NULL;
if (hvm) {
if (def->nparallels) {
--
1.6.1.2.467.g081e7
6:10 a.m.
On Wed, Feb 04, 2009 at 10:24:48PM +0100, Jim Meyering wrote:
diff --git a/src/xm_internal.c b/src/xm_internal.c
index a03263c..c1e9e99 100644
--- a/src/xm_internal.c
+++ b/src/xm_internal.c
@@ -2239,8 +2239,6 @@ virConfPtr xenXMDomainConfigFormat(virConnectPtr conn,
diskVal = NULL;
goto no_memory;
}
- diskVal = NULL;
-
if (VIR_ALLOC(netVal) < 0)
goto no_memory;
@@ -2259,7 +2257,6 @@ virConfPtr xenXMDomainConfigFormat(virConnectPtr conn,
netVal = NULL;
goto no_memory;
}
- netVal = NULL;
if (hvm) {
if (def->nparallels) {
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.
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
6:55 a.m.
"Daniel P. Berrange" <berrange(a)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);
Here's the adjusted part of that c-set:
From 2582d7c339e48e008f3f0066a30d3bd34cb7823e Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering(a)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/bridge.c b/src/bridge.c
...
diff --git a/src/xm_internal.c b/src/xm_internal.c
index a03263c..a6e0bee 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) {
+ bool err = (virConfSetValue(conf, "disk", diskVal) < 0);
diskVal = NULL;
- goto no_memory;
+ if (err)
+ 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) {
+ bool err = (virConfSetValue(conf, "vif", netVal) < 0);
netVal = NULL;
- goto no_memory;
+ if (err)
+ goto no_memory;
}
- netVal = NULL;
+ VIR_FREE(netVal);
if (hvm) {
if (def->nparallels) {
--
1.6.1.2.511.gc5d3f
9:41 a.m.
On Thu, Feb 05, 2009 at 01:55:39PM +0100, Jim Meyering wrote:
"Daniel P. Berrange" <berrange(a)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;
Regards,
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
10:01 a.m.
"Daniel P. Berrange" <berrange(a)redhat.com> wrote:
On Thu, Feb 05, 2009 at 01:55:39PM +0100, Jim Meyering wrote:
> "Daniel P. Berrange" <berrange(a)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(a)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
10:11 a.m.
On Thu, Feb 05, 2009 at 05:01:55PM +0100, Jim Meyering wrote:
"Daniel P. Berrange" <berrange(a)redhat.com> wrote:
> On Thu, Feb 05, 2009 at 01:55:39PM +0100, Jim Meyering wrote:
>> "Daniel P. Berrange" <berrange(a)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(a)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(-)
ACK
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
6:08 a.m.
On Wed, Feb 04, 2009 at 06:44:52PM +0100, Jim Meyering wrote:
Barely worth posting for review, but who knows...
>From a7cae36a3d966e80d82859d63c60a303b63d6720 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering(a)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).
ACK
---
src/bridge.c | 6 ++----
src/cgroup.c | 1 -
2 files changed, 2 insertions(+), 5 deletions(-)
diff --git a/src/bridge.c b/src/bridge.c
index 9c4ca74..990a567 100644
--- a/src/bridge.c
+++ b/src/bridge.c
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2007 Red Hat, Inc.
+ * Copyright (C) 2007, 2009 Red Hat, Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -539,10 +539,8 @@ brAddTap(brControl *ctl,
if ((errno = brSetInterfaceUp(ctl, try.ifr_name, 1)))
goto error;
VIR_FREE(*ifname);
- if (!(*ifname = strdup(try.ifr_name))) {
- errno = ENOMEM;
+ if (!(*ifname = strdup(try.ifr_name)))
goto error;
- }
ACK, one day we should do the same for strdup() that we already did
for malloc() and wrap it in a way that gets us compile time checking
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
5803
days inactive
5804
days old
7 comments
2 participants
participants (2)
-
Daniel P. Berrange -
Jim Meyering