> On April 10, 2015, 7:05 p.m., rmudgett wrote: > >
I'm posting my next diff here, then I will discard this review then post the same change to gerrit. This way you can look at reviewboard to see the changes between patches. > On April 10, 2015, 7:05 p.m., rmudgett wrote: > > /trunk/main/astobj2.c, lines 492-494 > > <https://reviewboard.asterisk.org/r/4108/diff/5-6/?file=71780#file71780line492> > > > > The comment doesn't make sense. How is destructor_fn supposed to > > access values in the weak proxy? The real object is no longer linked with > > the weakproxy. > > Corey Farrell wrote: > The following is allowed and a reasonable thing to do: > weakproxy->name = ast_strdup(name); > realobj->name = weakproxy->name; > > The idea is that weakproxy has to own storage of any shared fields since > realobj could be destroyed at any time. So if the destructor uses > realobj->name, it is accessing memory that is owned by the weakproxy. While addressing the other finding about the ref count games, I realized this comment no longer applies. ao2_ref(user_data, -1) happens inside the lock of weakproxy, and ao2_ref(weakproxy, -1) must happen after it's unlocked. Originally it wasn't clear without a comment that the weakproxy release couldn't be moved higher in internal_ao2_ref. > On April 10, 2015, 7:05 p.m., rmudgett wrote: > > /trunk/main/astobj2.c, lines 455-463 > > <https://reviewboard.asterisk.org/r/4108/diff/5-6/?file=71780#file71780line455> > > > > Rather than play games with the real object's ref count. How about > > replacing the indicated code with this: > > ---------- > > /* Unlink the obj from the weak proxy */ > > internal_weakproxy->priv_data.weakptr = NULL; > > obj->priv_data.weakptr = NULL; > > > > /* Notify the subscribers that obj is about to die. */ > > weakproxy_run_callbacks(weakproxy); > > > > /* > > * Weak is already unlinked from obj so this won't recurse. > > * This will destroy obj. > > */ > > ao2_ref(user_data, -1); > > ---------- > > > > The test to release the weakproxy ref below needs to be adjusted to: > > if (current_value == 1) > > and that code moved to just after the unlock of weakproxy. Retested, although the last two reference lines are out of order this is tolerated by refcounter.py. - Corey ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4108/#review15185 ----------------------------------------------------------- On April 3, 2015, 12:58 p.m., Corey Farrell wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/4108/ > ----------------------------------------------------------- > > (Updated April 3, 2015, 12:58 p.m.) > > > Review request for Asterisk Developers, George Joseph and rmudgett. > > > Bugs: ASTERISK-24936 > https://issues.asterisk.org/jira/browse/ASTERISK-24936 > > > Repository: Asterisk > > > Description > ------- > > This implements "weak" references. The weakproxy object is a real ao2 with > normal reference counting of its own. When a weakproxy is pointed to a > normal object they hold references to each other. The normal object is > automatically freed when a single reference remains (the weakproxy). The > weakproxy also supports subscriptions that will notify callbacks when it does > not point to any real object. > > > Diffs > ----- > > /trunk/tests/test_astobj2_weaken.c PRE-CREATION > /trunk/main/astobj2.c 433964 > /trunk/include/asterisk/astobj2.h 433964 > > Diff: https://reviewboard.asterisk.org/r/4108/diff/ > > > Testing > ------- > > Ran the included test with REF_DEBUG enabled under valgrind. No reference > leaks or improper memory access. Though this does not test for races, I > don't know of an automated way to do that. > > > Thanks, > > Corey Farrell > >
-- _____________________________________________________________________ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
