<tt>Martin Kletzander <mkletzan@redhat.com> wrote on 03/19/2014 09:13:51 AM: > > On Tue, Mar 18, 2014 at 09:45:14PM -0400, Stefan Berger wrote: > > From: Stefan Berger <stefanb@linux.vnet.ibm.com> > > > > </tt><a href="https://bugzilla.redhat.com/show_bug.cgi?id=1071181"><tt>https://bugzilla.redhat.com/show_bug.cgi?id=1071181</tt></a><tt> > > > > Commit 49b59a15 fixed one problem but masks another one related to pointer > > freeing. > > > > Use virAtomicIntGet() to test for 0 rather than trying to test for 'true' > > after virAtomicIntDecAndTest(). > > > > Avoid putting of the virNWFilterSnoopReq once the thread has been started. > > > > Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> > > --- > > src/nwfilter/nwfilter_dhcpsnoop.c | 11 +++++++++-- > > 1 file changed, 9 insertions(+), 2 deletions(-) > > > > diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c b/src/nwfilter/ > nwfilter_dhcpsnoop.c > > index d2a8062..32ca304 100644 > > --- a/src/nwfilter/nwfilter_dhcpsnoop.c > > +++ b/src/nwfilter/nwfilter_dhcpsnoop.c > > @@ -720,7 +720,10 @@ virNWFilterSnoopReqPut(virNWFilterSnoopReqPtr req) > > > > virNWFilterSnoopLock(); > > > > - if (virAtomicIntDecAndTest(&req->refctr)) { > > + virAtomicIntDecAndTest(&req->refctr); > > + > > + /* make sure it's 0; virAtomitIntDecAndTest may return true on '1' */ > > + if (virAtomicIntGet(&req->refctr) == 0) { > > NACK, using two atomic functions, you are making this non-atomic. > between these two atomic operations many things can happen an it's not > what you want I bet. The virAtomicIntDecAndTest() uses > __sync_fetch_and_sub(atomic, 1) and compares it to 1, that's true, but > since it is fetch_and_sub (and not the other way around, the value > being compared to 1 is the value that the atomic had before it was > decremented. That means it returns 1 (true) if and only if the > current value is 0. virAtomicIntDecAndTest() is what you really want > and should use here.</tt> <tt>Right. I initially thought there was something wrong in this part here until I found the double free error is related to the other part. So this part you comment on doesn't need to be changed.</tt> <tt> Stefan</tt>