[Asterisk-code-review] main/media cache: Provide an extension on the local file ass... (asterisk[master])

[Anonymous Coward]

Anonymous Coward #1000019 has submitted this change and it was merged.

Change subject: main/media_cache: Provide an extension on the local file associated with a URI
......................................................................


main/media_cache: Provide an extension on the local file associated with a URI

This patch does the following:

First, it addresses file extension handling in the media cache. The media core
in Asterisk is a bit interesting in that it wants:
 * A file to have an extension on it. That extension is used to associate the
   file with a defined format module.
 * The filename passed to the core to not have an extension on it. This allows
   the core to match the available file formats with the format a channel
   is capable of handling.

Unfortunately, this makes the current implementation a bit lacking in the media
cache. By default, we do not store the extension of a retrieved URI on the
local file that is created. As a result, the media core does not know what
format the file is, and the file is ignored. Modifying the file outside of the
media core is bad, as we would not be able to update the internal
ast_bucket_file's path.

At the same time, we do not want to pass the extension out in the file_path
parameter in ast_media_cache_retrieve. This parameter is intended to be fed
into the media core; if we passed the extension, all callers would have to
strip it off.

Thus, this patch does the following:
* If there is an extension specified in the URL, we append it to the local
  file name (if a preferred file name isn't specified), and we store that
  in the local file path.
* The extension, however, is stripped off of the file_path parameter passed
  back out of ast_media_cache_retrieve.

Second, this patch causes stale items to be completely removed from the system.
Prior to this patch, sound files could be orphaned due to the bucket
referencing the file being deleted, but the file itself not being removed. This
is now addressed by explicitly calling ast_bucket_file_delete on the
bucket_file when it is deemed to be stale. Note that this only happen when we
know we will attempt to retrieve the resource again.

Finally, this patch changes the AO2 container holding media items to just use
a regular mutex. The usage for this container already assumed it was a plain
mutex, and - given that retrieval of an item can cause it to be replaced in
the container - a mutex makes more sense than a read/write lock.

Change-Id: I51667fff86ae8d2e4a663555dfa85b11e935fe0f
---
M main/media_cache.c
1 file changed, 31 insertions(+), 11 deletions(-)

Approvals:
  Anonymous Coward #1000019: Verified
  Joshua Colp: Looks good to me, approved
  George Joseph: Looks good to me, but someone else must approve



diff --git a/main/media_cache.c b/main/media_cache.c
index 1f81e3a..9d68a10 100644
--- a/main/media_cache.c
+++ b/main/media_cache.c
@@ -189,23 +189,31 @@
 static void bucket_file_update_path(struct ast_bucket_file *bucket_file,
 	const char *preferred_file_name)
 {
-	if (ast_strlen_zero(preferred_file_name)) {
-		return;
-	}
+	char *ext;
 
-	if (!strcmp(bucket_file->path, preferred_file_name)) {
-		return;
-	}
+	if (!ast_strlen_zero(preferred_file_name) && strcmp(bucket_file->path, preferred_file_name)) {
+		/* Use the preferred file name if available */
 
-	rename(bucket_file->path, preferred_file_name);
-	ast_copy_string(bucket_file->path, preferred_file_name,
-		sizeof(bucket_file->path));
+		rename(bucket_file->path, preferred_file_name);
+		ast_copy_string(bucket_file->path, preferred_file_name,
+			sizeof(bucket_file->path));
+	} else if (!strchr(bucket_file->path, '.') && (ext = strrchr(ast_sorcery_object_get_id(bucket_file), '.'))) {
+		/* If we don't have a file extension and were provided one in the URI, use it */
+		char new_path[PATH_MAX];
+
+		ast_bucket_file_metadata_set(bucket_file, "ext", ext);
+
+		snprintf(new_path, sizeof(new_path), "%s%s", bucket_file->path, ext);
+		rename(bucket_file->path, new_path);
+		ast_copy_string(bucket_file->path, new_path, sizeof(bucket_file->path));
+	}
 }
 
 int ast_media_cache_retrieve(const char *uri, const char *preferred_file_name,
 	char *file_path, size_t len)
 {
 	struct ast_bucket_file *bucket_file;
+	char *ext;
 	SCOPED_AO2LOCK(media_lock, media_cache);
 
 	if (ast_strlen_zero(uri)) {
@@ -220,11 +228,18 @@
 	if (bucket_file) {
 		if (!ast_bucket_file_is_stale(bucket_file)) {
 			ast_copy_string(file_path, bucket_file->path, len);
+			if ((ext = strrchr(file_path, '.'))) {
+				*ext = '\0';
+			}
 			ao2_ref(bucket_file, -1);
+
+			ast_debug(5, "Returning media at local file: %s\n", file_path);
 			return 0;
 		}
 
-		/* Stale! Drop the ref, as we're going to retrieve it next. */
+		/* Stale! Remove the item completely, as we're going to replace it next */
+		ao2_unlink_flags(media_cache, bucket_file, OBJ_NOLOCK);
+		ast_bucket_file_delete(bucket_file);
 		ao2_ref(bucket_file, -1);
 	}
 
@@ -243,8 +258,13 @@
 	bucket_file_update_path(bucket_file, preferred_file_name);
 	media_cache_item_sync_to_astdb(bucket_file);
 	ast_copy_string(file_path, bucket_file->path, len);
+	if ((ext = strrchr(file_path, '.'))) {
+		*ext = '\0';
+	}
 	ao2_link_flags(media_cache, bucket_file, OBJ_NOLOCK);
 	ao2_ref(bucket_file, -1);
+
+	ast_debug(5, "Returning media at local file: %s\n", file_path);
 
 	return 0;
 }
@@ -692,7 +712,7 @@
 {
 	ast_register_atexit(media_cache_shutdown);
 
-	media_cache = ao2_container_alloc_options(AO2_ALLOC_OPT_LOCK_RWLOCK, AO2_BUCKETS,
+	media_cache = ao2_container_alloc_options(AO2_ALLOC_OPT_LOCK_MUTEX, AO2_BUCKETS,
 		media_cache_hash, media_cache_cmp);
 	if (!media_cache) {
 		return -1;

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

Gerrit-MessageType: merged
Gerrit-Change-Id: I51667fff86ae8d2e4a663555dfa85b11e935fe0f
Gerrit-PatchSet: 4
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Matt Jordan <mjordan at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: George Joseph <george.joseph at fairview5.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>
Gerrit-Reviewer: Matthew Fredrickson <creslin at digium.com>