> On July 21, 2014, 5:49 p.m., Mark Michelson wrote: > > /trunk/main/loader.c, lines 1001-1005 > > <https://reviewboard.asterisk.org/r/3777/diff/2/?file=64886#file64886line1001> > > > > Feel free to cringe at this suggestion, but since you've created both > > an AST_DLLIST_ENTRY and an AST_LIST_ENTRY on the ast_module structure, you > > could maintain two parallel lists of modules. You'd have the module_list, > > that is important for module loading and unloading, and you could have the > > alphabetical list that is used by the CLI command. > > > > At the time that you add the module to the module_list (what I've > > highlighted), you can also add it to the alphabetical list. That way, you > > would not have to lock the module_list and rebuild the alphabetical list > > each time ast_update_module_list() is called. Up to you really. > > Matt Jordan wrote: > /me cringes > > So, when I first started at this work, I approached it from the angle of > ordering the module_list during start up. I had a vector of modules lists, > where the vector had slots based on the various possible module priorities. > That was a mistake. > > Let me tell you: module loading is some strange joo-joo. Things get > opened, "loaded", and discarded more times than you would believe. By the > time *something* gets around calling load_module, that particular 'module' > has probably been in and out of memory (and possibly the modules list) a > number of times. And don't even get me started on embedded modules. That's a > whole other ball of string to untangle. > > I fully admit that this is the "poor man's" fix for the module loader - > ideally we'd have an actual dependency tree, and other niceties. I'm hesitant > to make too many changes in here - and having two module lists feels... > dangerous.
Agreed, I think complicating things more in that area with trying to maintain two lists is asking for trouble. I think doing a temporary list is a nice compromise for the few times that the list is actually shown. It doesn't have to be performant. - Joshua ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3777/#review12789 ----------------------------------------------------------- On July 21, 2014, 4:58 p.m., Matt Jordan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/3777/ > ----------------------------------------------------------- > > (Updated July 21, 2014, 4:58 p.m.) > > > Review request for Asterisk Developers. > > > Repository: Asterisk > > > Description > ------- > > When Asterisk starts a module (calling its load_module function), it > re-orders the module list, sorting it alphabetically. Ostensibly, this was > done so that the output of 'module show' listed modules in alphabetic order. > This had the unfortunate side effect of making modules with complex usage > patterns practically unloadable. A module that has a large number of modules > that depend on it is typically abandoned during the unloading process. This > results in its memory not being reclaimed during exit. > > Generally, this isn't harmful - when the process is destroyed, the operating > system will reclaim all memory allocated by the process. However, it makes > tracking memory leaks or ref debug leaks a real pain. > > While this patch is not a complete overhaul of the module loader - such an > effort would be beyond the scope of what could be done for Asterisk 13 - this > does make some marginal improvements to the loader such that modules like > res_pjsip or res_stasis *may* be made properly un-loadable in the future. > 1. The linked list of modules has been replaced with a doubly linked list. > This allows traversal of the module list to occur backwards. The module > shutdown routine now walks the global list backwards when it attempts to > unload modules. > 2. The alphabetic reorganization of the module list on startup has been > removed. Instead, a started module is placed at the end of the module list. > 3. The ast_update_module_list function - which is used by the CLI to display > the modules - now does the sorting alphabetically itself. It creates its own > linked list and inserts the modules into it in alphabetic order. This allows > for the intent of the previous code to be maintained. > > This patch also contains a fix for res_calendar. Without calendar.conf, the > calendar modules were improperly bumping the use count of res_calendar, then > failing to load themselves. This patch makes it so that we detect whether or > not calendaring is enabled before altering the use count. > > > Diffs > ----- > > /trunk/res/res_calendar.c 419109 > /trunk/main/loader.c 419109 > > Diff: https://reviewboard.asterisk.org/r/3777/diff/ > > > Testing > ------- > > Verified that the CLI command worked appropriately. > > Verified that module loading/unloading of res_calendar (whose calendar > modules modify the res_calendar use count) loaded/unloaded properly: > > Asterisk Dynamic Loader Starting: > [Jul 14 15:14:52] NOTICE[11877]: loader.c:1317 load_modules: 6 modules will > be loaded. > Loading res_calendar.so. > Loading res_calendar_icalendar.so. > Loading res_calendar_exchange.so. > Loading res_calendar_caldav.so. > Loading res_calendar_ews.so. > Asterisk Ready. > *CLI> core stop gracefully > Waiting for inactivity to perform halt... > Unloading res_calendar_ews.so > Unloading res_calendar_caldav.so > Unloading res_calendar_exchange.so > Unloading res_calendar_icalendar.so > Unloading res_calendar.so > Asterisk cleanly ending (0). > Executing last minute cleanups > > Verified that using autoload with all modules, module are started as they > were previously, and now are stopped/unloaded in the reverse order. > > > Thanks, > > Matt Jordan > >
-- _____________________________________________________________________ -- 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
