2:11 p.m.
libvir-list-bounces(a)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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list