[libvirt] [PATCH v3] nwfilter:

v3: - Fixed an indentation problem - added bool parameter to function terminating the IP address learner threads to determine whether future threads may still run (needed in case of driver reload) or all must terminate (need in case of libvirtd termination) 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. I have tested this now for a while with several machines running and needing the IP address learner thread(s). The rebuilding of the firewall rules seems to work fine following libvirtd restart or a SIGHUP. Also the termination of libvirtd worked fine. Signed-off-by: Stefan Berger <stefanb us ibm com> --- src/nwfilter/nwfilter_driver.c | 21 +++++++++++--- src/nwfilter/nwfilter_learnipaddr.c | 16 ++++++++--- src/nwfilter/nwfilter_learnipaddr.h | 1 src/qemu/qemu_driver.c | 52 +++++++++++++++++++++++++++++++++--- 4 files changed, 77 insertions(+), 13 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,26 @@ 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(true); + + 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 @@ -857,6 +857,17 @@ virNWFilterLearnInit(void) { } +void +virNWFilterLearnThreadsTerminate(bool allowNewThreads) { + threadsTerminate = true; + + while (virHashSize(pendingLearnReq) != 0) + usleep((PKT_TIMEOUT_MS * 1000) / 3); + + if (allowNewThreads) + threadsTerminate = false; +} + /** * virNWFilterLearnShutdown * Shutdown of this layer @@ -864,10 +875,7 @@ virNWFilterLearnInit(void) { void virNWFilterLearnShutdown(void) { - threadsTerminate = true; - - while (virHashSize(pendingLearnReq) != 0) - usleep((PKT_TIMEOUT_MS * 1000) / 3); + virNWFilterLearnThreadsTerminate(false); 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(bool allowNewThreads); #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; @@ -12738,6 +12760,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",

Incomplete subject line? It used to be nwfilter: extend nwfilter reload support On 08/13/2010 02:29 PM, Stefan Berger wrote:
v3: - Fixed an indentation problem - added bool parameter to function terminating the IP address learner threads to determine whether future threads may still run (needed in case of driver reload) or all must terminate (need in case of libvirtd termination)
+ conn = virConnectOpen("qemu:///system"); + + if (conn) { + /* shut down all threads -- qemud for example will restart them */ + virNWFilterLearnThreadsTerminate(true); + + nwfilterDriverLock(driverState); + virNWFilterPoolLoadAllConfigs(conn, + &driverState->pools,
Indentation problem still looks like it is here.
@@ -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");
How come this one is conditional, but the other one was hard-coded to system? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

libvir-list-bounces@redhat.com wrote on 08/13/2010 04:44:38 PM:
Incomplete subject line? It used to be nwfilter: extend nwfilter reload support
Yes. Due to a race condition :-)
On 08/13/2010 02:29 PM, Stefan Berger wrote:
v3: - Fixed an indentation problem - added bool parameter to function terminating the IP address learner threads to determine whether future threads may still run (needed in case of driver reload) or all must terminate (need in case
libvirtd termination)
+ conn = virConnectOpen("qemu:///system"); + + if (conn) { + /* shut down all threads -- qemud for example will restart
of them */
+ virNWFilterLearnThreadsTerminate(true); + + nwfilterDriverLock(driverState); + virNWFilterPoolLoadAllConfigs(conn, + &driverState->pools,
Indentation problem still looks like it is here.
The indentation problem doesn't exist in my local patch. Could this be another Thunderbird formatting issue?
@@ -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");
How come this one is conditional, but the other one was hard-coded to system?
The same type of check exists several times in this file of the qemu driver. I ended up hard-coding it in the nwfilter driver case above since I don't have a flag in the nwfilter driver for whether it's privileged or not and I don't know what the difference would be. Stefan

On 08/13/2010 03:11 PM, Stefan Berger wrote:
+ virNWFilterPoolLoadAllConfigs(conn, + &driverState->pools,
Indentation problem still looks like it is here.
The indentation problem doesn't exist in my local patch. Could this be another Thunderbird formatting issue?
Sorry, not this time. The fact that there are two leading + means that 'git diff' really did see two lines, and your second line is flush left instead of indented (and even still in the copy where you resent with a better subject line).
@@ -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");
How come this one is conditional, but the other one was hard-coded to system?
The same type of check exists several times in this file of the qemu driver. I ended up hard-coding it in the nwfilter driver case above since I don't have a flag in the nwfilter driver for whether it's privileged or not and I don't know what the difference would be.
The difference is whether you communicate with the libvirtd process running as root, or with just the session-specific qemu command. If you are running unprivileged but attempt to connect to qemu:///system, this will cause an authentication attempt (you have to supply the root password); but if the user is only running a qemu:///session, this is not what they want. I'm not sure what makes the right thing to do here; hopefully danpb can shed some better insight into this. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

Eric Blake <eblake@redhat.com> wrote on 08/13/2010 05:16:39 PM:
On 08/13/2010 03:11 PM, Stefan Berger wrote:
+ virNWFilterPoolLoadAllConfigs(conn, + &driverState->pools,
Indentation problem still looks like it is here.
The indentation problem doesn't exist in my local patch. Could this be
another Thunderbird formatting issue?
Sorry, not this time. The fact that there are two leading + means that 'git diff' really did see two lines, and your second line is flush left instead of indented (and even still in the copy where you resent with a better subject line).
This is how the lines around this look like. The removal of the line that starts with &driverState->pools is also wrong and I did not re-format that one, either (using quilt). I am using Thunderbird 3.3.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(true); + + nwfilterDriverLock(driverState); + virNWFilterPoolLoadAllConfigs(conn, + &driverState->pools, + driverState->configDir); + nwfilterDriverUnlock(driverState); + + virConnectClose(conn); + }
The difference is whether you communicate with the libvirtd process running as root, or with just the session-specific qemu command. If you are running unprivileged but attempt to connect to qemu:///system, this will cause an authentication attempt (you have to supply the root password); but if the user is only running a qemu:///session, this is not what they want.
I'm not sure what makes the right thing to do here; hopefully danpb can shed some better insight into this.
Since this is an libvirtd-internal call I suppose qemu:///system should be alright... I'll wait to hear from Dan then. Stefan

On Fri, Aug 13, 2010 at 02:44:38PM -0600, Eric Blake wrote:
Incomplete subject line? It used to be nwfilter: extend nwfilter reload support
On 08/13/2010 02:29 PM, Stefan Berger wrote:
v3: - Fixed an indentation problem - added bool parameter to function terminating the IP address learner threads to determine whether future threads may still run (needed in case of driver reload) or all must terminate (need in case of libvirtd termination)
+ conn = virConnectOpen("qemu:///system"); + + if (conn) { + /* shut down all threads -- qemud for example will restart them */ + virNWFilterLearnThreadsTerminate(true); + + nwfilterDriverLock(driverState); + virNWFilterPoolLoadAllConfigs(conn, + &driverState->pools,
Indentation problem still looks like it is here.
@@ -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");
How come this one is conditional, but the other one was hard-coded to system?
The 'nwfilter' driver is only run when it is privileged, totally disabled otherwise. The QEMU driver runs privileged and unprivilegd. So this difference is ok 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 :|
participants (4)
-
Daniel P. Berrange
-
Eric Blake
-
Stefan Berger
-
Stefan Berger