On Wed, Dec 01, 2010 at 05:32:44PM +0800, Hu Tao wrote:
Hi Eric,
Thanks for your careful review of these patches. I'll post v4 patches tomorrow fixing all problems you pointed out.
daemon/libvirtd.c already has a notion of worker threads; I'm wondering how much overlap there is between your implementation and that one. A better proof that this would be a useful API addition would be to have the next patch in the series convert libvirtd.c over to using this API.
OK. Will be in v4.
<...snip...>
+int virWorkerPoolSetMaxWorker(struct virWorkerPool *pool, int maxWorker) +{ + if (maxWorker < 0) + return -1; + + pthread_mutex_lock(&pool->mutex); + pool->nMaxWorker = maxWorker; + pthread_mutex_unlock(&pool->mutex);
Does this do the right thing if maxWorker < pool->nMaxWorker, or does it silently lose existing workers?
In the case maxWorker < pool->nMaxWorker and there are pool->nMaxWorker threads running, (pool->nMaxWorker - maxWorker) threads will exit after the new nMaxWorker set.
<...snip...>
+ +typedef void (*virWorkerFunc)(void *);
pthread_create() takes a function that can return void*. Should worker functions be allowed to return a value?
threadpool doesn't care the return value, neither it has no way to pass the return value to threadpool creator, so it's meaningless for worker functions to return a value.
Another example is virThreadFunc which does't return a value neither.
I've needed a thread pool implementation for an unrelated piece of work I'm doing on libvirt. I took your impl here, and updated it to follow libvirt naming style, use appropriate internals APIs, and hide the struct definitions from the header. Take a look at the files attached. Regards, Daniel