[libvirt] [PATCH] add an assert, to avoid a false-positive NULL-deref warning from clang

[Eric Blake]

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 at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 320 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20100301/0ee30005/attachment-0001.sig>