According to Jim Meyering on 3/1/2010 10:13 AM:
> Here's a case in which using an assertion appears to be the only
> way to tell clang that "client" really is non-NULL at that point.
> I'm sure clang's analyzers will eventually improve, and hence avoid
> this sort of false positive, so have marked this with a FIXME comment,
> to help ensure we eventually remove this otherwise unnecessary assertion.
Thanks for the extra context; it makes in-line review a breeze.
> @@ -1504,34 +1505,38 @@ static void *qemudWorker(void *data)
> virMutexLock(&server->lock);
> while (((client = qemudPendingJob(server)) == NULL) &&
> !worker->quitRequest) {
> if (virCondWait(&server->job, &server->lock) < 0) {
> virMutexUnlock(&server->lock);
> return NULL;
> }
> }
Indeed, the only way client can be NULL at this point is if
worker->quitRequest is true...
> if (worker->quitRequest) {
> if (client)
> virMutexUnlock(&client->lock);
> virMutexUnlock(&server->lock);
> return NULL;
> }
But that means we exit here.
> worker->processingCall = 1;
> virMutexUnlock(&server->lock);
>
> + /* Tell clang we know what we're doing.
> + FIXME: remove when clang improves. */
> + assert (client);
So this assertion is valid. ACK, if assert() is okay.
On the other hand, perhaps a more invasive rewrite would also work while
also avoiding assert(), by hoisting the worker->quitRequest into the while
loop, something like:
while ((client = qemudPendingJob(server)) == NULL) {
if (worker->quitRequest
|| virCondWait(&server->job, &server->lock) < 0) {
virMutexUnlock(&server->lock);
return NULL;
}
}
if (worker->quitRequest) {
virMutexUnlock(&client->lock);
virMutexUnlock(&server->lock);
return NULL;
}
Should I write that into patch format?
--
Eric Blake eblake@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list