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@xxxxxxxxxx>
> ---
> 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);
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list