[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v2 1/3] networkStartNetwork: Be more verbose



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);


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]