[libvirt] [PATCH v4 RESEND 1/4] threadpool impl

[Daniel P. Berrange]

On Fri, Dec 03, 2010 at 09:46:44AM +0800, Hu Tao wrote:
> On Thu, Dec 02, 2010 at 12:28:17PM +0000, Daniel P. Berrange wrote:
> > On Thu, Dec 02, 2010 at 03:26:57PM +0800, Hu Tao wrote:
> > > +virThreadPoolPtr virThreadPoolNew(size_t minWorkers,
> > > +                                  size_t maxWorkers,
> > > +                                  virThreadPoolJobFunc func,
> > > +                                  void *opaque)
> > > +{
> > > +    virThreadPoolPtr pool;
> > > +    size_t i;
> > > +
> > > +    if (minWorkers > maxWorkers)
> > > +        minWorkers = maxWorkers;
> > > +
> > > +    if (VIR_ALLOC(pool) < 0) {
> > > +        virReportOOMError();
> > > +        return NULL;
> > > +    }
> > > +
> > > +    pool->jobList.head = NULL;
> > > +    pool->jobList.tail = &pool->jobList.head;
> > > +
> > > +    pool->jobFunc = func;
> > > +    pool->jobOpaque = opaque;
> > > +
> > > +    if (virMutexInit(&pool->mutex) < 0)
> > > +        goto error;
> > > +    if (virCondInit(&pool->cond) < 0)
> > > +        goto error;
> > > +    if (virCondInit(&pool->quit_cond) < 0)
> > > +        goto error;
> > > +
> > > +    if (VIR_ALLOC_N(pool->workers, minWorkers) < 0)
> > > +        goto error;
> > > +
> > > +    pool->maxWorkers = maxWorkers;
> > > +    for (i = 0; i < minWorkers; i++) {
> > > +        if (virThreadCreate(&pool->workers[i],
> > > +                            true,
> > > +                            virThreadPoolWorker,
> > > +                            pool) < 0) {
> > > +            virThreadPoolFree(pool);
> > > +            return NULL;
> > > +        }
> > > +        pool->nWorkers++;
> > > +    }
> > > +
> > > +    return pool;
> > > +
> > > +error:
> > > +    VIR_FREE(pool->workers);
> > > +    ignore_value(virCondDestroy(&pool->quit_cond));
> > > +    ignore_value(virCondDestroy(&pool->cond));
> > > +    virMutexDestroy(&pool->mutex);
> > > +    return NULL;
> > > +}
> > 
> > This is leaking 'pool' on error. IMHO it is preferrable to
> > just call virThreadPoolDestroy, otherwise anytime we add
> > another field to virThreadPoolPtr struct, we have to consider
> > updating 2 cleanup paths.
> 
> Agree. Since it is in error clean path (thus thread pool is not yet
> created) it doesn't matter to lock on a broken mutex or wait on a
> broken cond.

IMHO it doesn't matter if we call virMutexDestroy on an uninitialized
mutex, not least because this code is already calling viCondDestroy
on potentially uninitialized conditions. We'll just get an error
code back from the destroy function which is ignored anyway. And the
failure to initialize a mutex is something I've never encountered in
practice.

Regards,
Daniel