[libvirt] nwfilter: grab driver lock earlier during init (bz96649)

[Stefan Berger]

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");
>>
>>