We have many places where the earliest error returns from a function
skip any cleanup label at the bottom (the assumption being that it is
so early in the function that there isn't yet anything that needs to
be explicitly undone on failure). But in general it is a bad sign if
there are any direct "return" statements in a function at any time
after there has been a "goto cleanup" - that indicates someone thought
that an earlier point in the code had done something needing cleanup,
so we shouldn't be skipping it.
There were two occurences of a "return -1" after "goto cleanup" in
qemuDomainAttachDeviceNet(). The first of these has been around for a
very long time (since 2013) and my assumption is that the earlier
"goto cleanup" didn't exist at that time (so it was proper), and when
the code further up in the function was added, the this return -1 was
missed. The second was added during a mass change to check the return
from qemuInterfacePrepareSlirp() in several places (commit
99a1cfc43889c6d); in this case it was erroneous from the start.
Change both of these "return -1"s to "goto cleanup". Since we already
have code paths earlier in the function that goto cleanup, this should
not cause any new problem.
Signed-off-by: Laine Stump <laine@xxxxxxxxxx>
---
src/qemu/qemu_hotplug.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index c6f275e11d..244cf65c87 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1212,7 +1212,7 @@ qemuDomainAttachNetDevice(virQEMUDriver *driver,
/* final validation now that we have full info on the type */
if (qemuDomainValidateActualNetDef(net, priv->qemuCaps) < 0)
- return -1;
+ goto cleanup;
actualType = virDomainNetGetActualType(net);
@@ -1330,7 +1330,7 @@ qemuDomainAttachNetDevice(virQEMUDriver *driver,
int rv = qemuInterfacePrepareSlirp(driver, net, &slirp);
if (rv == -1)
- return -1;
+ goto cleanup;
if (rv == 0)
break;
--
2.31.1
