[libvirt] [PATCH] bridge_driver: cleanup improvements in dhcpStartDhcpDaemon()
Run VIR_FREE only for non-NULL pointers. --- src/network/bridge_driver.c | 17 +++++++++-------- 1 files changed, 9 insertions(+), 8 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index b0834ae..f2857b4 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -534,34 +534,34 @@ dhcpStartDhcpDaemon(virNetworkObjPtr network) if (!VIR_SOCKET_IS_FAMILY(&network->def->ipAddress, AF_INET)) { networkReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot start dhcp daemon without IPv4 address for server")); - goto cleanup; + goto cleanup2; } if ((err = virFileMakePath(NETWORK_PID_DIR)) != 0) { virReportSystemError(err, _("cannot create directory %s"), NETWORK_PID_DIR); - goto cleanup; + goto cleanup2; } if ((err = virFileMakePath(NETWORK_STATE_DIR)) != 0) { virReportSystemError(err, _("cannot create directory %s"), NETWORK_STATE_DIR); - goto cleanup; + goto cleanup2; } if (!(pidfile = virFilePid(NETWORK_PID_DIR, network->def->name))) { virReportOOMError(); - goto cleanup; + goto cleanup2; } cmd = virCommandNew(DNSMASQ); if (networkBuildDnsmasqArgv(network, pidfile, cmd) < 0) { - goto cleanup; + goto cleanup1; } if (virCommandRun(cmd, NULL) < 0) - goto cleanup; + goto cleanup1; /* * There really is no race here - when dnsmasq daemonizes, its @@ -573,12 +573,13 @@ dhcpStartDhcpDaemon(virNetworkObjPtr network) if (virFileReadPid(NETWORK_PID_DIR, network->def->name, &network->dnsmasqPid) < 0) - goto cleanup; + goto cleanup1; ret = 0; -cleanup: +cleanup1: VIR_FREE(pidfile); virCommandFree(cmd); +cleanup2: return ret; }
2010/12/21 Paweł Krześniak <pawel.krzesniak@gmail.com>:
Run VIR_FREE only for non-NULL pointers. --- src/network/bridge_driver.c | 17 +++++++++-------- 1 files changed, 9 insertions(+), 8 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index b0834ae..f2857b4 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -534,34 +534,34 @@ dhcpStartDhcpDaemon(virNetworkObjPtr network) if (!VIR_SOCKET_IS_FAMILY(&network->def->ipAddress, AF_INET)) { networkReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot start dhcp daemon without IPv4 address for server")); - goto cleanup; + goto cleanup2; }
ret = 0; -cleanup: +cleanup1: VIR_FREE(pidfile); virCommandFree(cmd); +cleanup2: return ret; }
NACK as written, because you're complicating this function unnecessarily. It's totally fine to call VIR_FREE on a possible NULL pointer without checking, this is the common pattern in libvirt. Your additional label resembles the logic of if (pointer != NULL) { VIR_FREE(pointer); } This pattern in explicitly banned in libvirt by the make syntax-check rules and should to be simplified to VIR_FREE(pointer); as VIR_FREE (just like the normal free function) is safe to be called on a NULL pointer. If you really insists in avoiding the VIR_FREE call in the error path when the pointer is NULL, then you could just replace your goto cleanup2 by return -1, otherwise I suggest to leave this function as is, because there is nothing to fix here. Matthias
participants (2)
-
Matthias Bolte -
Paweł Krześniak