On Mon, Mar 14, 2016 at 07:28:12 -0400, John Ferlan wrote:
The virDomainConfVMNWFilterTeardown was called in the error path of
qemuBuildCommandLine once network setup was partially or fully completed
using the last_good_net as the basis to determine which filters needed
to be torn down. Commit id 'ef2ab8fd' moved that inside the new
qemuBuildNetCommandLine, so that lost the failure. Moving that cleanup
back outside the call to the more general failure case.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
Jan notes post push that the cleanup path after building the network
command line was there for any other failures and not just the failure
in building the network command line:
http://www.redhat.com/archives/libvir-list/2016-March/msg00561.html
Even though the teardown is called during qemuProcessStop from
virDomainConfVMNWFilterTeardown, the cleanup here could still be done.
The reason for moving it inside was based on a different patch where the
desire was to not leak anything that could have changed inside one of the
moved qemuBuild*CommandLine functions. In this case, though since it's
used more generally for cleanup, it seems a good idea to return it.
The other option would be to just let qemuProcessStop handle it and a
different patch to remove the error reset logic.
src/qemu/qemu_command.c | 18 ++++++++----------
1 file changed, 8 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index ba8c216..2e15c05 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7659,10 +7659,10 @@ qemuBuildNetCommandLine(virCommandPtr cmd,
bool emitBootindex,
size_t *nnicindexes,
int **nicindexes,
- int *bootHostdevNet)
+ int *bootHostdevNet,
+ int *last_good_net)
So ... this is fugly.
{
size_t i;
- int last_good_net = -1;
if (!def->nnets) {
/* If we have -device, then we set -nodefault already */
@@ -7697,9 +7697,9 @@ qemuBuildNetCommandLine(virCommandPtr cmd,
qemuCaps, vlan, bootNet, vmop,
standalone, nnicindexes,
nicindexes) < 0)
- goto error;
+ return -1;
- last_good_net = i;
+ *last_good_net = i;
/* if this interface is a type='hostdev' interface and we
* haven't yet added a "bootindex" parameter to an
* emulated network device, save the bootindex - hostdev
@@ -7714,11 +7714,6 @@ qemuBuildNetCommandLine(virCommandPtr cmd,
}
}
return 0;
-
- error:
- for (i = 0; last_good_net != -1 && i <= last_good_net; i++)
- virDomainConfNWFilterTeardown(def->nets[i]);
If you need to clean up a partial state, you should do it here and not
leak it in the caller ....
- return -1;
}
@@ -8604,6 +8599,7 @@ qemuBuildCommandLine(virConnectPtr conn,
bool emitBootindex = false;
virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
int bootHostdevNet = 0;
+ int last_good_net = -1;
VIR_DEBUG("conn=%p driver=%p def=%p mon=%p json=%d "
@@ -8725,7 +8721,7 @@ qemuBuildCommandLine(virConnectPtr conn,
if (qemuBuildNetCommandLine(cmd, driver, def, qemuCaps, vmop, standalone,
emitBootindex, nnicindexes, nicindexes,
- &bootHostdevNet) < 0)
+ &bootHostdevNet, &last_good_net) < 0)
goto error;
if (qemuBuildSmartcardCommandLine(logManager, cmd, def, qemuCaps) < 0)
@@ -9280,6 +9276,8 @@ qemuBuildCommandLine(virConnectPtr conn,
/* free up any resources in the network driver
* but don't overwrite the original error */
originalError = virSaveLastError();
+ for (i = 0; last_good_net != -1 && i <= last_good_net; i++)
+ virDomainConfNWFilterTeardown(def->nets[i]);
While the caller should remove none of the filters or all, depending on
whether you've successfully created them or not. Leaking state is ugly
and prone to breakages.
virSetError(originalError);
virFreeError(originalError);
virCommandFree(cmd);
--
2.5.0
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list