[libvirt] [PATCHv2 4/5] network: change cleanup: to error: in network*() functions

[Eric Blake]

On 08/14/2012 01:10 AM, Laine Stump wrote:
> A later patch will be adding a counter that will be
> incremented/decremented each time an guest interface starts/stops
> using a particular network. For this to work, all types of networks
> need to go through a common return sequence rather than returning
> early. To setup for this, the existing cleanup: label is renamed to
> error:, a new cleanup: label is added (when necessary), and early
> returns are changed to goto cleanup (except the case where the
> interface type != NETWORK).
> ---
>  src/network/bridge_driver.c | 106 ++++++++++++++++++++++----------------------
>  1 file changed, 53 insertions(+), 53 deletions(-)
> 
> @@ -3007,7 +3006,7 @@ networkAllocateActualDevice(virDomainNetDefPtr iface)
>                    dev->dev, dev->connections);
>      }
>      ret = 0;
> -cleanup:
> +error:
>      for (ii = 0; ii < num_virt_fns; ii++)

It looks weird to have the 'ret = 0' success case fall through into the
'error:' label.

> @@ -3114,8 +3113,9 @@ networkNotifyActualDevice(virDomainNetDefPtr iface)
>                    dev->dev, dev->connections);
>      }
>  
> -    ret = 0;
>  cleanup:
> +    ret = 0;
> +error:

and here, wouldn't it be better to name things 'success' for early exit
with ret = 0, and 'cleanup' for all cleanup whether on error or on
fallthrough from the ret = 0 case?

> @@ -3136,7 +3136,7 @@ int
>  networkReleaseActualDevice(virDomainNetDefPtr iface)
>  {
>      struct network_driver *driver = driverState;
> -    virNetworkObjPtr network = NULL;
> +    virNetworkObjPtr network;

I did a double-take on this one...

>      virNetworkDefPtr netdef;
>      const char *actualDev;
>      int ret = -1;
> @@ -3144,13 +3144,6 @@ networkReleaseActualDevice(virDomainNetDefPtr iface)
>      if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK)
>         return 0;
>  
> -    if (!iface->data.network.actual ||
> -        (virDomainNetGetActualType(iface) != VIR_DOMAIN_NET_TYPE_DIRECT)) {
> -        VIR_DEBUG("Nothing to release to network %s", iface->data.network.name);
> -        ret = 0;
> -        goto cleanup;
> -    }

...but it is correct; the code motion here means you no longer reach the
end labels prior to network being assigned.

> @@ -3199,8 +3198,9 @@ networkReleaseActualDevice(virDomainNetDefPtr iface)
>                    dev->dev, dev->connections);
>      }
>  
> -    ret = 0;
>  cleanup:
> +    ret = 0;
> +error:

Another case where this might make more sense:

success:
    ret = 0;
cleanup:

The cleanups are mechanical, but I'm not sure I like the resulting naming.

-- 
Eric Blake   eblake at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120814/ee5a9cde/attachment-0001.sig>