Re: [libvirt] [PATCH 01/10] util: Introduce thread queues

On 05/22/2015 10:31 AM, Jiri Denemark wrote: > On Fri, May 22, 2015 at 10:16:26 -0400, John Ferlan wrote: >> >> >> On 05/21/2015 06:42 PM, Jiri Denemark wrote: >>> Our usage of pthread conditions does not allow a single thread to wait >>> for several events from different sources. This is because the condition >>> is bound to the source of the event. We can invert the usage by giving >>> each thread its own condition and providing APIs for registering this >>> thread condition with several sources. Each of the sources can then >>> signal the thread condition. >>> >>> Thread queues also support several threads to be registered with a >>> single event source, which can either wakeup all waiting threads or just >>> the first one. >>> >>> Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; >>> --- >>> po/POTFILES.in | 1 + >>> src/Makefile.am | 2 + >>> src/libvirt_private.syms | 15 ++ >>> src/util/virthreadqueue.c | 343 ++++++++++++++++++++++++++++++++++++++++++++++ >>> src/util/virthreadqueue.h | 54 ++++++++ >>> 5 files changed, 415 insertions(+) >>> create mode 100644 src/util/virthreadqueue.c >>> create mode 100644 src/util/virthreadqueue.h >>> >> >> Ran the series through Coverity and came back with this gem >> >>> + >>> +void >>> +virThreadQueueFree(virThreadQueuePtr queue) >>> +{ >>> + if (!queue) >>> + return; >>> + >>> + while (queue->head) >>> + virThreadQueueRemove(queue, queue->head); >> >> (3) Event cond_true: Condition "queue->head", taking true branch >> (6) Event loop_begin: Jumped back to beginning of loop >> (7) Event cond_true: Condition "queue->head", taking true branch >> >> 283 while (queue->head) >> >> (4) Event freed_arg: "virThreadQueueRemove" frees "queue->head". [details] >> (5) Event loop: Jumping back to the beginning of the loop >> (8) Event deref_arg: Calling "virThreadQueueRemove" dereferences freed pointer "queue->head". [details] >> >> 284 virThreadQueueRemove(queue, queue->head); > > False positive. If virThreadQueueRemove frees queue->head, it also > updates it with queue->head->next. > I understand and looked at the code, but I think this is a case where pass by reference and pass by value makes a difference. Upon return what causes queue->head to be evaluated again? The call passes queue->head by value and removes it from queue. Upon return nothing causes queue->head to be evaluated again thus wouldn't we enter the loop the second time with the same address?