[Asterisk-code-review] loader: Improve error handling. (asterisk[master])

Corey Farrell asteriskteam at digium.com
Fri Sep 28 10:28:32 CDT 2018


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


Change subject: loader: Improve error handling.
......................................................................

loader: Improve error handling.

* Display list of unavailable dependencies when they cause another
  module to fail loading.
* When a module declines to load find all modules which depend on it so
  they can be declined and listed together.
* Prevent retry of declined modules during startup.

These changes are meant to reduce logger spam that were caused when a
module has many dependencies and declines to load.  This also fixes some
error paths which failed to recognize required modules.

Change-Id: I046052c71331c556c09d39f47a3b92975f3e1758
---
M main/loader.c
1 file changed, 133 insertions(+), 15 deletions(-)



  git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/15/10315/1

diff --git a/main/loader.c b/main/loader.c
index eb345b5..b3f1671 100644
--- a/main/loader.c
+++ b/main/loader.c
@@ -139,6 +139,13 @@
 
 AST_VECTOR(module_vector, struct ast_module *);
 
+/*! Used with AST_VECTOR_CALLBACK_VOID to create a
+ * comma separated list of module names for error messages. */
+#define STR_APPEND_TEXT(txt, str) \
+	ast_str_append(&str, 0, "%s%s", \
+		ast_str_strlen(str) > 0 ? ", " : "", \
+		txt)
+
 /* Built-in module registrations need special handling at startup */
 static unsigned int loader_ready;
 
@@ -1656,22 +1663,96 @@
 	if (lres == AST_MODULE_LOAD_SUCCESS) {
 		(*count)++;
 	} else if (lres == AST_MODULE_LOAD_FAILURE) {
-		ast_log(LOG_ERROR, "*** Failed to load module %s\n", mod->resource);
+		ast_log(LOG_ERROR, "*** Failed to load %smodule %s\n",
+			mod->flags.required ? "required " : "",
+			mod->resource);
 	}
 
 	return lres;
 }
 
+static int resource_list_recursive_decline(struct module_vector *resources, struct ast_module *mod,
+	struct ast_str **printmissing_ptr)
+{
+	struct module_vector missingdeps;
+	struct ast_vector_const_string localdeps;
+	struct ast_str *printmissing = *printmissing_ptr;
+	int i = 0;
+	int res = -1;
+
+	mod->flags.declined = 1;
+	if (mod->flags.required) {
+		ast_log(LOG_ERROR, "Required module %s declined to load.\n", ast_module_name(mod));
+
+		return -2;
+	}
+
+	ast_log(LOG_WARNING, "%s declined to load.\n", ast_module_name(mod));
+
+	if (!printmissing) {
+		printmissing = ast_str_create(64);
+		if (!printmissing) {
+			return -1;
+		}
+	} else {
+		ast_str_reset(printmissing);
+	}
+
+	AST_VECTOR_INIT(&missingdeps, 0);
+	AST_VECTOR_INIT(&localdeps, 0);
+
+	/* Decline everything that depends on 'mod' from resources so we can
+	 * print a concise list. */
+	while (res != -2 && i < AST_VECTOR_SIZE(resources)) {
+		struct ast_module *dep = AST_VECTOR_GET(resources, i);
+		i++;
+
+		AST_VECTOR_RESET(&missingdeps, AST_VECTOR_ELEM_CLEANUP_NOOP);
+		if (dep->flags.declined || module_deps_missing_recursive(dep, &missingdeps)) {
+			continue;
+		}
+
+		if (AST_VECTOR_GET_CMP(&missingdeps, mod, AST_VECTOR_ELEM_DEFAULT_CMP)) {
+			dep->flags.declined = 1;
+			if (dep->flags.required) {
+				ast_log(LOG_ERROR, "Cannot load required module %s that depends on %s\n",
+					ast_module_name(dep), ast_module_name(mod));
+				res = -2;
+			} else {
+				AST_VECTOR_APPEND(&localdeps, ast_module_name(dep));
+			}
+		}
+	}
+	AST_VECTOR_FREE(&missingdeps);
+
+	if (res != -2 && AST_VECTOR_SIZE(&localdeps)) {
+		AST_VECTOR_CALLBACK_VOID(&localdeps, STR_APPEND_TEXT, printmissing);
+		ast_log(LOG_WARNING, "Declined modules which depend on %s: %s\n",
+			ast_module_name(mod), ast_str_buffer(printmissing));
+	}
+	AST_VECTOR_FREE(&localdeps);
+
+	*printmissing_ptr = printmissing;
+
+	return res;
+}
+
 static int start_resource_list(struct module_vector *resources, int *mod_count)
 {
 	struct module_vector missingdeps;
 	int res = 0;
+	struct ast_str *printmissing = NULL;
 
 	AST_VECTOR_INIT(&missingdeps, 0);
-	while (AST_VECTOR_SIZE(resources)) {
+	while (res != -2 && AST_VECTOR_SIZE(resources)) {
 		struct ast_module *mod = AST_VECTOR_REMOVE(resources, 0, 1);
 		enum ast_module_load_result lres;
 
+		if (mod->flags.declined) {
+			ast_debug(1, "%s is already declined, skipping\n", ast_module_name(mod));
+			continue;
+		}
+
 retry_load:
 		lres = start_resource_attempt(mod, mod_count);
 		if (lres == AST_MODULE_LOAD_SUCCESS) {
@@ -1680,21 +1761,19 @@
 		}
 
 		if (lres == AST_MODULE_LOAD_FAILURE) {
-			ast_log(LOG_ERROR, "Failed to load %s.\n", ast_module_name(mod));
 			res = -2;
 			break;
 		}
 
 		if (lres == AST_MODULE_LOAD_DECLINE) {
+			res = resource_list_recursive_decline(resources, mod, &printmissing);
 			continue;
 		}
 
-		res = module_deps_missing_recursive(mod, &missingdeps);
-		if (res) {
+		if (module_deps_missing_recursive(mod, &missingdeps)) {
 			AST_VECTOR_RESET(&missingdeps, AST_VECTOR_ELEM_CLEANUP_NOOP);
 			ast_log(LOG_ERROR, "Failed to resolve dependencies for %s\n", ast_module_name(mod));
-			mod->flags.declined = 1;
-
+			res = resource_list_recursive_decline(resources, mod, &printmissing);
 			continue;
 		}
 
@@ -1703,8 +1782,8 @@
 				"This is a bug in the module.\n", ast_module_name(mod));
 			/* Dependencies were met but the module failed to start and the result
 			 * code was not AST_MODULE_LOAD_FAILURE or AST_MODULE_LOAD_DECLINE. */
-			res = -1;
-			break;
+			res = resource_list_recursive_decline(resources, mod, &printmissing);
+			continue;
 		}
 
 		ast_debug(1, "%s has %d dependencies\n",
@@ -1716,8 +1795,16 @@
 			while (i < AST_VECTOR_SIZE(&missingdeps)) {
 				struct ast_module *dep = AST_VECTOR_GET(&missingdeps, i);
 
+				if (dep->flags.declined) {
+					ast_debug(1, "%s tried to start %s but it's already declined\n",
+						ast_module_name(mod), ast_module_name(dep));
+					i++;
+					continue;
+				}
+
 				ast_debug(1, "%s trying to start %s\n", ast_module_name(mod), ast_module_name(dep));
-				if (!start_resource_attempt(dep, mod_count)) {
+				lres = start_resource_attempt(dep, mod_count);
+				if (lres == AST_MODULE_LOAD_SUCCESS) {
 					ast_debug(1, "%s started %s\n", ast_module_name(mod), ast_module_name(dep));
 					AST_VECTOR_REMOVE(&missingdeps, i, 1);
 					AST_VECTOR_REMOVE_CMP_ORDERED(resources, dep,
@@ -1725,6 +1812,13 @@
 					didwork++;
 					continue;
 				}
+
+				if (lres == AST_MODULE_LOAD_FAILURE) {
+					ast_log(LOG_ERROR, "Failed to load %s.\n", ast_module_name(dep));
+					res = -2;
+					goto exitpoint;
+				}
+
 				ast_debug(1, "%s failed to start %s\n", ast_module_name(mod), ast_module_name(dep));
 				i++;
 			}
@@ -1735,9 +1829,26 @@
 		}
 
 		if (AST_VECTOR_SIZE(&missingdeps)) {
-			ast_log(LOG_WARNING, "Failed to load %s due to unfilled dependencies.\n",
-				ast_module_name(mod));
-			mod->flags.declined = 1;
+			if (!printmissing) {
+				printmissing = ast_str_create(64);
+			} else {
+				ast_str_reset(printmissing);
+			}
+
+			if (printmissing) {
+				struct ast_vector_const_string localdeps;
+
+				AST_VECTOR_INIT(&localdeps, 0);
+				module_deps_reference(mod, &localdeps);
+				AST_VECTOR_CALLBACK_VOID(&localdeps, STR_APPEND_TEXT, printmissing);
+				AST_VECTOR_FREE(&localdeps);
+			}
+
+			ast_log(LOG_WARNING, "Failed to load %s due to dependencies: %s.\n",
+				ast_module_name(mod),
+				printmissing ? ast_str_buffer(printmissing) : "allocation failure creating list");
+			res = resource_list_recursive_decline(resources, mod, &printmissing);
+
 			AST_VECTOR_RESET(&missingdeps, AST_VECTOR_ELEM_CLEANUP_NOOP);
 
 			continue;
@@ -1748,6 +1859,8 @@
 		goto retry_load;
 	}
 
+exitpoint:
+	ast_free(printmissing);
 	AST_VECTOR_FREE(&missingdeps);
 
 	return res;
@@ -1772,7 +1885,7 @@
 		return -1;
 	}
 
-	while (!res) {
+	while (res != -2) {
 		didwork = 0;
 
 		AST_LIST_TRAVERSE_SAFE_BEGIN(load_order, order, entry) {
@@ -1789,6 +1902,7 @@
 				 * module that is missing dependencies. */
 				break;
 			case AST_MODULE_LOAD_DECLINE:
+				res = -1;
 				break;
 			case AST_MODULE_LOAD_FAILURE:
 				/* LOAD_FAILURE only happens for required modules */
@@ -1821,7 +1935,7 @@
 		attempt++;
 	}
 
-	if (!res) {
+	if (res != -2) {
 		res = start_resource_list(&module_priorities, &count);
 	}
 
@@ -2023,6 +2137,10 @@
 		ast_log(LOG_NOTICE, "%u modules will be loaded.\n", load_count);
 
 	res = load_resource_list(&load_order, &modulecount);
+	if (res == -1) {
+		ast_log(LOG_WARNING, "Some non-required modules failed to load.\n");
+		res = 0;
+	}
 
 done:
 	while ((order = AST_LIST_REMOVE_HEAD(&load_order, entry))) {

-- 
To view, visit https://gerrit.asterisk.org/10315
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I046052c71331c556c09d39f47a3b92975f3e1758
Gerrit-Change-Number: 10315
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/20180928/fab6f873/attachment-0001.html>


More information about the asterisk-code-review mailing list