[Asterisk-code-review] pbx: Ensure we don't operate on dialplan while dialplan is reloading, ... (asterisk[18])

[Mark Murawski]

Mark Murawski has uploaded this change for review. ( https://gerrit.asterisk.org/c/asterisk/+/17798 )


Change subject: pbx: Ensure we don't operate on dialplan while dialplan is reloading, which causes deadlocks
......................................................................

pbx: Ensure we don't operate on dialplan while dialplan is reloading, which causes deadlocks

Previously it was possible to execute 'dialplan add' during a dialplan
reload, causing deadlocks

Now we prevent 'dialplan add' until the dialplan completes reloading.
Any 'dialplan add' executed during a reload will fail gracefully.

This change also prevents 'continuous dialplan reload'.  Previously
subsequent 'dialplan reload' would all be queued.  For example 10x
'dialplan reload' will result in asterisk being busy reloading
dialplan 10 times in a row, which is doubtfully desited behavior on
any system when there's backend scripts instructing Asterisk to reload
upon change.

Now, subsequent 'dialplan reload' will result in the normal 'module
reload in progress' type of behavior that many other modules implement
if they recieve commands during a reload.

ASTERISK-29774 #close
Reported-by: Steve Sether

Change-Id: I435db97fc56f18b6f5c25a3e400603817b07aa49
---
M include/asterisk/module.h
M main/loader.c
M main/pbx.c
M pbx/pbx_config.c
4 files changed, 42 insertions(+), 5 deletions(-)



  git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/98/17798/1

diff --git a/include/asterisk/module.h b/include/asterisk/module.h
index cce8735..9e089c7 100644
--- a/include/asterisk/module.h
+++ b/include/asterisk/module.h
@@ -178,6 +178,8 @@
  */
 enum ast_module_reload_result ast_module_reload(const char *name);
 
+enum ast_module_reload_result ast_module_reload_pending_check(const char *name);
+
 /*!
  * \brief Notify when usecount has been changed.
  *
diff --git a/main/loader.c b/main/loader.c
index aafcd3b..1058721 100644
--- a/main/loader.c
+++ b/main/loader.c
@@ -1559,6 +1559,24 @@
 	publish_load_message_type("Reload", S_OR(name, "All"), res_buffer);
 }
 
+enum ast_module_reload_result ast_module_reload_pending_check(const char *name)
+{
+	/* If we aren't fully booted, we just pretend we reloaded but we queue this
+	   up to run once we are booted up. */
+	if (!modules_loaded) {
+	        return AST_MODULE_RELOAD_IN_PROGRESS;
+	}
+
+	if (ast_mutex_trylock(&reloadlock)) {
+		ast_verb(3, "The previous reload command didn't finish yet\n");
+		return AST_MODULE_RELOAD_IN_PROGRESS;
+	}
+
+        ast_mutex_unlock(&reloadlock);
+
+        return AST_MODULE_RELOAD_SUCCESS;
+}
+
 enum ast_module_reload_result ast_module_reload(const char *name)
 {
 	struct ast_module *cur;
diff --git a/main/pbx.c b/main/pbx.c
index 07cf8e7..0401b71 100644
--- a/main/pbx.c
+++ b/main/pbx.c
@@ -8508,6 +8508,11 @@
 	return ast_mutex_lock(&conlock);
 }
 
+int ast_rdlock_contexts_try(void)
+{
+	return ast_mutex_trylock(&conlock);
+}
+
 int ast_unlock_contexts(void)
 {
 	return ast_mutex_unlock(&conlock);
diff --git a/pbx/pbx_config.c b/pbx/pbx_config.c
index 648d751..1d3a08d 100644
--- a/pbx/pbx_config.c
+++ b/pbx/pbx_config.c
@@ -1100,6 +1100,11 @@
 		if (strcmp(a->argv[6], "replace"))
 			return CLI_SHOWUSAGE;
 
+        if (ast_module_reload_pending_check("pbx_config") == AST_MODULE_RELOAD_IN_PROGRESS) {
+		ast_cli(a->fd, "Dialplan reload in progress.  Cannot add extensions right now.\n");
+	        return;
+        }
+
 	whole_exten = ast_strdupa(a->argv[3]);
 	exten = strsep(&whole_exten,",");
 	if (strchr(exten, '/')) {
@@ -1575,6 +1580,8 @@
 
 static char *handle_cli_dialplan_reload(struct ast_cli_entry *e, int cmd, struct ast_cli_args *a)
 {
+	static char *ret;
+
 	switch (cmd) {
 	case CLI_INIT:
 		e->command = "dialplan reload";
@@ -1591,12 +1598,15 @@
 	if (a->argc != 2)
 		return CLI_SHOWUSAGE;
 
-	if (clearglobalvars_config)
-		pbx_builtin_clear_globals();
+	/* Lock-Protected reload with early abort if currently reloading or the last reload has not finished */
+	/* This runs reload() under the hood */
+	ret = ast_module_reload("pbx_config") == AST_MODULE_RELOAD_SUCCESS ? CLI_SUCCESS : CLI_FAILURE;
 
-	pbx_load_module();
-	ast_cli(a->fd, "Dialplan reloaded.\n");
-	return CLI_SUCCESS;
+	if (ret == CLI_SUCCESS) {
+		ast_cli(a->fd, "Dialplan reloaded.\n");
+	}
+
+	return ret;
 }
 
 /*!
@@ -2153,6 +2163,8 @@
 {
 	if (clearglobalvars_config)
 		pbx_builtin_clear_globals();
+
+	/* Lock-Protected reload not needed because we're already being called from ast_module_reload() */
 	return pbx_load_module();
 }
 

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

Gerrit-Project: asterisk
Gerrit-Branch: 18
Gerrit-Change-Id: I435db97fc56f18b6f5c25a3e400603817b07aa49
Gerrit-Change-Number: 17798
Gerrit-PatchSet: 1
Gerrit-Owner: Mark Murawski <markm at intellasoft.net>
Gerrit-MessageType: newchange
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20220111/7f75390f/attachment-0001.html>