On Thu, Dec 02, 2010 at 12:42:19PM +0000, Daniel P. Berrange wrote:
<...snip...>
> > -
> > -/* Caller must hold server lock */
> > -static struct qemud_client *qemudPendingJob(struct qemud_server *server)
> > +static void qemudWorker(void *data, void *opaque ATTRIBUTE_UNUSED)
> > {
> > - int i;
> > - for (i = 0 ; i < server->nclients ; i++) {
> > - virMutexLock(&server->clients[i]->lock);
> > - if (server->clients[i]->dx) {
> > - /* Delibrately don't unlock client - caller wants the lock */
> > - return server->clients[i];
> > - }
> > - virMutexUnlock(&server->clients[i]->lock);
> > - }
> > - return NULL;
> > -}
> > + struct qemud_client *client = data;
> > + struct qemud_client_message *msg;
> >
> > -static void *qemudWorker(void *data)
> > -{
> > - struct qemud_worker *worker = data;
> > - struct qemud_server *server = worker->server;
> > + virMutexLock(&client->lock);
>
> It is neccessary to hold the lock on 'server' before obtaining a
> lock on 'client'. The server lock can be released again immediately
> if no longer needed.
I guess the reason is to avoid locking a being-freed client. But
client->refs has been already incremented by 1 right before the client
goes into thread pool, which insures the client against being freed.
<...snip...>
>
> > + if (!server->workerPool) {
> > + VIR_ERROR0(_("Failed to create thread pool"));
> > + virMutexUnlock(&server->lock);
> > + return NULL;
> > }
> >
> > for (;!server->quitEventThread;) {
>
>
> A small change in that we no longer kill off idle worker threads,
Thread pool can be improved to achieve this internally or provide an
interface for callers to force kill off idle worker threads.
> but the improved simplicity of libvirtd code makes this a worthwhile
> tradeoff. So looks good to me aside from the minor locking bug.
>
> Regards,
> Daniel
--
Thanks,
Hu Tao
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list