src/bhyve/bhyve_command.c | 39 +++++++++++++++++---------------------- 1 file changed, 17 insertions(+), 22 deletions(-)
Use 'goto cleanup'-style error handling instead of explicitly
freeing variables in every error path.
---
src/bhyve/bhyve_command.c | 39 +++++++++++++++++----------------------
1 file changed, 17 insertions(+), 22 deletions(-)
diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c
index 022b03b..4914d98 100644
--- a/src/bhyve/bhyve_command.c
+++ b/src/bhyve/bhyve_command.c
@@ -52,26 +52,25 @@ bhyveBuildNetArgStr(const virDomainDef *def,
char macaddr[VIR_MAC_STRING_BUFLEN];
char *realifname = NULL;
char *brname = NULL;
+ int ret = -1;
virDomainNetType actualType = virDomainNetGetActualType(net);
if (actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) {
if (VIR_STRDUP(brname, virDomainNetGetActualBridgeName(net)) < 0)
- return -1;
+ goto cleanup;
} else {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("Network type %d is not supported"),
virDomainNetGetActualType(net));
- return -1;
+ goto cleanup;
}
if (!net->ifname ||
STRPREFIX(net->ifname, VIR_NET_GENERATED_PREFIX) ||
strchr(net->ifname, '%')) {
VIR_FREE(net->ifname);
- if (VIR_STRDUP(net->ifname, VIR_NET_GENERATED_PREFIX "%d") < 0) {
- VIR_FREE(brname);
- return -1;
- }
+ if (VIR_STRDUP(net->ifname, VIR_NET_GENERATED_PREFIX "%d") < 0)
+ goto cleanup;
}
if (!dryRun) {
@@ -80,33 +79,24 @@ bhyveBuildNetArgStr(const virDomainDef *def,
virDomainNetGetActualVirtPortProfile(net),
virDomainNetGetActualVlan(net),
VIR_NETDEV_TAP_CREATE_IFUP | VIR_NETDEV_TAP_CREATE_PERSIST) < 0) {
- VIR_FREE(net->ifname);
- VIR_FREE(brname);
- return -1;
+ goto cleanup;
}
realifname = virNetDevTapGetRealDeviceName(net->ifname);
- if (realifname == NULL) {
- VIR_FREE(net->ifname);
- VIR_FREE(brname);
- return -1;
- }
+ if (realifname == NULL)
+ goto cleanup;
VIR_DEBUG("%s -> %s", net->ifname, realifname);
/* hack on top of other hack: we need to set
* interface to 'UP' again after re-opening to find its
* name
*/
- if (virNetDevSetOnline(net->ifname, true) != 0) {
- VIR_FREE(realifname);
- VIR_FREE(net->ifname);
- VIR_FREE(brname);
- return -1;
- }
+ if (virNetDevSetOnline(net->ifname, true) != 0)
+ goto cleanup;
} else {
if (VIR_STRDUP(realifname, "tap0") < 0)
- return -1;
+ goto cleanup;
}
@@ -114,9 +104,14 @@ bhyveBuildNetArgStr(const virDomainDef *def,
virCommandAddArgFormat(cmd, "%d:0,virtio-net,%s,mac=%s",
net->info.addr.pci.slot,
realifname, virMacAddrFormat(&net->mac, macaddr));
+
+ ret = 0;
+ cleanup:
+ VIR_FREE(brname);
+ VIR_FREE(net->ifname);
VIR_FREE(realifname);
- return 0;
+ return ret;
}
static int
--
2.10.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Nov 21, 2016 at 06:45:05PM +0300, Roman Bogorodskiy wrote: >Use 'goto cleanup'-style error handling instead of explicitly >freeing variables in every error path. >--- > src/bhyve/bhyve_command.c | 39 +++++++++++++++++---------------------- > 1 file changed, 17 insertions(+), 22 deletions(-) > >diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c >index 022b03b..4914d98 100644 >--- a/src/bhyve/bhyve_command.c >+++ b/src/bhyve/bhyve_command.c >@@ -114,9 +104,14 @@ bhyveBuildNetArgStr(const virDomainDef *def, > virCommandAddArgFormat(cmd, "%d:0,virtio-net,%s,mac=%s", > net->info.addr.pci.slot, > realifname, virMacAddrFormat(&net->mac, macaddr)); >+ >+ ret = 0; >+ cleanup: >+ VIR_FREE(brname); Okay, brname was leaked before. >+ VIR_FREE(net->ifname); But freeing stuff from the virDomainNetDef structure on success seems wrong. Previously, all the error codepaths except if (VIR_STRDUP(realifname, "tap0") < 0) on a dry run left net->ifname at NULL, but after this patch it seems the network will not be cleaned up later even though it was created successfully. ACK if you wrap it in: if (ret < 0) Jan -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Ján Tomko wrote: > On Mon, Nov 21, 2016 at 06:45:05PM +0300, Roman Bogorodskiy wrote: > >Use 'goto cleanup'-style error handling instead of explicitly > >freeing variables in every error path. > >--- > > src/bhyve/bhyve_command.c | 39 +++++++++++++++++---------------------- > > 1 file changed, 17 insertions(+), 22 deletions(-) > > > >diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c > >index 022b03b..4914d98 100644 > >--- a/src/bhyve/bhyve_command.c > >+++ b/src/bhyve/bhyve_command.c > > >@@ -114,9 +104,14 @@ bhyveBuildNetArgStr(const virDomainDef *def, > > virCommandAddArgFormat(cmd, "%d:0,virtio-net,%s,mac=%s", > > net->info.addr.pci.slot, > > realifname, virMacAddrFormat(&net->mac, macaddr)); > >+ > >+ ret = 0; > >+ cleanup: > >+ VIR_FREE(brname); > > Okay, brname was leaked before. Yeah, I've noticed that while working on the e1000 support patches. > >+ VIR_FREE(net->ifname); > > But freeing stuff from the virDomainNetDef structure on success seems > wrong. > > Previously, all the error codepaths except > if (VIR_STRDUP(realifname, "tap0") < 0) > on a dry run left net->ifname at NULL, but after this patch it seems > the network will not be cleaned up later even though it was created > successfully. > > ACK if you wrap it in: > if (ret < 0) Pushed with this change squashed, thanks! Roman Bogorodskiy -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2024 Red Hat, Inc.