On 06/03/2013 12:29 PM, Laine Stump wrote:

On 06/03/2013 11:57 AM, Stefan Berger wrote:

...

This patch is in relation to Bug 966449:

https://bugzilla.redhat.com/show_bug.cgi?id=966449

Below is a possible patch addressing the coredump.

Thread 1 must be calling nwfilterDriverRemoveDBusMatches(). It does so with nwfilterDriverLock held. In the patch below I am now moving the nwfilterDriverLock(driverState) further up so that the initialization, which seems to either take a long time or is entirely stuck, occurs with the lock held and the shutdown cannot occur at the same time. As Dan pointed out in the BZ comments, this is just one example of an class of problems with libvirt's virStateInitialize and virStateCleanup, and virStateReload - these three functions need to be serialized, otherwise this patch will only be narrowing the window of opportunity for a problem, but not completely eliminating it.

Still, it *is* proper for the nwfilter lock to be acquired earlier as you're doing in this patch.

I don't know that it's necessary to have either the "needDriverLock arg virNWFilterDriverIsWatchingFirewallD or to add "NoLock" to the name. If this is the only caller, I would be just as happy removing the locking from it completely and leaving the name as is (it's highly likely that it will never be called from anywhere else, or that if it is, the new place it's called from will also already have the lock.

So, ACK to the movement of the lock acquisition, and ACK to either adding the needDriverLock arg or just removing the locking from virNWFilterDriverIsWatchingFirewallD completely - the choice is yours.

I'll remove the lock entirely. I'll post v2 then. Stefan

...

To avoid having to make the nwfilterDriverLock lockable multiple times / recursive I changed the virNWFilterDriverIsWatchingFirewallD to take as an argument whether it has to grab that lock. There's only a single caller at the moment that calls this function during initialization. We could remove this lock entirely and maybe append to the name of the function NoLock (?).

--- src/nwfilter/nwfilter_driver.c | 18 +++++++++++++----- src/nwfilter/nwfilter_driver.h | 2 +- src/nwfilter/nwfilter_ebiptables_driver.c | 7 ++++++- 3 files changed, 20 insertions(+), 7 deletions(-)

Index: libvirt/src/nwfilter/nwfilter_driver.c =================================================================== --- libvirt.orig/src/nwfilter/nwfilter_driver.c +++ libvirt/src/nwfilter/nwfilter_driver.c @@ -191,6 +191,8 @@ nwfilterStateInitialize(bool privileged, if (!privileged) return 0;

+ nwfilterDriverLock(driverState); + if (virNWFilterIPAddrMapInit() < 0) goto err_free_driverstate; if (virNWFilterLearnInit() < 0) @@ -203,8 +205,6 @@ nwfilterStateInitialize(bool privileged, if (virNWFilterConfLayerInit(virNWFilterDomainFWUpdateCB) < 0) goto err_techdrivers_shutdown;

- nwfilterDriverLock(driverState); - /* * startup the DBus late so we don't get a reload signal while * initializing @@ -309,21 +309,29 @@ nwfilterStateReload(void) { /** * virNWFilterIsWatchingFirewallD: * + * @needDriverLock: Provide 'true' if this function needs to grab + * the nwfilter driver lock, 'false' otherwise, + * which may be the case during initialization + * * Checks if the nwfilter has the DBus watches for FirewallD installed. * * Returns true if it is watching firewalld, false otherwise */ bool -virNWFilterDriverIsWatchingFirewallD(void) +virNWFilterDriverIsWatchingFirewallD(bool needDriverLock) { bool ret;

if (!driverState) return false;

- nwfilterDriverLock(driverState); + if (needDriverLock) + nwfilterDriverLock(driverState); + ret = driverState->watchingFirewallD; - nwfilterDriverUnlock(driverState); + + if (needDriverLock) + nwfilterDriverUnlock(driverState);

return ret; } Index: libvirt/src/nwfilter/nwfilter_driver.h =================================================================== --- libvirt.orig/src/nwfilter/nwfilter_driver.h +++ libvirt/src/nwfilter/nwfilter_driver.h @@ -33,6 +33,6 @@

int nwfilterRegister(void);

-bool virNWFilterDriverIsWatchingFirewallD(void); +bool virNWFilterDriverIsWatchingFirewallD(bool needDriverLock);

#endif /* __VIR_NWFILTER_DRIVER_H__ */ Index: libvirt/src/nwfilter/nwfilter_ebiptables_driver.c =================================================================== --- libvirt.orig/src/nwfilter/nwfilter_ebiptables_driver.c +++ libvirt/src/nwfilter/nwfilter_ebiptables_driver.c @@ -4191,7 +4191,12 @@ ebiptablesDriverInitWithFirewallD(void) int status; int ret = -1;

- if (!virNWFilterDriverIsWatchingFirewallD()) + /* + * check whether we are watching firewalld + * Since we call this function during initialization we won't need + * to have it get the lock, so we pass 'false'. + */ + if (!virNWFilterDriverIsWatchingFirewallD(false)) return -1;

firewall_cmd_path = virFindFileInPath("firewall-cmd");