[libvirt] [PATCH v2] nwfilter: extend nwfilter reload support

v2: Fixes to the nwfilter driver reload function that also needs a valid virConnectPtr. In this patch I am extending and fixing the nwfilter module's reload support to stop all ongoing threads (for learning IP addresses of interfaces) and rebuild the filtering rules of all interfaces of all VMs when libvirt is started. Now libvirtd rebuilds the filters upon the SIGHUP signal and libvirtd restart. About the patch: The nwfilter functions require a virConnectPtr. Therefore I am opening a connection in qemudStartup, which later on needs to be closed outside where the driver lock is held since otherwise it ends up in a deadlock due to virConnectClose() trying to lock the driver as well. Signed-off-by: Stefan Berger <stefanb@us.ibm.com> --- src/nwfilter/nwfilter_driver.c | 21 ++++++++++---- src/nwfilter/nwfilter_learnipaddr.c | 15 +++++++--- src/nwfilter/nwfilter_learnipaddr.h | 1 src/qemu/qemu_driver.c | 52 +++++++++++++++++++++++++++++++++--- 4 files changed, 75 insertions(+), 14 deletions(-) Index: libvirt-acl/src/nwfilter/nwfilter_driver.c =================================================================== --- libvirt-acl.orig/src/nwfilter/nwfilter_driver.c +++ libvirt-acl/src/nwfilter/nwfilter_driver.c @@ -143,15 +143,25 @@ conf_init_err: */ static int nwfilterDriverReload(void) { + virConnectPtr conn; if (!driverState) { return -1; } - nwfilterDriverLock(driverState); - virNWFilterPoolLoadAllConfigs(NULL, - &driverState->pools, - driverState->configDir); - nwfilterDriverUnlock(driverState); + conn = virConnectOpen("qemu:///system"); + + if (conn) { + /* shut down all threads -- qemud for example will restart them */ + virNWFilterLearnThreadsTerminate(); + + nwfilterDriverLock(driverState); + virNWFilterPoolLoadAllConfigs(conn, + &driverState->pools, + driverState->configDir); + nwfilterDriverUnlock(driverState); + + virConnectClose(conn); + } return 0; } Index: libvirt-acl/src/nwfilter/nwfilter_learnipaddr.c =================================================================== --- libvirt-acl.orig/src/nwfilter/nwfilter_learnipaddr.c +++ libvirt-acl/src/nwfilter/nwfilter_learnipaddr.c @@ -855,6 +855,16 @@ virNWFilterLearnInit(void) { } +void +virNWFilterLearnThreadsTerminate() { + threadsTerminate = true; + + while (virHashSize(pendingLearnReq) != 0) + usleep((PKT_TIMEOUT_MS * 1000) / 3); + + threadsTerminate = false; +} + /** * virNWFilterLearnShutdown * Shutdown of this layer @@ -862,10 +872,7 @@ virNWFilterLearnInit(void) { void virNWFilterLearnShutdown(void) { - threadsTerminate = true; - - while (virHashSize(pendingLearnReq) != 0) - usleep((PKT_TIMEOUT_MS * 1000) / 3); + virNWFilterLearnThreadsTerminate(); virHashFree(pendingLearnReq, freeLearnReqEntry); pendingLearnReq = NULL; Index: libvirt-acl/src/nwfilter/nwfilter_learnipaddr.h =================================================================== --- libvirt-acl.orig/src/nwfilter/nwfilter_learnipaddr.h +++ libvirt-acl/src/nwfilter/nwfilter_learnipaddr.h @@ -71,5 +71,6 @@ void virNWFilterUnlockIface(const char * int virNWFilterLearnInit(void); void virNWFilterLearnShutdown(void); +void virNWFilterLearnThreadsTerminate(void); #endif /* __NWFILTER_LEARNIPADDR_H */ Index: libvirt-acl/src/qemu/qemu_driver.c =================================================================== --- libvirt-acl.orig/src/qemu/qemu_driver.c +++ libvirt-acl/src/qemu/qemu_driver.c @@ -170,6 +170,9 @@ static int qemuDetectVcpuPIDs(struct qem static int qemuUpdateActivePciHostdevs(struct qemud_driver *driver, virDomainDefPtr def); +static int qemudVMFiltersInstantiate(virConnectPtr conn, + virDomainDefPtr def); + static struct qemud_driver *qemu_driver = NULL; @@ -1423,6 +1426,10 @@ error: return ret; } +struct virReconnectDomainData { + virConnectPtr conn; + struct qemud_driver *driver; +}; /* * Open an existing VM's monitor, re-detect VCPU threads * and re-reserve the security labels in use @@ -1431,9 +1438,11 @@ static void qemuReconnectDomain(void *payload, const char *name ATTRIBUTE_UNUSED, void *opaque) { virDomainObjPtr obj = payload; - struct qemud_driver *driver = opaque; + struct virReconnectDomainData *data = opaque; + struct qemud_driver *driver = data->driver; qemuDomainObjPrivatePtr priv; unsigned long long qemuCmdFlags; + virConnectPtr conn = data->conn; virDomainObjLock(obj); @@ -1467,6 +1476,9 @@ qemuReconnectDomain(void *payload, const obj) < 0) goto error; + if (qemudVMFiltersInstantiate(conn, obj->def)) + goto error; + if (obj->def->id >= driver->nextvmid) driver->nextvmid = obj->def->id + 1; @@ -1491,9 +1503,10 @@ error: * about. */ static void -qemuReconnectDomains(struct qemud_driver *driver) +qemuReconnectDomains(virConnectPtr conn, struct qemud_driver *driver) { - virHashForEach(driver->domains.objs, qemuReconnectDomain, driver); + struct virReconnectDomainData data = {conn, driver}; + virHashForEach(driver->domains.objs, qemuReconnectDomain, &data); } @@ -1691,6 +1704,7 @@ qemudStartup(int privileged) { char *base = NULL; char driverConf[PATH_MAX]; int rc; + virConnectPtr conn = NULL; if (VIR_ALLOC(qemu_driver) < 0) return -1; @@ -1912,7 +1926,11 @@ qemudStartup(int privileged) { 1, NULL, NULL) < 0) goto error; - qemuReconnectDomains(qemu_driver); + conn = virConnectOpen(qemu_driver->privileged ? + "qemu:///system" : + "qemu:///session"); + + qemuReconnectDomains(conn, qemu_driver); /* Then inactive persistent configs */ if (virDomainLoadAllConfigs(qemu_driver->caps, @@ -1930,6 +1948,8 @@ qemudStartup(int privileged) { qemudAutostartConfigs(qemu_driver); + if (conn) + virConnectClose(conn); return 0; @@ -1938,6 +1958,8 @@ out_of_memory: error: if (qemu_driver) qemuDriverUnlock(qemu_driver); + if (conn) + virConnectClose(conn); VIR_FREE(base); qemudShutdown(); return -1; @@ -12731,6 +12753,28 @@ qemudVMFilterRebuild(virConnectPtr conn return 0; } +static int +qemudVMFiltersInstantiate(virConnectPtr conn, + virDomainDefPtr def) +{ + int err = 0; + int i; + + if (!conn) + return 1; + + for (i = 0 ; i < def->nnets ; i++) { + virDomainNetDefPtr net = def->nets[i]; + if ((net->filter) && (net->ifname)) { + if (virDomainConfNWFilterInstantiate(conn, net)) { + err = 1; + break; + } + } + } + + return err; +} static virNWFilterCallbackDriver qemuCallbackDriver = { .name = "QEMU",

On Thu, Aug 12, 2010 at 02:18:26PM -0400, Stefan Berger wrote:
Index: libvirt-acl/src/nwfilter/nwfilter_driver.c =================================================================== --- libvirt-acl.orig/src/nwfilter/nwfilter_driver.c +++ libvirt-acl/src/nwfilter/nwfilter_driver.c @@ -143,15 +143,25 @@ conf_init_err: */ static int nwfilterDriverReload(void) { + virConnectPtr conn; if (!driverState) { return -1; }
- nwfilterDriverLock(driverState); - virNWFilterPoolLoadAllConfigs(NULL, - &driverState->pools, - driverState->configDir); - nwfilterDriverUnlock(driverState); + conn = virConnectOpen("qemu:///system"); + + if (conn) { + /* shut down all threads -- qemud for example will restart them */ + virNWFilterLearnThreadsTerminate(); + + nwfilterDriverLock(driverState); + virNWFilterPoolLoadAllConfigs(conn, + &driverState->pools, + driverState->configDir); + nwfilterDriverUnlock(driverState); + + virConnectClose(conn); + }
return 0;
There's a small indentation issue here
} Index: libvirt-acl/src/nwfilter/nwfilter_learnipaddr.c =================================================================== --- libvirt-acl.orig/src/nwfilter/nwfilter_learnipaddr.c +++ libvirt-acl/src/nwfilter/nwfilter_learnipaddr.c @@ -855,6 +855,16 @@ virNWFilterLearnInit(void) { }
+void +virNWFilterLearnThreadsTerminate() { + threadsTerminate = true; + + while (virHashSize(pendingLearnReq) != 0) + usleep((PKT_TIMEOUT_MS * 1000) / 3); + + threadsTerminate = false; +}
Is there any risk of thread's failing to terminate, requiring us to kill them, or ignore them instead of blocking forever ? Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

Please respond to "Daniel P. Berrange"
On Thu, Aug 12, 2010 at 02:18:26PM -0400, Stefan Berger wrote:
Index: libvirt-acl/src/nwfilter/nwfilter_driver.c =================================================================== --- libvirt-acl.orig/src/nwfilter/nwfilter_driver.c +++ libvirt-acl/src/nwfilter/nwfilter_driver.c @@ -143,15 +143,25 @@ conf_init_err: */ static int nwfilterDriverReload(void) { + virConnectPtr conn; if (!driverState) { return -1; }
- nwfilterDriverLock(driverState); - virNWFilterPoolLoadAllConfigs(NULL, - &driverState->pools, - driverState->configDir); - nwfilterDriverUnlock(driverState); + conn = virConnectOpen("qemu:///system"); + + if (conn) { + /* shut down all threads -- qemud for example will restart
libvir-list-bounces@redhat.com wrote on 08/13/2010 10:44:51 AM: them */
+ virNWFilterLearnThreadsTerminate(); + + nwfilterDriverLock(driverState); + virNWFilterPoolLoadAllConfigs(conn, + &driverState->pools, + driverState->configDir); + nwfilterDriverUnlock(driverState); + + virConnectClose(conn); + }
return 0;
There's a small indentation issue here
Oh, will fix it.
} Index: libvirt-acl/src/nwfilter/nwfilter_learnipaddr.c =================================================================== --- libvirt-acl.orig/src/nwfilter/nwfilter_learnipaddr.c +++ libvirt-acl/src/nwfilter/nwfilter_learnipaddr.c @@ -855,6 +855,16 @@ virNWFilterLearnInit(void) { }
+void +virNWFilterLearnThreadsTerminate() { + threadsTerminate = true; + + while (virHashSize(pendingLearnReq) != 0) + usleep((PKT_TIMEOUT_MS * 1000) / 3); + + threadsTerminate = false; +}
Is there any risk of thread's failing to terminate, requiring us to kill them, or ignore them instead of blocking forever ?
A thread may just have been created with pthread_create() but hasn't run yet to see that threadTerminate is 'true' -> in that case the virHashSize() would count the thread until the thread itself has deregistered itself from the pendingLearnReq list. So that case should be ok. Assume a thread is just about to be created (due to a VM start for example) and registered with the call to rc = virNWFilterRegisterLearnReq(req) preceding the pthread_create(), in that case a 'threadsTerminate = false' would allow that thread to run then. So I guess the 'threadsTerminate = false' is wrong in that case and would prevent the thread from terminating. The challenge is to do this correctly for reloading of the driver and at the same time for shutting down. If I take out the 'threadsTerminate = false', all threads and those about to be born should terminate, which does it correctly for the libvirtd termination case. But in case of reload where I would want new threads to run again I should probably introduce a boolean parameter indicating into what state the threadsTerminate variable should go once this above function terminates. What that I would do the following changes: void virNWFilterLearnThreadsTerminate(bool allowNewThreads) { threadsTerminate = true; while (virHashSize(pendingLearnReq) != 0) usleep((PKT_TIMEOUT_MS * 1000) / 3); if (allowNewThreads) threadsTerminate = false; } Above call (with the indentation error) would then look like this: [...] if (conn) { /* shut down all threads -- qemud for example will restart them */ virNWFilterLearnThreadsTerminate(true); nwfilterDriverLock(driverState); virNWFilterPoolLoadAllConfigs(conn, &driverState->pools, driverState->configDir); nwfilterDriverUnlock(driverState); virConnectClose(conn); } [...] Does this sound right? Stefan
Daniel -- |: Red Hat, Engineering, London -o-
http://people.redhat.com/berrange/:|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org:| |: http://autobuild.org -o- http://search.cpan.org/~danberr/:| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (3)
-
Daniel P. Berrange
-
Stefan Berger
-
Stefan Berger