Re: nwfilter: grab driver lock earlier during init (bz96649)

[Stefan Berger <stefanb@xxxxxxxxxxxxxxxxxx>]

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

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list