libvir-list-bounces@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@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list