[libvirt] [PATCH v2] nwfilter: extend nwfilter reload support

[Stefan Berger]

libvir-list-bounces at redhat.com wrote on 08/13/2010 10:44:51 AM:

> Please respond to "Daniel P. Berrange"
> 
> On Thu, Aug 12, 2010 at 02:18:26PM -0400, Stefan Berger wrote:
> > 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,25 @@ 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();
> > +
> > +        nwfilterDriverLock(driverState);
> > +        virNWFilterPoolLoadAllConfigs(conn,
> > + &driverState->pools,
> > +                                      driverState->configDir);
> > +        nwfilterDriverUnlock(driverState);
> > +
> > +        virConnectClose(conn);
> > +    }
> > 
> >      return 0;
> 
> There's a small indentation issue here

Oh, will fix it.

> 
> 
> >  }
> > Index: libvirt-acl/src/nwfilter/nwfilter_learnipaddr.c
> > ===================================================================
> > --- libvirt-acl.orig/src/nwfilter/nwfilter_learnipaddr.c
> > +++ libvirt-acl/src/nwfilter/nwfilter_learnipaddr.c
> > @@ -855,6 +855,16 @@ virNWFilterLearnInit(void) {
> >  }
> > 
> > 
> > +void
> > +virNWFilterLearnThreadsTerminate() {
> > +    threadsTerminate = true;
> > +
> > +    while (virHashSize(pendingLearnReq) != 0)
> > +        usleep((PKT_TIMEOUT_MS * 1000) / 3);
> > +
> > +    threadsTerminate = false;
> > +}
> 
> Is there any risk of thread's failing to terminate, requiring
> us to kill them, or ignore them instead of blocking forever ?

A thread may just have been created with pthread_create() but hasn't run 
yet to see that threadTerminate is 'true' -> in that case the 
virHashSize() would count the thread until the thread itself has 
deregistered itself from the pendingLearnReq list. So that case should be 
ok.

Assume a thread is just about to be created (due to a VM start for 
example) and registered with the call to rc = 
virNWFilterRegisterLearnReq(req) preceding the pthread_create(), in that 
case a 'threadsTerminate = false' would allow that thread to run then. So 
I guess the 'threadsTerminate = false' is wrong in that case and would 
prevent the thread from terminating. The challenge is to do this correctly 
for reloading of the driver and at the same time for shutting down. If I 
take out the 'threadsTerminate = false', all threads and those about to be 
born should terminate, which does it correctly for the libvirtd 
termination case. But in case of reload where I would want new threads to 
run again I should probably introduce a boolean parameter indicating into 
what state the threadsTerminate variable should go once this above 
function terminates.

What that I would do the following changes:

void
virNWFilterLearnThreadsTerminate(bool allowNewThreads) {
    threadsTerminate = true;

    while (virHashSize(pendingLearnReq) != 0)
        usleep((PKT_TIMEOUT_MS * 1000) / 3);

    if (allowNewThreads)
        threadsTerminate = false;
}


Above call (with the indentation error) would then look like this:

[...]
    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);
    }
[...]

Does this sound right?

   Stefan

> 
> 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 :|
> 
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20100813/680e9f0a/attachment-0001.htm>