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

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.