[libvirt] [PATCH] nwfilter: fix tear down order and consolidate functions
To avoid race-conditions, the tear down of a filter has to happen before the tap interface disappears and another tap interface with the same name can re-appear. This patch tries to fix this. In one place, where communication with the qemu monitor may fail, I am only tearing the filters down after knowing that the function did not fail. I am also moving the tear down functions into an include file for other drivers to reuse. Signed-off-by: Stefan Berger <stefanb@us.ibm.com> --- src/nwfilter/nwfilter_gentech_driver.h | 14 ++++++++++++++ src/qemu/qemu_conf.c | 13 ++++++++----- src/qemu/qemu_driver.c | 28 +++++++--------------------- 3 files changed, 29 insertions(+), 26 deletions(-) Index: libvirt-acl/src/qemu/qemu_conf.c =================================================================== --- libvirt-acl.orig/src/qemu/qemu_conf.c +++ libvirt-acl/src/qemu/qemu_conf.c @@ -4074,10 +4074,13 @@ int qemudBuildCommandLine(virConnectPtr goto error; if (VIR_REALLOC_N(*tapfds, (*ntapfds)+1) < 0) { + virNWFilterTearNWFilter(net); close(tapfd); goto no_memory; } + last_good_net = i; + (*tapfds)[(*ntapfds)++] = tapfd; if (snprintf(tapfd_name, sizeof(tapfd_name), "%d", tapfd) >= sizeof(tapfd_name)) @@ -4091,10 +4094,13 @@ int qemudBuildCommandLine(virConnectPtr goto error; if (VIR_REALLOC_N(*tapfds, (*ntapfds)+1) < 0) { + virNWFilterTearNWFilter(net); close(tapfd); goto no_memory; } + last_good_net = i; + (*tapfds)[(*ntapfds)++] = tapfd; if (snprintf(tapfd_name, sizeof(tapfd_name), "%d", tapfd) >= sizeof(tapfd_name)) @@ -4154,7 +4160,6 @@ int qemudBuildCommandLine(virConnectPtr goto error; ADD_ARG(host); } - last_good_net = i; } } @@ -4603,6 +4608,8 @@ int qemudBuildCommandLine(virConnectPtr no_memory: virReportOOMError(); error: + for (i = 0; i <= last_good_net; i++) + virNWFilterTearNWFilter(def->nets[i]); if (tapfds && *tapfds) { for (i = 0; i < *ntapfds; i++) @@ -4620,11 +4627,6 @@ int qemudBuildCommandLine(virConnectPtr VIR_FREE((qenv)[i]); VIR_FREE(qenv); } - for (i = 0; i <= last_good_net; i++) { - virDomainNetDefPtr net = def->nets[i]; - if ((net->filter) && (net->ifname)) - virNWFilterTeardownFilter(net); - } return -1; #undef ADD_ARG Index: libvirt-acl/src/qemu/qemu_driver.c =================================================================== --- libvirt-acl.orig/src/qemu/qemu_driver.c +++ libvirt-acl/src/qemu/qemu_driver.c @@ -3070,21 +3070,6 @@ cleanup: } -static void -qemuTearNWFilter(virDomainNetDefPtr net) { - if ((net->filter) && (net->ifname)) - virNWFilterTeardownFilter(net); -} - - -static void -qemuTearVMNWFilters(virDomainObjPtr vm) { - int i; - for (i = 0; i < vm->def->nnets; i++) - qemuTearNWFilter(vm->def->nets[i]); -} - - struct qemudHookData { virConnectPtr conn; virDomainObjPtr vm; @@ -3397,6 +3382,9 @@ static int qemudStartVMDaemon(virConnect VIR_FREE(progenv[i]); VIR_FREE(progenv); + if (ret == -1) /* The VM failed to start; tear filters before taps */ + virNWFilterTearVMNWFilters(vm); + if (tapfds) { for (i = 0 ; i < ntapfds ; i++) { close(tapfds[i]); @@ -3461,8 +3449,6 @@ cleanup: /* We jump here if we failed to start the VM for any reason * XXX investigate if we can kill this block and safely call * qemudShutdownVMDaemon even though no PID is running */ - qemuTearVMNWFilters(vm); - qemuDomainReAttachHostDevices(driver, vm->def); if (driver->securityDriver && @@ -3511,7 +3497,7 @@ static void qemudShutdownVMDaemon(struct * reporting so we don't squash a legit error. */ orig_err = virSaveLastError(); - qemuTearVMNWFilters(vm); + virNWFilterTearVMNWFilters(vm); if (driver->macFilter) { def = vm->def; @@ -7153,7 +7139,8 @@ cleanup: qemuDomainPCIAddressReleaseAddr(priv->pciaddrs, &net->info) < 0) VIR_WARN0("Unable to release PCI address on NIC"); - qemuTearNWFilter(net); + if (ret != 0) + virNWFilterTearNWFilter(net); VIR_FREE(nicstr); VIR_FREE(netstr); @@ -7953,6 +7940,8 @@ qemudDomainDetachNetDevice(struct qemud_ } qemuDomainObjExitMonitorWithDriver(driver, vm); + virNWFilterTearNWFilter(detach); + #if WITH_MACVTAP if (detach->type == VIR_DOMAIN_NET_TYPE_DIRECT) { if (detach->ifname) @@ -7970,8 +7959,6 @@ qemudDomainDetachNetDevice(struct qemud_ } } - qemuTearNWFilter(detach); - if (vm->def->nnets > 1) { memmove(vm->def->nets + i, vm->def->nets + i + 1, Index: libvirt-acl/src/nwfilter/nwfilter_gentech_driver.h =================================================================== --- libvirt-acl.orig/src/nwfilter/nwfilter_gentech_driver.h +++ libvirt-acl/src/nwfilter/nwfilter_gentech_driver.h @@ -65,4 +65,20 @@ void virNWFilterDomainFWUpdateCB(void *p const char *name ATTRIBUTE_UNUSED, void *data); + +/* tear down an interface's filter before tearing down the interface */ +static inline void +virNWFilterTearNWFilter(virDomainNetDefPtr net) { + if ((net->filter) && (net->ifname)) + virNWFilterTeardownFilter(net); +} + + +static inline void +virNWFilterTearVMNWFilters(virDomainObjPtr vm) { + int i; + for (i = 0; i < vm->def->nnets; i++) + virNWFilterTearNWFilter(vm->def->nets[i]); +} + #endif
On 04/14/2010 03:10 PM, Stefan Berger wrote:
To avoid race-conditions, the tear down of a filter has to happen before the tap interface disappears and another tap interface with the same name can re-appear. This patch tries to fix this. In one place, where communication with the qemu monitor may fail, I am only tearing the filters down after knowing that the function did not fail.
I am also moving the tear down functions into an include file for other drivers to reuse.
ACK.
+static inline void +virNWFilterTearVMNWFilters(virDomainObjPtr vm) { + int i; + for (i = 0; i < vm->def->nnets; i++)
Is it worth using C99 syntax, to condense two lines to one? for (int i = 0; ... But that's a general question, and should not hold up your patch. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Thu, Apr 15, 2010 at 08:27:07AM -0600, Eric Blake wrote:
On 04/14/2010 03:10 PM, Stefan Berger wrote:
To avoid race-conditions, the tear down of a filter has to happen before the tap interface disappears and another tap interface with the same name can re-appear. This patch tries to fix this. In one place, where communication with the qemu monitor may fail, I am only tearing the filters down after knowing that the function did not fail.
I am also moving the tear down functions into an include file for other drivers to reuse.
ACK.
ACK too,
+static inline void +virNWFilterTearVMNWFilters(virDomainObjPtr vm) { + int i; + for (i = 0; i < vm->def->nnets; i++)
Is it worth using C99 syntax, to condense two lines to one?
for (int i = 0; ...
But that's a general question, and should not hold up your patch.
so far we never did it in the code, so for consistency I would avoid that, I also like to have an empty line after variable(s) declarations. Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
participants (3)
-
Daniel Veillard -
Eric Blake -
Stefan Berger