[asterisk-commits] stasis recording/stored: remove calls to deprecated readdir ... (asterisk[14])
SVN commits to the Asterisk project
asterisk-commits at lists.digium.com
Tue Nov 8 04:57:37 CST 2016
Joshua Colp has submitted this change and it was merged. ( https://gerrit.asterisk.org/4223 )
Change subject: stasis_recording/stored: remove calls to deprecated readdir_r function.
......................................................................
stasis_recording/stored: remove calls to deprecated readdir_r function.
The readdir_r function has been deprecated and should no longer be used. This
patch removes the readdir_r dependency (replaced it with readdir) and also moves
the directory search code to a more centralized spot (file.c)
Also removed a strict dependency on the dirent structure's d_type field as it
is not portable. The code now checks to see if the value is available. If so,
it tries to use it, but defaults back to using the stats function if necessary.
Lastly, for most implementations of readdir it *should* be thread-safe to make
concurrent calls to it as long as different directory streams are specified.
glibc falls into this category. However, since it is possible that there exist
some implementations that are not safe, locking has been added for those other
than glibc.
ASTERISK-26412
ASTERISK-26509 #close
Change-Id: Id8f54689b1e2873e82a09d0d0d2faf41964e80ba
---
M include/asterisk/file.h
M main/file.c
M res/stasis_recording/stored.c
A tests/test_file.c
4 files changed, 421 insertions(+), 151 deletions(-)
Approvals:
Mark Michelson: Looks good to me, but someone else must approve
Anonymous Coward #1000019: Verified
Joshua Colp: Looks good to me, approved
Corey Farrell: Looks good to me, but someone else must approve
diff --git a/include/asterisk/file.h b/include/asterisk/file.h
index c71866e..01e5797 100644
--- a/include/asterisk/file.h
+++ b/include/asterisk/file.h
@@ -138,6 +138,34 @@
int ast_filecopy(const char *oldname, const char *newname, const char *fmt);
/*!
+ * \brief Callback called for each file found when reading directories
+ * \param dir_name the name of the directory
+ * \param filename the name of the file
+ * \param obj user data object
+ * \return non-zero to stop reading, otherwise zero to continue
+ */
+typedef int (*ast_file_on_file)(const char *dir_name, const char *filename, void *obj);
+
+/*!
+ * \brief Recursively iterate through files and directories up to max_depth
+ * \param dir_name the name of the directory to search
+ * \param on_file callback called on each file
+ * \param obj user data object
+ * \param max_depth re-curse into sub-directories up to a given maximum (-1 = infinite)
+ * \return -1 or errno on failure, otherwise 0
+ */
+int ast_file_read_dirs(const char *dir_name, ast_file_on_file on_file, void *obj, int max_depth);
+
+/*!
+ * \brief Iterate over each file in a given directory
+ * \param dir_name the name of the directory to search
+ * \param on_file callback called on each file
+ * \param obj user data object
+ * \return -1 or errno on failure, otherwise 0
+ */
+#define ast_file_read_dir(dir_name, on_file, obj) ast_file_read_dirs(dir_name, on_file, obj, 1)
+
+/*!
* \brief Waits for a stream to stop or digit to be pressed
* \param c channel to waitstream on
* \param breakon string of DTMF digits to break upon
diff --git a/main/file.c b/main/file.c
index 4503625..0239416 100644
--- a/main/file.c
+++ b/main/file.c
@@ -1095,6 +1095,150 @@
return filehelper(filename, filename2, fmt, ACTION_COPY);
}
+static int __ast_file_read_dirs(struct ast_str **path, ast_file_on_file on_file,
+ void *obj, int max_depth)
+{
+ DIR *dir;
+ struct dirent *entry;
+ size_t size;
+ int res;
+
+ if (!(dir = opendir(ast_str_buffer(*path)))) {
+ ast_log(LOG_ERROR, "Error opening directory - %s: %s\n",
+ ast_str_buffer(*path), strerror(errno));
+ return -1;
+ }
+ size = ast_str_strlen(*path);
+
+ --max_depth;
+
+ res = 0;
+
+ while ((entry = readdir(dir)) != NULL && !errno) {
+ int is_file, is_dir, used_stat = 0;
+
+ if (!strcmp(entry->d_name, ".") || !strcmp(entry->d_name, "..")) {
+ continue;
+ }
+
+/*
+ * If the dirent structure has a d_type use it to determine if we are dealing with
+ * a file or directory. Unfortunately if it doesn't have it, or if the type is
+ * unknown, or a link then we'll need to use the stat function instead.
+ */
+#ifdef _DIRENT_HAVE_D_TYPE
+ if (entry->d_type != DT_UNKNOWN && entry->d_type != DT_LNK) {
+ is_file = entry->d_type == DT_REG;
+ is_dir = entry->d_type == DT_DIR;
+ } else
+#endif
+ {
+ struct stat statbuf;
+
+ /*
+ * If using the stat function the file needs to be appended to the
+ * path so it can be found. However, before appending make sure the
+ * path contains only the directory for this depth level.
+ */
+ ast_str_truncate(*path, size);
+ ast_str_append(path, 0, "/%s", entry->d_name);
+
+ if (stat(ast_str_buffer(*path), &statbuf)) {
+ ast_log(LOG_ERROR, "Error reading path stats - %s: %s\n",
+ ast_str_buffer(*path), strerror(errno));
+ /*
+ * Output an error, but keep going. It could just be
+ * a broken link and other files could be fine.
+ */
+ continue;
+ }
+
+ is_file = S_ISREG(statbuf.st_mode);
+ is_dir = S_ISDIR(statbuf.st_mode);
+ used_stat = 1;
+ }
+
+ if (is_file) {
+ /* If the handler returns non-zero then stop */
+ if ((res = on_file(ast_str_buffer(*path), entry->d_name, obj))) {
+ break;
+ }
+ /* Otherwise move on to next item in directory */
+ continue;
+ }
+
+ if (!is_dir) {
+ ast_debug(5, "Skipping %s: not a regular file or directory\n",
+ ast_str_buffer(*path));
+ continue;
+ }
+
+ /* Only re-curse into sub-directories if not at the max depth */
+ if (max_depth != 0) {
+ /*
+ * If the stat function was used then the sub-directory has
+ * already been appended, otherwise append it.
+ */
+ if (!used_stat) {
+ ast_str_truncate(*path, size);
+ ast_str_append(path, 0, "/%s", entry->d_name);
+ }
+
+ if ((res = __ast_file_read_dirs(path, on_file, obj, max_depth))) {
+ break;
+ }
+ }
+ }
+
+ closedir(dir);
+
+ if (!res && errno) {
+ ast_log(LOG_ERROR, "Error while reading directories - %s: %s\n",
+ ast_str_buffer(*path), strerror(errno));
+ res = -1;
+ }
+
+ return res;
+}
+
+#if !defined(__GLIBC__)
+/*!
+ * \brief Lock to hold when iterating over directories.
+ *
+ * Currently, 'readdir' is not required to be thread-safe. In most modern implementations
+ * it should be safe to make concurrent calls into 'readdir' that specify different directory
+ * streams (glibc would be one of these). However, since it is potentially unsafe for some
+ * implementations we'll use our own locking in order to achieve synchronization for those.
+ */
+AST_MUTEX_DEFINE_STATIC(read_dirs_lock);
+#endif
+
+int ast_file_read_dirs(const char *dir_name, ast_file_on_file on_file, void *obj, int max_depth)
+{
+ struct ast_str *path;
+ int res;
+
+ if (!(path = ast_str_create(256))) {
+ return -1;
+ }
+
+ ast_str_set(&path, 0, "%s", dir_name);
+ errno = 0;
+
+#if !defined(__GLIBC__)
+ ast_mutex_lock(&read_dirs_lock);
+#endif
+
+ res = __ast_file_read_dirs(&path, on_file, obj, max_depth);
+
+#if !defined(__GLIBC__)
+ ast_mutex_unlock(&read_dirs_lock);
+#endif
+
+ ast_free(path);
+ return res;
+}
+
int ast_streamfile(struct ast_channel *chan, const char *filename, const char *preflang)
{
struct ast_filestream *fs;
diff --git a/res/stasis_recording/stored.c b/res/stasis_recording/stored.c
index 50232c4..e0fd4db 100644
--- a/res/stasis_recording/stored.c
+++ b/res/stasis_recording/stored.c
@@ -31,7 +31,6 @@
#include "asterisk/paths.h"
#include "asterisk/stasis_app_recording.h"
-#include <dirent.h>
#include <sys/stat.h>
#include <sys/types.h>
#include <unistd.h>
@@ -140,12 +139,47 @@
return 0;
}
-static void safe_closedir(DIR *dirp)
+struct match_recording_data {
+ const char *file;
+ char *file_with_ext;
+};
+
+static int is_recording(const char *filename)
{
- if (!dirp) {
- return;
+ const char *ext = strrchr(filename, '.');
+
+ if (!ext) {
+ /* No file extension; not us */
+ return 0;
}
- closedir(dirp);
+ ++ext;
+
+ if (!ast_get_format_for_file_ext(ext)) {
+ ast_debug(5, "Recording %s: unrecognized format %s\n",
+ filename, ext);
+ /* Keep looking */
+ return 0;
+ }
+
+ /* Return the index to the .ext */
+ return ext - filename - 1;
+}
+
+static int handle_find_recording(const char *dir_name, const char *filename, void *obj)
+{
+ struct match_recording_data *data = obj;
+ int num;
+
+ /* If not a recording or the names do not match the keep searching */
+ if (!(num = is_recording(filename)) || strncmp(data->file, filename, num)) {
+ return 0;
+ }
+
+ if (ast_asprintf(&data->file_with_ext, "%s/%s", dir_name, filename)) {
+ return -1;
+ }
+
+ return 1;
}
/*!
@@ -161,46 +195,15 @@
*/
static char *find_recording(const char *dir_name, const char *file)
{
- RAII_VAR(DIR *, dir, NULL, safe_closedir);
- struct dirent entry;
- struct dirent *result = NULL;
- char *ext = NULL;
- char *file_with_ext = NULL;
+ struct match_recording_data data = {
+ .file = file,
+ .file_with_ext = NULL
+ };
- dir = opendir(dir_name);
- if (!dir) {
- return NULL;
- }
+ ast_file_read_dir(dir_name, handle_find_recording, &data);
- while (readdir_r(dir, &entry, &result) == 0 && result != NULL) {
- ext = strrchr(result->d_name, '.');
-
- if (!ext) {
- /* No file extension; not us */
- continue;
- }
- *ext++ = '\0';
-
- if (strcmp(file, result->d_name) == 0) {
- if (!ast_get_format_for_file_ext(ext)) {
- ast_log(LOG_WARNING,
- "Recording %s: unrecognized format %s\n",
- result->d_name,
- ext);
- /* Keep looking */
- continue;
- }
- /* We have a winner! */
- break;
- }
- }
-
- if (!result) {
- return NULL;
- }
-
- ast_asprintf(&file_with_ext, "%s/%s.%s", dir_name, file, ext);
- return file_with_ext;
+ /* Note, string potentially allocated in handle_file_recording */
+ return data.file_with_ext;
}
/*!
@@ -256,43 +259,33 @@
return cmp;
}
-static int scan(struct ao2_container *recordings,
- const char *base_dir, const char *subdir, struct dirent *entry);
-
-static int scan_file(struct ao2_container *recordings,
- const char *base_dir, const char *subdir, const char *filename,
- const char *path)
+static int handle_scan_file(const char *dir_name, const char *filename, void *obj)
{
- RAII_VAR(struct stasis_app_stored_recording *, recording, NULL,
- ao2_cleanup);
- const char *ext;
- char *dot;
+ struct ao2_container *recordings = obj;
+ struct stasis_app_stored_recording *recording;
+ char *dot, *filepath;
- ext = strrchr(filename, '.');
-
- if (!ext) {
- ast_verb(4, " Ignore file without extension: %s\n",
- filename);
- /* No file extension; not us */
+ /* Skip if it is not a recording */
+ if (!is_recording(filename)) {
return 0;
}
- ++ext;
- if (!ast_get_format_for_file_ext(ext)) {
- ast_verb(4, " Not a media file: %s\n", filename);
- /* Not a media file */
- return 0;
+ if (ast_asprintf(&filepath, "%s/%s", dir_name, filename)) {
+ return -1;
}
recording = recording_alloc();
if (!recording) {
+ ast_free(filepath);
return -1;
}
- ast_string_field_set(recording, file_with_ext, path);
-
+ ast_string_field_set(recording, file_with_ext, filepath);
/* Build file and format from full path */
- ast_string_field_set(recording, file, path);
+ ast_string_field_set(recording, file, filepath);
+
+ ast_free(filepath);
+
dot = strrchr(recording->file, '.');
*dot = '\0';
recording->format = dot + 1;
@@ -303,92 +296,14 @@
/* Add it to the recordings container */
ao2_link(recordings, recording);
-
- return 0;
-}
-
-static int scan_dir(struct ao2_container *recordings,
- const char *base_dir, const char *subdir, const char *dirname,
- const char *path)
-{
- RAII_VAR(DIR *, dir, NULL, safe_closedir);
- RAII_VAR(struct ast_str *, rel_dirname, NULL, ast_free);
- struct dirent entry;
- struct dirent *result = NULL;
-
- if (strcmp(dirname, ".") == 0 ||
- strcmp(dirname, "..") == 0) {
- ast_verb(4, " Ignoring self/parent dir\n");
- return 0;
- }
-
- /* Build relative dirname */
- rel_dirname = ast_str_create(80);
- if (!rel_dirname) {
- return -1;
- }
- if (!ast_strlen_zero(subdir)) {
- ast_str_append(&rel_dirname, 0, "%s/", subdir);
- }
- if (!ast_strlen_zero(dirname)) {
- ast_str_append(&rel_dirname, 0, "%s", dirname);
- }
-
- /* Read the directory */
- dir = opendir(path);
- if (!dir) {
- ast_log(LOG_WARNING, "Error reading dir '%s'\n", path);
- return -1;
- }
- while (readdir_r(dir, &entry, &result) == 0 && result != NULL) {
- scan(recordings, base_dir, ast_str_buffer(rel_dirname), result);
- }
-
- return 0;
-}
-
-static int scan(struct ao2_container *recordings,
- const char *base_dir, const char *subdir, struct dirent *entry)
-{
- RAII_VAR(struct ast_str *, path, NULL, ast_free);
-
- path = ast_str_create(255);
- if (!path) {
- return -1;
- }
-
- /* Build file path */
- ast_str_append(&path, 0, "%s", base_dir);
- if (!ast_strlen_zero(subdir)) {
- ast_str_append(&path, 0, "/%s", subdir);
- }
- if (entry) {
- ast_str_append(&path, 0, "/%s", entry->d_name);
- }
- ast_verb(4, "Scanning '%s'\n", ast_str_buffer(path));
-
- /* Handle this file */
- switch (entry->d_type) {
- case DT_REG:
- scan_file(recordings, base_dir, subdir, entry->d_name,
- ast_str_buffer(path));
- break;
- case DT_DIR:
- scan_dir(recordings, base_dir, subdir, entry->d_name,
- ast_str_buffer(path));
- break;
- default:
- ast_log(LOG_WARNING, "Skipping %s: not a regular file\n",
- ast_str_buffer(path));
- break;
- }
+ ao2_ref(recording, -1);
return 0;
}
struct ao2_container *stasis_app_stored_recording_find_all(void)
{
- RAII_VAR(struct ao2_container *, recordings, NULL, ao2_cleanup);
+ struct ao2_container *recordings;
int res;
recordings = ao2_container_alloc_rbtree(AO2_ALLOC_OPT_LOCK_NOLOCK,
@@ -397,13 +312,13 @@
return NULL;
}
- res = scan_dir(recordings, ast_config_AST_RECORDING_DIR, "", "",
- ast_config_AST_RECORDING_DIR);
- if (res != 0) {
+ res = ast_file_read_dirs(ast_config_AST_RECORDING_DIR,
+ handle_scan_file, recordings, -1);
+ if (res) {
+ ao2_ref(recordings, -1);
return NULL;
}
- ao2_ref(recordings, +1);
return recordings;
}
diff --git a/tests/test_file.c b/tests/test_file.c
new file mode 100644
index 0000000..e1abcbd
--- /dev/null
+++ b/tests/test_file.c
@@ -0,0 +1,183 @@
+/*
+ * Asterisk -- An open source telephony toolkit.
+ *
+ * Copyright (C) 2016, Digium, Inc.
+ *
+ * Kevin Harwell <kharwell at digium.com>
+ *
+ * See http://www.asterisk.org for more information about
+ * the Asterisk project. Please do not directly contact
+ * any of the maintainers of this project for assistance;
+ * the project provides a web site, mailing lists and IRC
+ * channels for your use.
+ *
+ * This program is free software, distributed under the terms of
+ * the GNU General Public License Version 2. See the LICENSE file
+ * at the top of the source tree.
+ */
+
+/*** MODULEINFO
+ <depend>TEST_FRAMEWORK</depend>
+ <support_level>core</support_level>
+ ***/
+
+#include "asterisk.h"
+
+ASTERISK_REGISTER_FILE()
+
+#include "asterisk/file.h"
+#include "asterisk/paths.h"
+#include "asterisk/test.h"
+#include "asterisk/module.h"
+#include "asterisk/strings.h"
+#include "asterisk/vector.h"
+
+#define FOUND -7
+
+AST_VECTOR(_filenames, struct ast_str *);
+
+static void rm_file(struct ast_str *filename)
+{
+ if (unlink(ast_str_buffer(filename))) {
+ ast_log(LOG_ERROR, "Unable to remove file: %s\n", ast_str_buffer(filename));
+ }
+
+ ast_free(filename);
+}
+
+static int test_files_destroy(struct ast_test *test, char *dir_name,
+ struct _filenames *filenames)
+{
+ int res;
+
+ if (filenames) {
+ AST_VECTOR_CALLBACK_VOID(filenames, rm_file);
+ AST_VECTOR_FREE(filenames);
+ }
+
+ if ((res = rmdir(dir_name)) < 0) {
+ ast_test_status_update(test, "Failed to remove directory: %s\n", dir_name);
+ }
+
+ return res;
+}
+
+static int test_files_create(struct ast_test *test, char *dir_name,
+ struct _filenames *filenames, int num)
+{
+ int i;
+
+ if (!(mkdtemp(dir_name))) {
+ ast_test_status_update(test, "Failed to create directory: %s\n", dir_name);
+ return -1;
+ }
+
+
+ AST_VECTOR_INIT(filenames, num);
+
+ /*
+ * Create "num" files under the specified directory
+ */
+ for (i = 0; i < num; ++i) {
+ int fd;
+ struct ast_str *filename = ast_str_create(32);
+
+ if (!filename) {
+ break;
+ }
+
+ ast_str_set(&filename, 0, "%s/XXXXXX", dir_name);
+
+ fd = mkstemp(ast_str_buffer(filename));
+ if (fd < 0) {
+ ast_test_status_update(test, "Failed to create file: %s\n",
+ ast_str_buffer(filename));
+ ast_free(filename);
+ break;
+ }
+ close(fd);
+
+ AST_VECTOR_APPEND(filenames, filename);
+ }
+
+ if (i != num) {
+ test_files_destroy(test, dir_name, filenames);
+ return -1;
+ }
+
+ return 0;
+}
+
+static char *test_files_get_one(struct _filenames *filenames, int num)
+{
+ /* Every file is in a directory and contains a '/' so okay to do this */
+ return strrchr(ast_str_buffer(
+ AST_VECTOR_GET(filenames, ast_random() % (num - 1))), '/') + 1;
+}
+
+static int handle_find_file(const char *dir_name, const char *filename, void *obj)
+{
+ /* obj contains the name of the file we are looking for */
+ return strcmp(obj, filename) ? 0 : FOUND;
+}
+
+AST_TEST_DEFINE(read_dirs_test)
+{
+ char tmp_dir[] = "/tmp/tmpdir.XXXXXX";
+ struct ast_str *tmp_sub_dir;
+ struct _filenames filenames;
+ enum ast_test_result_state res;
+ const int num_files = 10 + (ast_random() % 10); /* 10-19 random files */
+
+ switch (cmd) {
+ case TEST_INIT:
+ info->name = "read_dir_test";
+ info->category = "/main/file/";
+ info->summary = "Read a directory's content";
+ info->description = "Iterate over directories looking for a file.";
+ return AST_TEST_NOT_RUN;
+ case TEST_EXECUTE:
+ break;
+ }
+
+ /*
+ * We want to test recursively searching into a subdirectory, so
+ * create a top level tmp directory where we will start the search.
+ */
+ if (!(mkdtemp(tmp_dir))) {
+ ast_test_status_update(test, "Failed to create directory: %s\n", tmp_dir);
+ return AST_TEST_FAIL;
+ }
+
+ tmp_sub_dir = ast_str_alloca(32);
+ ast_str_set(&tmp_sub_dir, 0, "%s/XXXXXX", tmp_dir);
+
+ if (test_files_create(test, ast_str_buffer(tmp_sub_dir), &filenames, num_files)) {
+ test_files_destroy(test, tmp_dir, NULL);
+ return AST_TEST_FAIL;
+ }
+
+ res = ast_file_read_dirs(tmp_dir, handle_find_file, test_files_get_one(
+ &filenames, num_files), 2) == FOUND ? AST_TEST_PASS : AST_TEST_FAIL;
+
+ if (test_files_destroy(test, ast_str_buffer(tmp_sub_dir), &filenames) ||
+ test_files_destroy(test, tmp_dir, NULL)) {
+ res = AST_TEST_FAIL;
+ }
+
+ return res;
+}
+
+static int unload_module(void)
+{
+ AST_TEST_UNREGISTER(read_dirs_test);
+ return 0;
+}
+
+static int load_module(void)
+{
+ AST_TEST_REGISTER(read_dirs_test);
+ return AST_MODULE_LOAD_SUCCESS;
+}
+
+AST_MODULE_INFO_STANDARD(ASTERISK_GPL_KEY, "File test module");
--
To view, visit https://gerrit.asterisk.org/4223
To unsubscribe, visit https://gerrit.asterisk.org/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Id8f54689b1e2873e82a09d0d0d2faf41964e80ba
Gerrit-PatchSet: 7
Gerrit-Project: asterisk
Gerrit-Branch: 14
Gerrit-Owner: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
More information about the asterisk-commits
mailing list