On Tue, Mar 18, 2014 at 09:45:14PM -0400, Stefan Berger wrote: > From: Stefan Berger <stefanb@xxxxxxxxxxxxxxxxxx> > > https://bugzilla.redhat.com/show_bug.cgi?id=1071181 > > 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@xxxxxxxxxxxxxxxxxx> > --- > 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. Martin > /* > * delete the request: > * - if we don't find req on the global list anymore > @@ -1605,6 +1608,7 @@ virNWFilterDHCPSnoopReq(virNWFilterTechDriverPtr techdriver, > int tmp; > virThread thread; > virNWFilterVarValuePtr dhcpsrvrs; > + bool threadPuts = false; > > virNWFilterSnoopIFKeyFMT(ifkey, vmuuid, macaddr); > > @@ -1690,6 +1694,8 @@ virNWFilterDHCPSnoopReq(virNWFilterTechDriverPtr techdriver, > /* prevent thread from holding req */ > virNWFilterSnoopReqLock(req); > > + threadPuts = true; > + > if (virThreadCreate(&thread, false, virNWFilterDHCPSnoopThread, > req) != 0) { > virReportError(VIR_ERR_INTERNAL_ERROR, > @@ -1737,7 +1743,8 @@ exit_rem_ifnametokey: > exit_snoopunlock: > virNWFilterSnoopUnlock(); > exit_snoopreqput: > - virNWFilterSnoopReqPut(req); > + if (!threadPuts) > + virNWFilterSnoopReqPut(req); > > return -1; > } > -- > 1.8.1.4 > > -- > libvir-list mailing list > libvir-list@xxxxxxxxxx > https://www.redhat.com/mailman/listinfo/libvir-list
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list