[Asterisk-code-review] loader: Rework load resource list. (asterisk[master])

[Corey Farrell]

Corey Farrell has uploaded this change for review. ( https://gerrit.asterisk.org/7987


Change subject: loader: Rework load_resource_list.
......................................................................

loader: Rework load_resource_list.

Use a single loop in a loop to scan the resource list attempting to
dlopen each module.  The inner loop is repeated until it doesn't do any
work, then it is run one more time to allow printing of error messages.

Change-Id: I60c15cd57ff9680b62e2a94c7519401fa4a38e45
---
M main/loader.c
1 file changed, 30 insertions(+), 63 deletions(-)



  git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/87/7987/1

diff --git a/main/loader.c b/main/loader.c
index 62a97fc..14de8cd 100644
--- a/main/loader.c
+++ b/main/loader.c
@@ -1635,13 +1635,12 @@
 {
 	struct module_vector resource_heap;
 	struct load_order_entry *order;
-	struct load_retries load_retries;
+	int attempt = 0;
 	int count = 0;
 	int res = 0;
-	int i = 0;
+	int didwork;
+	int lasttry = 0;
 #define LOAD_RETRIES 4
-
-	AST_LIST_HEAD_INIT_NOLOCK(&load_retries);
 
 	if (AST_VECTOR_INIT(&resource_heap, 500)) {
 		ast_log(LOG_ERROR, "Failed to initialize module loader.\n");
@@ -1649,91 +1648,59 @@
 		return -1;
 	}
 
-	/* first, add find and add modules to heap */
-	AST_LIST_TRAVERSE_SAFE_BEGIN(load_order, order, entry) {
-		enum ast_module_load_result lres;
+	while (!res) {
+		didwork = 0;
 
-		/* Suppress log messages unless this is the last pass */
-		lres = load_resource(order->resource, 1, &resource_heap, order->required);
-		ast_debug(3, "PASS 0: %-46s %d\n", order->resource, lres);
-		switch (lres) {
-		case AST_MODULE_LOAD_SUCCESS:
-			/* We're supplying a heap so SUCCESS isn't possible but we still have to test for it. */
-			break;
-		case AST_MODULE_LOAD_FAILURE:
-		case AST_MODULE_LOAD_DECLINE:
-			/*
-			 * DECLINE or FAILURE means there was an issue with dlopen or module_register
-			 * which might be retryable.  LOAD_FAILURE only happens for required modules
-			 * but we're still going to retry.  We need to remove the entry from the
-			 * load_order list and add it to the load_retries list.
-			 */
-			AST_LIST_REMOVE_CURRENT(entry);
-			AST_LIST_INSERT_TAIL(&load_retries, order, entry);
-			break;
-		case AST_MODULE_LOAD_SKIP:
-			/*
-			 * SKIP means that dlopen worked but global_symbols was set and this module doesn't qualify.
-			 * Leave it in load_order for the next call of load_resource_list.
-			 */
-			break;
-		case AST_MODULE_LOAD_PRIORITY:
-			/* load_resource worked and the module was added to the priority vector */
-			AST_LIST_REMOVE_CURRENT(entry);
-			ast_free(order->resource);
-			ast_free(order);
-			break;
-		}
-	}
-	AST_LIST_TRAVERSE_SAFE_END;
-
-	/* Retry the failures until the list is empty or we reach LOAD_RETRIES */
-	for (i = 0; !AST_LIST_EMPTY(&load_retries) && i < LOAD_RETRIES; i++) {
-		AST_LIST_TRAVERSE_SAFE_BEGIN(&load_retries, order, entry) {
+		AST_LIST_TRAVERSE_SAFE_BEGIN(load_order, order, entry) {
 			enum ast_module_load_result lres;
 
 			/* Suppress log messages unless this is the last pass */
-			lres = load_resource(order->resource, (i < LOAD_RETRIES - 1), &resource_heap, order->required);
-			ast_debug(3, "PASS %d %-46s %d\n", i + 1, order->resource, lres);
+			lres = load_resource(order->resource, !lasttry, &resource_heap, order->required);
+			ast_debug(3, "PASS %d: %-46s %d\n", attempt, order->resource, lres);
 			switch (lres) {
-			/* These are all retryable. */
 			case AST_MODULE_LOAD_SUCCESS:
+			case AST_MODULE_LOAD_SKIP:
+				/* We're supplying a heap so SUCCESS isn't possible but we still have to test for it.
+				 * SKIP is only used when we try to start a module that is missing dependencies. */
+				break;
 			case AST_MODULE_LOAD_DECLINE:
 				break;
 			case AST_MODULE_LOAD_FAILURE:
 				/* LOAD_FAILURE only happens for required modules */
-				if (i == LOAD_RETRIES - 1) {
-					/* This was the last chance to load a required module*/
+				if (lasttry) {
+					/* This run is just to print errors. */
 					ast_log(LOG_ERROR, "*** Failed to load module %s - Required\n", order->resource);
 					fprintf(stderr, "*** Failed to load module %s - Required\n", order->resource);
 					res =  -2;
-					goto done;
 				}
-				break;;
-			case AST_MODULE_LOAD_SKIP:
-				/*
-				 * SKIP means that dlopen worked but global_symbols was set and this module
-				 * doesn't qualify.  Put it back in load_order for the next call of
-				 * load_resource_list.
-				 */
-				AST_LIST_REMOVE_CURRENT(entry);
-				AST_LIST_INSERT_TAIL(load_order, order, entry);
 				break;
 			case AST_MODULE_LOAD_PRIORITY:
-				/* load_resource worked and the module was added to the priority heap */
+				/* load_resource worked and the module was added to the priority vector */
 				AST_LIST_REMOVE_CURRENT(entry);
 				ast_free(order->resource);
 				ast_free(order);
+				didwork = 1;
 				break;
 			}
 		}
 		AST_LIST_TRAVERSE_SAFE_END;
+
+		if (!didwork) {
+			if (lasttry) {
+				break;
+			}
+			/* We know the next try is going to fail, it's only being performed
+			 * so we can print errors. */
+			lasttry = 1;
+		}
+		attempt++;
 	}
 
-	res = start_resource_list(&resource_heap, &count);
+	if (!res) {
+		res = start_resource_list(&resource_heap, &count);
+	}
 
-done:
-	while ((order = AST_LIST_REMOVE_HEAD(&load_retries, entry))) {
+	while ((order = AST_LIST_REMOVE_HEAD(load_order, entry))) {
 		ast_free(order->resource);
 		ast_free(order);
 	}

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

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I60c15cd57ff9680b62e2a94c7519401fa4a38e45
Gerrit-Change-Number: 7987
Gerrit-PatchSet: 1
Gerrit-Owner: Corey Farrell <git at cfware.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180117/4952eb33/attachment.html>