From 41b087198d09c5a416bdb9c2e037997234193fb2 Mon Sep 17 00:00:00 2001
From: Stefan Berger <stefanb@us.ibm.com>
Date: Thu, 15 Apr 2010 10:49:24 -0400
Subject: [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.
---
 src/nwfilter/nwfilter_gentech_driver.h | 16 ++++++++++++++
 src/qemu/qemu_conf.c                   | 14 +++++++------
 src/qemu/qemu_driver.c                 | 29 +++++++-------------------
 3 files changed, 32 insertions(+), 27 deletions(-)

diff --git a/src/nwfilter/nwfilter_gentech_driver.h b/src/nwfilter/nwfilter_gentech_driver.h
index 9aed0e9b1c..606a73d05d 100644
--- a/src/nwfilter/nwfilter_gentech_driver.h
+++ b/src/nwfilter/nwfilter_gentech_driver.h
@@ -65,4 +65,20 @@ void virNWFilterDomainFWUpdateCB(void *payload,
                                  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
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 48252a5363..03fc29ad32 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -4074,10 +4074,13 @@ int qemudBuildCommandLine(virConnectPtr conn,
                     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 conn,
                     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 conn,
                     goto error;
                 ADD_ARG(host);
             }
-            last_good_net = i;
         }
     }
 
@@ -4603,6 +4608,8 @@ int qemudBuildCommandLine(virConnectPtr conn,
  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 conn,
             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
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index f5cf1f1538..f6fdb280f5 100644
--- a/src/qemu/qemu_driver.c
+++ b/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(virConnectPtr conn,
         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 qemud_driver *driver,
      * reporting so we don't squash a legit error. */
     orig_err = virSaveLastError();
 
-    qemuTearVMNWFilters(vm);
+    virNWFilterTearVMNWFilters(vm);
 
     if (driver->macFilter) {
         def = vm->def;
@@ -7158,7 +7144,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);
@@ -7958,6 +7945,8 @@ qemudDomainDetachNetDevice(struct qemud_driver *driver,
     }
     qemuDomainObjExitMonitorWithDriver(driver, vm);
 
+    virNWFilterTearNWFilter(detach);
+
 #if WITH_MACVTAP
     if (detach->type == VIR_DOMAIN_NET_TYPE_DIRECT) {
         if (detach->ifname)
@@ -7975,8 +7964,6 @@ qemudDomainDetachNetDevice(struct qemud_driver *driver,
         }
     }
 
-    qemuTearNWFilter(detach);
-
     if (vm->def->nnets > 1) {
         memmove(vm->def->nets + i,
                 vm->def->nets + i + 1,