On Tue, 24 Mar 2020 at 10:54, Yuval Shaia <yuval.shaia...@gmail.com> wrote: > > To protect from the case that users of the protected_qlist are still > using the qlist let's lock before detsroying it. > > Reported-by: Coverity (CID 1421951) > Signed-off-by: Yuval Shaia <yuval.shaia...@gmail.com> > --- > hw/rdma/rdma_utils.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/hw/rdma/rdma_utils.c b/hw/rdma/rdma_utils.c > index 73f279104c..55779cbef6 100644 > --- a/hw/rdma/rdma_utils.c > +++ b/hw/rdma/rdma_utils.c > @@ -63,6 +63,7 @@ void rdma_protected_qlist_init(RdmaProtectedQList *list) > void rdma_protected_qlist_destroy(RdmaProtectedQList *list) > { > if (list->list) { > + qemu_mutex_lock(&list->lock); > qlist_destroy_obj(QOBJECT(list->list)); > qemu_mutex_destroy(&list->lock); > list->list = NULL;
Looking at the code a bit more closely, I don't think that taking the lock here helps. If there really could be another thread that might be calling eg rdma_protected_qlist_append_int64() at the same time that we're calling rdma_protected_qlist_destroy() then it's just as likely that the code calling destroy could finish just before the append starts: in that case the append will crash because the list has been freed and the mutex destroyed. So I think we require that the user of a protected-qlist ensures that there are no more users of it before it is destroyed (which is fairly normal semantics), and the code as it stands is correct (ie coverity false-positive). thanks -- PMM