[libvirt] [PATCH] libxl: unref objects in error paths

[Michal Privoznik]

On 24.02.2016 23:55, Jim Fehlig wrote:
> libxlMakeNic opens a virConnect object and takes a reference on a
> virNetwork object, but doesn't drop the references on all error
> paths. Rework the function to follow the standard libvirt pattern
> of using a local 'ret' variable to hold the function return value,
> performing all cleanup and returning 'ret' at a 'cleanup' label.
> 
> Signed-off-by: Jim Fehlig <jfehlig at suse.com>
> ---
> 
> I noticed these leaks while trying to rework the code to use a
> common virConnect object. That task has turned into quite an
> effort [1], but in the meantime the leaks in the current code
> should be plugged.
> 
> [1] https://www.redhat.com/archives/libvir-list/2016-February/msg01188.html
> 
>  src/libxl/libxl_conf.c | 57 ++++++++++++++++++++++++--------------------------
>  1 file changed, 27 insertions(+), 30 deletions(-)
> 
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index b20df29..763f4c5 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -1286,7 +1286,11 @@ libxlMakeNic(virDomainDefPtr def,
>  {
>      bool ioemu_nic = def->os.type == VIR_DOMAIN_OSTYPE_HVM;
>      virDomainNetType actual_type = virDomainNetGetActualType(l_nic);
> +    char *brname = NULL;
> +    virNetworkPtr network = NULL;
> +    virConnectPtr conn = NULL;
>      virNetDevBandwidthPtr actual_bw;
> +    int ret = -1;
>  
>      /* TODO: Where is mtu stored?
>       *
> @@ -1312,64 +1316,50 @@ libxlMakeNic(virDomainDefPtr def,
>  
>      if (l_nic->model) {
>          if (VIR_STRDUP(x_nic->model, l_nic->model) < 0)
> -            return -1;
> +            goto cleanup;
>          if (STREQ(l_nic->model, "netfront"))
>              x_nic->nictype = LIBXL_NIC_TYPE_VIF;
>      }
>  
>      if (VIR_STRDUP(x_nic->ifname, l_nic->ifname) < 0)
> -        return -1;
> +        goto cleanup;
>  
>      switch (actual_type) {
>          case VIR_DOMAIN_NET_TYPE_BRIDGE:
>              if (VIR_STRDUP(x_nic->bridge,
>                             virDomainNetGetActualBridgeName(l_nic)) < 0)
> -                return -1;
> +                goto cleanup;
>              /* fallthrough */
>          case VIR_DOMAIN_NET_TYPE_ETHERNET:
>              if (VIR_STRDUP(x_nic->script, l_nic->script) < 0)
> -                return -1;
> +                goto cleanup;
>              if (l_nic->nips > 0) {
>                  x_nic->ip = virSocketAddrFormat(&l_nic->ips[0]->address);
>                  if (!x_nic->ip)
> -                    return -1;
> +                    goto cleanup;
>              }
>              break;
>          case VIR_DOMAIN_NET_TYPE_NETWORK:
>          {
> -            bool fail = false;
> -            char *brname = NULL;
> -            virNetworkPtr network;
> -            virConnectPtr conn;
> -
>              if (!(conn = virConnectOpen("xen:///system")))
> -                return -1;
> +                goto cleanup;
>  
>              if (!(network =
>                    virNetworkLookupByName(conn, l_nic->data.network.name))) {
> -                virObjectUnref(conn);
> -                return -1;
> +                goto cleanup;
>              }
>  
>              if (l_nic->nips > 0) {
>                  x_nic->ip = virSocketAddrFormat(&l_nic->ips[0]->address);
>                  if (!x_nic->ip)
> -                    return -1;
> +                    goto cleanup;
>              }
>  
> -            if ((brname = virNetworkGetBridgeName(network))) {
> -                if (VIR_STRDUP(x_nic->bridge, brname) < 0)
> -                    fail = true;
> -            } else {
> -                fail = true;
> -            }
> -
> -            VIR_FREE(brname);
> +            if (!(brname = virNetworkGetBridgeName(network)))
> +                goto cleanup;
>  
> -            virObjectUnref(network);
> -            virObjectUnref(conn);
> -            if (fail)
> -                return -1;
> +            if (VIR_STRDUP(x_nic->bridge, brname) < 0)
> +                    goto cleanup;

How about switching pointers instead of yet another strdup?

x_nic->bridge = brname;
brname = NULL;

And as Joao already pointed out, the indentation of 'goto' is a bit off.
Otherwise looking good. ACK and safe for the freeze.

Michal