[Asterisk-code-review] Add the ability to read the media file type from HTTP header... (asterisk[15])

[Richard Mudgett]

Richard Mudgett has posted comments on this change. ( https://gerrit.asterisk.org/8209 )

Change subject: Add the ability to read the media file type from HTTP header for playback
......................................................................


Patch Set 7: Code-Review-1

(3 comments)

https://gerrit.asterisk.org/#/c/8209/7/include/asterisk/mod_format.h
File include/asterisk/mod_format.h:

https://gerrit.asterisk.org/#/c/8209/7/include/asterisk/mod_format.h@47
PS7, Line 47: 	char mime_types[80]; /*!< MIME Types related to the format (separated by | if more than one)*/
Adding this breaks ABI by shifting everything down.  Also you cannot add it to the end of the struct as you have no way to determine if a third-party's precompiled module has the field.  If the module doesn't have the field you would be reading garbage.

Because you have to add something to this public struct this patch cannot go into anything other than master.


https://gerrit.asterisk.org/#/c/8209/7/main/media_cache.c
File main/media_cache.c:

https://gerrit.asterisk.org/#/c/8209/7/main/media_cache.c@133
PS7, Line 133: static void normalize_content_type_header(const char *content_type)
content_type cannot be const because it is modified by params.


https://gerrit.asterisk.org/#/c/8209/7/main/media_cache.c@177
PS7, Line 177: 				char *mime_type = ast_strdup(header->value);
             : 				if (mime_type) {
             : 					normalize_content_type_header(mime_type);
             : 					if (ast_get_extension_for_mime_type(mime_type, found_ext, sizeof(found_ext))) {
             : 						ext = found_ext;
             : 					}
             : 					ast_free(mime_type);
             : 				}
             : 			}
normalize_content_type_header() could return mime_type as an empty string which is not a good thing as it will match everything that has an empty mime list string.

char *mime_type;

mime_type = ast_strdup(header->value);
if (mime_type) {
	normalize_content_type_header(mime_type);
}
if (!ast_strlen_zero(mime_type)) {
	if (ast_get_extension_for_mime_type(mime_type, found_ext, sizeof(found_ext))) {
		ext = found_ext;
	}
}
ast_free(mime_type);



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

Gerrit-Project: asterisk
Gerrit-Branch: 15
Gerrit-MessageType: comment
Gerrit-Change-Id: I1b14692a49b2c1ac67688f58757184122e92ba89
Gerrit-Change-Number: 8209
Gerrit-PatchSet: 7
Gerrit-Owner: Sean Bright <sean.bright at gmail.com>
Gerrit-Reviewer: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: Gaurav Khurana <gkhurana at godaddy.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>
Gerrit-Reviewer: Matthew Fredrickson <creslin at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: Sean Bright <sean.bright at gmail.com>
Gerrit-Comment-Date: Mon, 30 Apr 2018 16:53:40 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180430/ae68da70/attachment.html>