[Asterisk-code-review] stasis recording/stored: remove calls to deprecated readdir ... (asterisk[14])
Kevin Harwell
asteriskteam at digium.com
Fri Oct 28 15:26:14 CDT 2016
Kevin Harwell has uploaded a new change for review. ( 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 the dependency on the dirent structure's d_type field as it is not
portable.
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
3 files changed, 223 insertions(+), 152 deletions(-)
git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/23/4223/1
diff --git a/include/asterisk/file.h b/include/asterisk/file.h
index c71866e..e601b8c 100644
--- a/include/asterisk/file.h
+++ b/include/asterisk/file.h
@@ -138,6 +138,44 @@
int ast_filecopy(const char *oldname, const char *newname, const char *fmt);
/*!
+ * \brief Given a filepath retrieve the base filename
+ *
+ * Attempts to retrieve the string following the final '/'. If no '/' is
+ * found then the original filepath is returned.
+ *
+ * \param filepath filename with path
+ * \return a pointer to the base filename
+ */
+const char *ast_file_basename(const char *filepath);
+
+/*!
+ * \brief Callback called for each file found when reading directories
+ * \param filepath the filname with full path
+ * \param obj user data object
+ * \return non-zero to stop reading, otherwise zero to continue
+ */
+typedef int (*ast_file_on_file)(const char *filepath, 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..74e370b 100644
--- a/main/file.c
+++ b/main/file.c
@@ -1095,6 +1095,132 @@
return filehelper(filename, filename2, fmt, ACTION_COPY);
}
+const char *ast_file_basename(const char *filepath) {
+ const char *res;
+
+ if (!(res = strrchr(filepath, '/'))) {
+ return filepath;
+ }
+
+ return ++res;
+}
+
+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) {
+ struct stat statbuf;
+
+ if (!strcmp(entry->d_name, ".") || !strcmp(entry->d_name, "..")) {
+ continue;
+ }
+
+ /*
+ * 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));
+ res = -1;
+ break;
+ }
+
+ /*
+ * It is tempting to use the d_type field found on the dirent
+ * structure. However, that field is not as portable as using
+ * the stat function.
+ */
+ if (S_ISREG(statbuf.st_mode)) {
+ /* If the handler returns non-zero then stop */
+ if ((res = on_file(ast_str_buffer(*path), obj))) {
+ break;
+ }
+ /* Otherwise move on to next item in directory */
+ continue;
+ }
+
+ if (!S_ISDIR(statbuf.st_mode)) {
+ 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 &&
+ ((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..130a5a7 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,46 @@
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 *filepath, void *obj)
+{
+ struct match_recording_data *data = obj;
+ int num;
+ const char *filename = ast_file_basename(filepath);
+
+ /* 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;
+ }
+
+ /* Otherwise we have a winner! */
+ data->file_with_ext = ast_strdup(filepath);
+ return 1;
}
/*!
@@ -161,46 +194,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,31 +258,14 @@
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 *filepath, void *obj)
{
- RAII_VAR(struct stasis_app_stored_recording *, recording, NULL,
- ao2_cleanup);
- const char *ext;
+ struct ao2_container *recordings = obj;
+ struct stasis_app_stored_recording *recording;
char *dot;
- ext = strrchr(filename, '.');
-
- if (!ext) {
- ast_verb(4, " Ignore file without extension: %s\n",
- filename);
- /* No file extension; not us */
- 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 */
+ /* Skip if it is not a recording */
+ if (!is_recording(filepath)) {
return 0;
}
@@ -289,10 +274,10 @@
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);
+
dot = strrchr(recording->file, '.');
*dot = '\0';
recording->format = dot + 1;
@@ -303,92 +288,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 +304,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;
}
--
To view, visit https://gerrit.asterisk.org/4223
To unsubscribe, visit https://gerrit.asterisk.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Id8f54689b1e2873e82a09d0d0d2faf41964e80ba
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: 14
Gerrit-Owner: Kevin Harwell <kharwell at digium.com>
More information about the asterisk-code-review
mailing list