[Asterisk-code-review] loader: Miscellaneous fixes. (asterisk[13])

[Richard Mudgett]

Richard Mudgett has posted comments on this change. ( https://gerrit.asterisk.org/7985 )

Change subject: loader: Miscellaneous fixes.
......................................................................


Patch Set 1: Code-Review-1

(2 comments)

https://gerrit.asterisk.org/#/c/7985/1/main/loader.c
File main/loader.c:

https://gerrit.asterisk.org/#/c/7985/1/main/loader.c@678
PS1, Line 678: 	} while (somethingchanged && !final);
We are currently staying in the loop only while things are changing.  The "final" variable is meaningless because it can never get set.  It has been this way since the original code was added by commit c0b3c923a46afc12837698a78a11b17a7fc384d8.  The "final" variable and code controlled by it can be removed as dead code.

Not forcing the unload of modules with a use count appears to be intensional since the return value is true if the module list is not empty.  The return value is used by the caller to switch to the fast shutdown mode (Commit f373de302032c13487cfcaa616fc070f10d68b57).

Should fix this in all branches.

Remove final and update the code as if final=0.


https://gerrit.asterisk.org/#/c/7985/1/main/loader.c@1433
PS1, Line 1433: 	for (v = ast_variable_browse(cfg, "modules"); v; v = v->next) {
              : 		if (find_resource(v->value, 0)) {
              : 			continue;
              : 		}
              : 
              : 		if (!strcasecmp(v->name, preload_only ? "preload" : "load")) {
              : 			add_to_load_order(v->value, &load_order, 0);
              : 		}
              : 
              : 		if (!strcasecmp(v->name, preload_only ? "preload-require" : "require")) {
              : 			/* Add the module to the list and make sure it's required */
              : 			add_to_load_order(v->value, &load_order, 1);
              : 			ast_debug(2, "Adding module to required list: %s (%s)\n", v->value, v->name);
              : 		}
              : 	}
Remember find_resource() searches a list so it isn't exactly cheap.

for (v = ast_variable_browse(cfg, "modules"); v; v = v->next) {
    if (!strcasecmp(v->name, preload_only ? "preload" : "load")) {
        if (!preload_only && find_resource(v->value, 0)) {
            /* Preload already loaded it */
            continue;
        }
        add_to_load_order(v->value, &load_order, 0);
    } else if (!strcasecmp(v->name, preload_only ? "preload-require" : "require")) {
        if (!preload_only && find_resource(v->value, 0)) {
            /* Preload already loaded it */
            continue;
        }
        /* Add the module to the list and make sure it's required */
        add_to_load_order(v->value, &load_order, 1);
        ast_debug(2, "Adding module to required list: %s (%s)\n", v->value, v->name);
    }
}



-- 
To view, visit https://gerrit.asterisk.org/7985
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-MessageType: comment
Gerrit-Change-Id: I22261599c46d0f416e568910ec9502f45143197f
Gerrit-Change-Number: 7985
Gerrit-PatchSet: 1
Gerrit-Owner: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Comment-Date: Wed, 17 Jan 2018 16:19:21 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180117/39e517db/attachment.html>