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