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@xxxxxxxxxx> > --- > > 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@xxxxxxxxxx > https://www.redhat.com/mailman/listinfo/libvir-list
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list