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

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?