[libvirt] nwfilter: grab driver lock earlier during init (bz96649)
Stefan Berger
stefanb at linux.vnet.ibm.com
Mon Jun 3 19:57:28 UTC 2013
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");
>>
>>
More information about the libvir-list
mailing list