----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3184/#review10799 -----------------------------------------------------------
branches/12/include/asterisk/sorcery.h <https://reviewboard.asterisk.org/r/3184/#comment20345> Spacing const char *module branches/12/main/sorcery.c <https://reviewboard.asterisk.org/r/3184/#comment20342> These should be set NULL after cleanup so the pointer is not dangling on destroyed data. branches/12/main/sorcery.c <https://reviewboard.asterisk.org/r/3184/#comment20343> instance = alloc(); if (!instance) { } The above is a better format because there are more and better line break opportunities available with the stand alone assignment statement than when combining the assignment into the if test. Also you don't need the error message here because the alloc has already given a memory allocation failure error message. branches/12/main/sorcery.c <https://reviewboard.asterisk.org/r/3184/#comment20346> It is best to have a blank line between declarations and code. branches/12/main/sorcery.c <https://reviewboard.asterisk.org/r/3184/#comment20347> spacing const char *module_name branches/12/main/sorcery.c <https://reviewboard.asterisk.org/r/3184/#comment20350> Use ao2_wrlock() instead since the instances container has a rwlock. ao2_lock() is a synonym for ao2_wrlock() when an object has a rwlock. branches/12/main/sorcery.c <https://reviewboard.asterisk.org/r/3184/#comment20349> Use OBJ_SEARCH_KEY not OBJ_KEY. branches/12/main/sorcery.c <https://reviewboard.asterisk.org/r/3184/#comment20351> OBJ_SEARCH_KEY branches/12/main/sorcery.c <https://reviewboard.asterisk.org/r/3184/#comment20353> There are two red flags here: 1) You are using sorcery after you have released your ref to it. 2) Any time you are looking at the number of references left, there is a design problem. In this case, only the owner of the sorcery object should close/destroy it which will remove it from the instances container. Only the owner of the sorcery object should open/create it which will add it to the instances container. Everyone else will search the instances container for the object to get a reference. branches/12/main/sorcery.c <https://reviewboard.asterisk.org/r/3184/#comment20355> This ao2_find is really an ao2_unlink(). - rmudgett On Feb. 6, 2014, 2:07 p.m., George Joseph wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/3184/ > ----------------------------------------------------------- > > (Updated Feb. 6, 2014, 2:07 p.m.) > > > Review request for Asterisk Developers. > > > Bugs: ASTERISK-22537 > https://issues.asterisk.org/jira/browse/ASTERISK-22537 > > > Repository: Asterisk > > > Description > ------- > > Create sorcery instance registry as a precursor to creating a generic > dialplan function that can retrieve parameters from a sorcery-based config > file. > > ast_sorcery_init now creates a hashtab as a registry. > ast_sorcery_open now checks the hashtab for an existing sorcery instance > matching the caller's module name. If it finds one, it bumps the refcount > and returns it. If not, it creates a new sorcery instance, adds it to the > hashtab, then returns it. > ast_sorcery_retrieve_by_module_name is a new function that does a hashtab > lookup by module name. It can be called by the future dialplan function. > > A side effect of this patch is that a module can only have 1 sorcery instance > (because it's the key for the hashtab). res_pjsip/config_system needed a > small change to share the main res_pjsip sorcery instance. > > > Diffs > ----- > > branches/12/tests/test_sorcery.c 407566 > branches/12/res/res_pjsip/include/res_pjsip_private.h 407566 > branches/12/res/res_pjsip/config_system.c 407566 > branches/12/res/res_pjsip.c 407566 > branches/12/main/sorcery.c 407566 > branches/12/include/asterisk/sorcery.h 407566 > > Diff: https://reviewboard.asterisk.org/r/3184/diff/ > > > Testing > ------- > > Made sure that users of sorcery (mostly res_pjsip) continued to load their > configs correctly. > Made sure there were no ill effects on res_pjsip from config_system sharing > the same sorcery instance as the rest of the pjsip infrastructure. > Made sure that config_system was properly marked as 'not reloadable' and that > it was maintaining it's original values when res_pjsip was reloaded. > > > ernie*CLI> test execute category /main/sorcery/ > Running all available tests matching category /main/sorcery/ > > START /main/sorcery/ - configuration_file_wizard_retrieve_multiple_all > END /main/sorcery/ - configuration_file_wizard_retrieve_multiple_all Time: > <1ms Result: PASS > [snip] > START /main/sorcery/ - open > END /main/sorcery/ - open Time: <1ms Result: PASS > START /main/sorcery/ - wizard_registration > END /main/sorcery/ - wizard_registration Time: <1ms Result: PASS > > 43 Test(s) Executed 43 Passed 0 Failed > > > 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
