[Asterisk-code-review] res_musiconhold: Use a vector instead of custom array allocation (...asterisk[17])

George Joseph asteriskteam at digium.com
Tue Aug 6 11:05:39 CDT 2019


George Joseph has submitted this change and it was merged. ( https://gerrit.asterisk.org/c/asterisk/+/11655 )

Change subject: res_musiconhold: Use a vector instead of custom array allocation
......................................................................

res_musiconhold: Use a vector instead of custom array allocation

Change-Id: Ic476a56608b1820ca93dcf68d10cd76fc0b94141
---
M res/res_musiconhold.c
1 file changed, 55 insertions(+), 87 deletions(-)

Approvals:
  Kevin Harwell: Looks good to me, but someone else must approve
  Joshua Colp: Looks good to me, but someone else must approve
  George Joseph: Looks good to me, approved; Approved for Submit



diff --git a/res/res_musiconhold.c b/res/res_musiconhold.c
index b598f08..32e77dd 100644
--- a/res/res_musiconhold.c
+++ b/res/res_musiconhold.c
@@ -171,12 +171,8 @@
 	char announcement[256];
 	char mode[80];
 	char digit;
-	/*! A dynamically sized array to hold the list of filenames in "files" mode */
-	char **filearray;
-	/*! The current size of the filearray */
-	int allowed_files;
-	/*! The current number of files loaded into the filearray */
-	int total_files;
+	/*! A vector of filenames in "files" mode */
+	struct ast_vector_string files;
 	unsigned int flags;
 	/*! The format from the MOH source, not applicable to "files" mode */
 	struct ast_format *format;
@@ -318,6 +314,7 @@
 {
 	struct moh_files_state *state = ast_channel_music_state(chan);
 	int tries;
+	size_t file_count;
 
 	/* Discontinue a stream if it is running already */
 	if (ast_channel_stream(chan)) {
@@ -335,7 +332,8 @@
 		state->announcement = 0;
 	}
 
-	if (!state->class->total_files) {
+	file_count = AST_VECTOR_SIZE(&state->class->files);
+	if (!file_count) {
 		ast_log(LOG_WARNING, "No files available for class '%s'\n", state->class->name);
 		return -1;
 	}
@@ -343,15 +341,15 @@
 	if (state->pos == 0 && ast_strlen_zero(state->save_pos_filename)) {
 		/* First time so lets play the file. */
 		state->save_pos = -1;
-	} else if (state->save_pos >= 0 && state->save_pos < state->class->total_files && !strcmp(state->class->filearray[state->save_pos], state->save_pos_filename)) {
+	} else if (state->save_pos >= 0 && state->save_pos < file_count && !strcmp(AST_VECTOR_GET(&state->class->files, state->save_pos), state->save_pos_filename)) {
 		/* If a specific file has been saved confirm it still exists and that it is still valid */
 		state->pos = state->save_pos;
 		state->save_pos = -1;
 	} else if (ast_test_flag(state->class, MOH_SORTMODE) == MOH_RANDOMIZE) {
 		/* Get a random file and ensure we can open it */
 		for (tries = 0; tries < 20; tries++) {
-			state->pos = ast_random() % state->class->total_files;
-			if (ast_fileexists(state->class->filearray[state->pos], NULL, NULL) > 0) {
+			state->pos = ast_random() % file_count;
+			if (ast_fileexists(AST_VECTOR_GET(&state->class->files, state->pos), NULL, NULL) > 0) {
 				break;
 			}
 		}
@@ -360,29 +358,29 @@
 	} else {
 		/* This is easy, just increment our position and make sure we don't exceed the total file count */
 		state->pos++;
-		state->pos %= state->class->total_files;
+		state->pos %= file_count;
 		state->save_pos = -1;
 		state->samples = 0;
 	}
 
-	for (tries = 0; tries < state->class->total_files; ++tries) {
-		if (ast_openstream_full(chan, state->class->filearray[state->pos], ast_channel_language(chan), 1)) {
+	for (tries = 0; tries < file_count; ++tries) {
+		if (ast_openstream_full(chan, AST_VECTOR_GET(&state->class->files, state->pos), ast_channel_language(chan), 1)) {
 			break;
 		}
 
-		ast_log(LOG_WARNING, "Unable to open file '%s': %s\n", state->class->filearray[state->pos], strerror(errno));
+		ast_log(LOG_WARNING, "Unable to open file '%s': %s\n", AST_VECTOR_GET(&state->class->files, state->pos), strerror(errno));
 		state->pos++;
-		state->pos %= state->class->total_files;
+		state->pos %= file_count;
 	}
 
-	if (tries == state->class->total_files) {
+	if (tries == file_count) {
 		return -1;
 	}
 
 	/* Record the pointer to the filename for position resuming later */
-	ast_copy_string(state->save_pos_filename, state->class->filearray[state->pos], sizeof(state->save_pos_filename));
+	ast_copy_string(state->save_pos_filename, AST_VECTOR_GET(&state->class->files, state->pos), sizeof(state->save_pos_filename));
 
-	ast_debug(1, "%s Opened file %d '%s'\n", ast_channel_name(chan), state->pos, state->class->filearray[state->pos]);
+	ast_debug(1, "%s Opened file %d '%s'\n", ast_channel_name(chan), state->pos, state->save_pos_filename);
 
 	if (state->samples) {
 		size_t loc;
@@ -490,6 +488,7 @@
 {
 	struct moh_files_state *state;
 	struct mohclass *class = params;
+	size_t file_count;
 
 	state = ast_channel_music_state(chan);
 	if (!state && (state = ast_calloc(1, sizeof(*state)))) {
@@ -505,14 +504,16 @@
 		}
 	}
 
+	file_count = AST_VECTOR_SIZE(&class->files);
+
 	/* Resume MOH from where we left off last time or start from scratch? */
-	if (state->save_total != class->total_files || strcmp(state->name, class->name) != 0) {
+	if (state->save_total != file_count || strcmp(state->name, class->name) != 0) {
 		/* Start MOH from scratch. */
 		ao2_cleanup(state->origwfmt);
 		ao2_cleanup(state->mohwfmt);
 		memset(state, 0, sizeof(*state));
-		if (ast_test_flag(class, MOH_RANDOMIZE) && class->total_files) {
-			state->pos = ast_random() % class->total_files;
+		if (ast_test_flag(class, MOH_RANDOMIZE) && file_count) {
+			state->pos = ast_random() % file_count;
 		}
 	}
 
@@ -522,7 +523,7 @@
 	ao2_replace(state->mohwfmt, ast_channel_writeformat(chan));
 	/* For comparison on restart of MOH (see above) */
 	ast_copy_string(state->name, class->name, sizeof(state->name));
-	state->save_total = class->total_files;
+	state->save_total = file_count;
 
 	moh_post_start(chan, class->name);
 
@@ -1131,45 +1132,6 @@
 	}
 }
 
-static int moh_add_file(struct mohclass *class, const char *filepath)
-{
-	if (!class->allowed_files) {
-		class->filearray = ast_calloc(1, INITIAL_NUM_FILES * sizeof(*class->filearray));
-		if (!class->filearray) {
-			return -1;
-		}
-		class->allowed_files = INITIAL_NUM_FILES;
-	} else if (class->total_files == class->allowed_files) {
-		char **new_array;
-
-		new_array = ast_realloc(class->filearray, class->allowed_files * sizeof(*class->filearray) * 2);
-		if (!new_array) {
-			return -1;
-		}
-		class->filearray = new_array;
-		class->allowed_files *= 2;
-	}
-
-	class->filearray[class->total_files] = ast_strdup(filepath);
-	if (!class->filearray[class->total_files]) {
-		return -1;
-	}
-
-	class->total_files++;
-
-	return 0;
-}
-
-static int moh_sort_compare(const void *i1, const void *i2)
-{
-	char *s1, *s2;
-
-	s1 = ((char **)i1)[0];
-	s2 = ((char **)i2)[0];
-
-	return strcasecmp(s1, s2);
-}
-
 static int moh_scan_files(struct mohclass *class) {
 
 	DIR *files_DIR;
@@ -1178,7 +1140,7 @@
 	char filepath[PATH_MAX];
 	char *ext;
 	struct stat statbuf;
-	int i;
+	int res;
 
 	if (class->dir[0] != '/') {
 		snprintf(dir_path, sizeof(dir_path), "%s/%s", ast_config_AST_DATA_DIR, class->dir);
@@ -1192,12 +1154,11 @@
 		return -1;
 	}
 
-	for (i = 0; i < class->total_files; i++) {
-		ast_free(class->filearray[i]);
-	}
-	class->total_files = 0;
+	AST_VECTOR_RESET(&class->files, ast_free);
 
 	while ((files_dirent = readdir(files_DIR))) {
+		char *filepath_copy;
+
 		/* The file name must be at least long enough to have the file type extension */
 		if ((strlen(files_dirent->d_name) < 4))
 			continue;
@@ -1222,20 +1183,32 @@
 			*ext = '\0';
 
 		/* if the file is present in multiple formats, ensure we only put it into the list once */
-		for (i = 0; i < class->total_files; i++)
-			if (!strcmp(filepath, class->filearray[i]))
-				break;
+		if (AST_VECTOR_GET_CMP(&class->files, &filepath[0], !strcmp)) {
+			continue;
+		}
 
-		if (i == class->total_files) {
-			if (moh_add_file(class, filepath))
-				break;
+		filepath_copy = ast_strdup(filepath);
+		if (!filepath_copy) {
+			break;
+		}
+
+		if (ast_test_flag(class, MOH_SORTALPHA)) {
+			res = AST_VECTOR_ADD_SORTED(&class->files, filepath_copy, strcasecmp);
+		} else {
+			res = AST_VECTOR_APPEND(&class->files, filepath_copy);
+		}
+
+		if (res) {
+			ast_free(filepath_copy);
+			break;
 		}
 	}
 
 	closedir(files_DIR);
-	if (ast_test_flag(class, MOH_SORTALPHA))
-		qsort(&class->filearray[0], class->total_files, sizeof(char *), moh_sort_compare);
-	return class->total_files;
+
+	AST_VECTOR_COMPACT(&class->files);
+
+	return AST_VECTOR_SIZE(&class->files);
 }
 
 static int init_files_class(struct mohclass *class)
@@ -1420,6 +1393,7 @@
 		class->format = ao2_bump(ast_format_slin);
 		class->srcfd = -1;
 		class->kill_delay = 100000;
+		AST_VECTOR_INIT(&class->files, 0);
 	}
 
 	return class;
@@ -1614,7 +1588,7 @@
 	}
 
 	if (!state || !state->class || strcmp(mohclass->name, state->class->name)) {
-		if (mohclass->total_files) {
+		if (AST_VECTOR_SIZE(&mohclass->files)) {
 			res = ast_activate_generator(chan, &moh_file_stream, mohclass);
 		} else {
 			res = ast_activate_generator(chan, &mohgen, mohclass);
@@ -1693,14 +1667,8 @@
 		class->srcfd = -1;
 	}
 
-	if (class->filearray) {
-		int i;
-		for (i = 0; i < class->total_files; i++) {
-			ast_free(class->filearray[i]);
-		}
-		ast_free(class->filearray);
-		class->filearray = NULL;
-	}
+	AST_VECTOR_RESET(&class->files, ast_free);
+	AST_VECTOR_FREE(&class->files);
 
 	if (class->timer) {
 		ast_timer_close(class->timer);
@@ -1885,13 +1853,13 @@
 	for (; (class = ao2_t_iterator_next(&i, "Show files iterator")); mohclass_unref(class, "Unref iterator in moh show files")) {
 		int x;
 
-		if (!class->total_files) {
+		if (!AST_VECTOR_SIZE(&class->files)) {
 			continue;
 		}
 
 		ast_cli(a->fd, "Class: %s\n", class->name);
-		for (x = 0; x < class->total_files; x++) {
-			ast_cli(a->fd, "\tFile: %s\n", class->filearray[x]);
+		for (x = 0; x < AST_VECTOR_SIZE(&class->files); x++) {
+			ast_cli(a->fd, "\tFile: %s\n", AST_VECTOR_GET(&class->files, x));
 		}
 	}
 	ao2_iterator_destroy(&i);

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

Gerrit-Project: asterisk
Gerrit-Branch: 17
Gerrit-Change-Id: Ic476a56608b1820ca93dcf68d10cd76fc0b94141
Gerrit-Change-Number: 11655
Gerrit-PatchSet: 3
Gerrit-Owner: Sean Bright <sean.bright at gmail.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-MessageType: merged
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20190806/cd0db217/attachment-0001.html>


More information about the asterisk-code-review mailing list