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