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

[Jim Fehlig]

Michal Privoznik wrote:
> 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?

Ah, good point. In fact, the brname local can be dropped and the result of
virNetworkGetBridgeName assigned directly to x_nic->bridge. E.g.

  if (!(x_nic->bridge = virNetworkGetBridgeName(network)))
      goto cleanup;

> 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.

I've made the above change and pushed the patch. Thanks!

Regards,
Jim