[libvirt] [PATCH] remove useless code
Barely worth posting for review, but who knows...
From a7cae36a3d966e80d82859d63c60a303b63d6720 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/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
Jim Meyering <jim@meyering.net> wrote:
From a7cae36a3d966e80d82859d63c60a303b63d6720 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).
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@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
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 :|
"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); Here's the adjusted part of that c-set:
From 2582d7c339e48e008f3f0066a30d3bd34cb7823e 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/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
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; 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 :|
"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
On Thu, Feb 05, 2009 at 05:01:55PM +0100, Jim Meyering wrote:
"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(-)
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 :|
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@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 :|
participants (2)
-
Daniel P. Berrange -
Jim Meyering