----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4608/#review15164 -----------------------------------------------------------
I think I still prefer ao2_callback over ao2_iterator for taking an action against every object in a container, but don't feel strongly enough to hold this up. Minor nit with the return value, then we can ship this. branches/13/res/res_pjsip_phoneprov_provider.c <https://reviewboard.asterisk.org/r/4608/#comment25828> If we're sticking with the ao2_iterator lets loose the 'int' return, just make this function void. branches/13/res/res_pjsip_phoneprov_provider.c <https://reviewboard.asterisk.org/r/4608/#comment25827> Remove.. branches/13/res/res_pjsip_phoneprov_provider.c <https://reviewboard.asterisk.org/r/4608/#comment25826> To record our brief discussion about this change: <coreyfarrell> gtjoseph: so one thing I don't see how using an ao2_iterator instead of ao2_callback in users_apply_handler makes it more straight forward.. <coreyfarrell> I don't want to hold up the fix, but that change doesn't make much sense to me <gtjoseph> there's no worry about the callback return value, OBJ_NODATA, etc. Get the container and iterate over it. It also eliminates a lot of overhead in the ao2_traversal code. <gtjoseph> unless the callback is controlling the traversal with it's return code, there's no point. - Corey Farrell On April 9, 2015, 3:20 p.m., George Joseph wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/4608/ > ----------------------------------------------------------- > > (Updated April 9, 2015, 3:20 p.m.) > > > Review request for Asterisk Developers and Corey Farrell. > > > Bugs: ASTERISK-24935 > https://issues.asterisk.org/jira/browse/ASTERISK-24935 > > > Repository: Asterisk > > > Description > ------- > > res_pjsip_phoneprov_provider was leaking references to phoneprov objects due > to a missing OBJ_NODATA in an ao2_callback in load_users(). Rather than > adding the OBJ_NODATA, I changed load_users to use a more straightforward > ao2_iterator. This plugged the leak but exposed an unload order issue > between res_pjsip_phoneprov_provider, res_phoneprov and res_pjsip. > > res_pjsip_phoneprov_provider unloads first, then res_phoneprov, then > res_pjsip. Since res_pjsip_phoneprov_provider uses res_pjsip's sorcery > instance, when it unloads, it's objects are still in the sorcery instance. > When res_pjsip unloads, it destroys all its objects including > res_pjsip_phoneprov_provider's. The phoneprov destructor then attempts to > unregister the extension from res_phoneprov but because res_phoneprov is > already cleaned up, its users container is gone and we get a FRACK. > > Simple solution, check for the NULL users container before attempting to > remove the entry. Duh. > > > Diffs > ----- > > branches/13/res/res_pjsip_phoneprov_provider.c 434447 > branches/13/res/res_phoneprov.c 434447 > > Diff: https://reviewboard.asterisk.org/r/4608/diff/ > > > Testing > ------- > > Ran tests/res_phoneprov/res_phoneprov_provider. No leaks in > res_pjsip_phoneprov_provider and no FRACKs. > > > Thanks, > > George Joseph > >
-- _____________________________________________________________________ -- 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