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

[Daniel Veillard]

On Mon, Mar 01, 2010 at 10:27:43AM -0700, Eric Blake wrote:
> 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.

  In general we really try to avoid assert() basically if we
exit that means that the clients loose their access to their VM
so that's a really really bad situation that we really try to avoid.
  Without refactoring assert() in that case would be acceptable
because basically that would mean the compiler generated broken code
and well, that something we don't try to gard against...

> 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?

  But if you can rewrite the loops to avoid the assert and keep clang
happy, I think it's an even better solution,

    thanks !

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/