[Asterisk-code-review] loader: Use ast cli completion add for 'module load' complet... (asterisk[15])

Jenkins2 asteriskteam at digium.com
Wed Jan 31 07:46:15 CST 2018


Jenkins2 has submitted this change and it was merged. ( https://gerrit.asterisk.org/8051 )

Change subject: loader: Use ast_cli_completion_add for 'module load' completion.
......................................................................

loader: Use ast_cli_completion_add for 'module load' completion.

This addresses all performance issues with 'module load' completion.  In
addition to using ast_cli_completion_add we stop using libedit's
filename_completion_function, instead using ast_file_read_dir.  This
ensures all results are produced from a single call to opendir.

Change-Id: I8bf51ffaa7ef1606f3bd1b5bb13f1905d72c6134
---
M include/asterisk/file.h
M main/Makefile
M main/loader.c
3 files changed, 54 insertions(+), 50 deletions(-)

Approvals:
  Sean Bright: Looks good to me, but someone else must approve
  Joshua Colp: Looks good to me, approved
  Jenkins2: Approved for Submit



diff --git a/include/asterisk/file.h b/include/asterisk/file.h
index 453dc07..c17cb32 100644
--- a/include/asterisk/file.h
+++ b/include/asterisk/file.h
@@ -143,6 +143,11 @@
  * \param filename the name of the file
  * \param obj user data object
  * \return non-zero to stop reading, otherwise zero to continue
+ *
+ * \note dir_name is not processed by realpath or other functions,
+ *       symbolic links are not resolved.  This ensures dir_name
+ *       always starts with the exact string originally passed to
+ *       \ref ast_file_read_dir or \ref ast_file_read_dirs.
  */
 typedef int (*ast_file_on_file)(const char *dir_name, const char *filename, void *obj);
 
diff --git a/main/Makefile b/main/Makefile
index c724e20..63b65af 100644
--- a/main/Makefile
+++ b/main/Makefile
@@ -149,7 +149,6 @@
 
 db.o: _ASTCFLAGS+=$(SQLITE3_INCLUDE)
 asterisk.o: _ASTCFLAGS+=$(LIBEDIT_INCLUDE)
-loader.o: _ASTCFLAGS+=$(LIBEDIT_INCLUDE)
 json.o: _ASTCFLAGS+=$(JANSSON_INCLUDE)
 bucket.o: _ASTCFLAGS+=$(URIPARSER_INCLUDE)
 crypt.o: _ASTCFLAGS+=$(CRYPT_INCLUDE)
diff --git a/main/loader.c b/main/loader.c
index afb4054..c495d76 100644
--- a/main/loader.c
+++ b/main/loader.c
@@ -36,7 +36,6 @@
 #include "asterisk/_private.h"
 #include "asterisk/paths.h"	/* use ast_config_AST_MODULE_DIR */
 #include <dirent.h>
-#include <editline/readline.h>
 
 #include "asterisk/dlinkedlists.h"
 #include "asterisk/module.h"
@@ -56,6 +55,7 @@
 #include "asterisk/app.h"
 #include "asterisk/test.h"
 #include "asterisk/sounds_index.h"
+#include "asterisk/cli.h"
 
 #include <dlfcn.h>
 
@@ -765,57 +765,55 @@
 	}
 }
 
-static char *module_load_helper(const char *word, int state)
-{
-	struct ast_module *mod;
-	int which = 0;
-	char *name;
-	char *ret = NULL;
-	char *editline_ret;
-	char fullpath[PATH_MAX];
-	int idx = 0;
-	/* This is needed to avoid listing modules that are already running. */
-	AST_VECTOR(, char *) running_modules;
+struct module_load_word {
+	const char *word;
+	size_t len;
+	size_t moddir_len;
+};
 
-	AST_VECTOR_INIT(&running_modules, 200);
+static int module_load_helper_on_file(const char *dir_name, const char *filename, void *obj)
+{
+	struct module_load_word *word = obj;
+	struct ast_module *mod;
+	char *filename_merged = NULL;
+
+	/* dir_name will never be shorter than word->moddir_len. */
+	dir_name += word->moddir_len;
+	if (!ast_strlen_zero(dir_name)) {
+		ast_assert(dir_name[0] == '/');
+
+		dir_name += 1;
+		if (ast_asprintf(&filename_merged, "%s/%s", dir_name, filename) < 0) {
+			/* If we can't allocate the string just give up! */
+			return -1;
+		}
+		filename = filename_merged;
+	}
+
+	if (!strncasecmp(filename, word->word, word->len)) {
+		/* Don't list files that are already loaded! */
+		mod = find_resource(filename, 0);
+		if (!mod || !mod->flags.running) {
+			ast_cli_completion_add(ast_strdup(filename));
+		}
+	}
+
+	ast_free(filename_merged);
+
+	return 0;
+}
+
+static void module_load_helper(const char *word)
+{
+	struct module_load_word word_l = {
+		.word = word,
+		.len = strlen(word),
+		.moddir_len = strlen(ast_config_AST_MODULE_DIR),
+	};
 
 	AST_DLLIST_LOCK(&module_list);
-	AST_DLLIST_TRAVERSE(&module_list, mod, entry) {
-		if (mod->flags.running) {
-			AST_VECTOR_APPEND(&running_modules, mod->resource);
-		}
-	}
-
-	if (word[0] == '/') {
-		/* BUGBUG: we should not support this. */
-		ast_copy_string(fullpath, word, sizeof(fullpath));
-	} else {
-		snprintf(fullpath, sizeof(fullpath), "%s/%s", ast_config_AST_MODULE_DIR, word);
-	}
-
-	/*
-	 * This is ugly that we keep calling filename_completion_function.
-	 * The only way to avoid this would be to make a copy of the function
-	 * that skips matches found in the running_modules vector.
-	 */
-	while (!ret && (name = editline_ret = filename_completion_function(fullpath, idx++))) {
-		if (word[0] != '/') {
-			name += (strlen(ast_config_AST_MODULE_DIR) + 1);
-		}
-
-		/* Don't list files that are already loaded! */
-		if (!AST_VECTOR_GET_CMP(&running_modules, name, !strcasecmp) && ++which > state) {
-			ret = ast_strdup(name);
-		}
-
-		ast_std_free(editline_ret);
-	}
-
-	/* Do not clean-up the elements, they belong to module_list. */
-	AST_VECTOR_FREE(&running_modules);
+	ast_file_read_dirs(ast_config_AST_MODULE_DIR, module_load_helper_on_file, &word_l, -1);
 	AST_DLLIST_UNLOCK(&module_list);
-
-	return ret;
 }
 
 char *ast_module_helper(const char *line, const char *word, int pos, int state, int rpos, int _type)
@@ -831,7 +829,9 @@
 	}
 
 	if (type == AST_MODULE_HELPER_LOAD) {
-		return module_load_helper(word, state);
+		module_load_helper(word);
+
+		return NULL;
 	}
 
 	if (type == AST_MODULE_HELPER_RELOAD) {

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

Gerrit-Project: asterisk
Gerrit-Branch: 15
Gerrit-MessageType: merged
Gerrit-Change-Id: I8bf51ffaa7ef1606f3bd1b5bb13f1905d72c6134
Gerrit-Change-Number: 8051
Gerrit-PatchSet: 4
Gerrit-Owner: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Sean Bright <sean.bright at gmail.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180131/07a021c7/attachment.html>


More information about the asterisk-code-review mailing list