On 02/05/2014 12:11 PM, Michal Privoznik wrote:
The lack of debug printings might be frustrating in the future. Moreover, this function doesn't follow the usual pattern we have in the rest of the code:
int ret = -1; /* do some work */ ret = 0; cleanup: /* some cleanup work */ return ret;
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/bridge_driver.c | 35 +++++++++++++++++++---------------- 1 file changed, 19 insertions(+), 16 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index c8b167b..2bd015b 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -1992,23 +1992,29 @@ static int networkStartNetwork(virNetworkDriverStatePtr driver, virNetworkObjPtr network) { - int ret = 0; + int ret = -1; + + VIR_DEBUG("driver=%p, network=%p", driver, network);
if (virNetworkObjIsActive(network)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("network is already active")); - return -1; + return ret;
Okay, this fixes the one problem I saw with v1 (calling UnsetDefTransient on cleanup for a network that was already active).
}
+ VIR_DEBUG("Beginning network startup process"); + + VIR_DEBUG("Setting current network def as transient"); if (virNetworkObjSetDefTransient(network, true) < 0) - return -1; + goto cleanup;
I *think* it's safe to call UnsetDefTransient when SetDefTransient failed, so this is probably okay. And the rest of cleanup (which is mostly just a call to networkShutdownNetwork(), after saving any errors) is safe as well - it's a NOP if the network isn't active. ACK.
switch (network->def->forward.type) {
case VIR_NETWORK_FORWARD_NONE: case VIR_NETWORK_FORWARD_NAT: case VIR_NETWORK_FORWARD_ROUTE: - ret = networkStartNetworkVirtual(driver, network); + if (networkStartNetworkVirtual(driver, network) < 0) + goto cleanup; break;
case VIR_NETWORK_FORWARD_BRIDGE: @@ -2016,28 +2022,25 @@ networkStartNetwork(virNetworkDriverStatePtr driver, case VIR_NETWORK_FORWARD_VEPA: case VIR_NETWORK_FORWARD_PASSTHROUGH: case VIR_NETWORK_FORWARD_HOSTDEV: - ret = networkStartNetworkExternal(driver, network); + if (networkStartNetworkExternal(driver, network) < 0) + goto cleanup; break; }
- if (ret < 0) { - virNetworkObjUnsetDefTransient(network); - return ret; - } - /* Persist the live configuration now that anything autogenerated * is setup. */ - if ((ret = virNetworkSaveStatus(driverState->stateDir, - network)) < 0) { - goto error; - } + VIR_DEBUG("Writing network status to disk"); + if (virNetworkSaveStatus(driverState->stateDir, network) < 0) + goto cleanup;
- VIR_INFO("Starting up network '%s'", network->def->name); network->active = 1; + VIR_INFO("Network '%s' started up", network->def->name); + ret = 0;
-error: +cleanup: if (ret < 0) { + virNetworkObjUnsetDefTransient(network); virErrorPtr save_err = virSaveLastError(); int save_errno = errno; networkShutdownNetwork(driver, network);