[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 4: Code-Review-1

(8 comments)

https://gerrit.asterisk.org/#/c/8209/4/include/asterisk/file.h
File include/asterisk/file.h:

https://gerrit.asterisk.org/#/c/8209/4/include/asterisk/file.h@447
PS4, Line 447:  * \retval 1 if not supported
             :  * \retval 0 if supported
             :  */
             : int ast_is_file_ext_supported(const char *file_ext);
The return value is opposite of what the function name implies.  Avoid confusion and make return what someone would expect.

if (ast_is_file_ext_supported(ext)) {
   ext is supported.
}


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

https://gerrit.asterisk.org/#/c/8209/4/include/asterisk/mod_format.h@47
PS4, Line 47: 	char mime_types[80]; /*!< MIME Types related to the format (separated by | if more than one)*/
> Some kinda delimiter will be needed to support cases like : https://gerrit.
* 80 should be enough.  The contents of the struct is constant and the compiler will warn if you try to assign a longer string into the fixed size where it is initialized.

* This patch cannot go anywhere but the master branch because adding anything to this struct breaks ABI.


https://gerrit.asterisk.org/#/c/8209/4/main/file.c
File main/file.c:

https://gerrit.asterisk.org/#/c/8209/4/main/file.c@356
PS4, Line 356: static int mime_types_compare(const char *mime_types, const char *type)
This function is not needed.  The only difference between this function and the already existing exts_compare() function is the name.

How about renaming the old function to:
int type_is_in(const char *list, const char *type)


Also make use ast_strdupa() to dynamically allocate and copy 'list' to the stack:
char *stringp = ast_strdupa(list);


https://gerrit.asterisk.org/#/c/8209/4/main/file.c@1950
PS4, Line 1950: 			if (exts) {
> Not sure I get your suggestion here. 
It would avoid wasted processing to not pass in a NULL exts or check it for NULL first.  As it is you check it for every time through the loop.


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

https://gerrit.asterisk.org/#/c/8209/4/main/media_cache.c@146
PS4, Line 146: 		char temp_ext[PATH_MAX];
How about found_ext[] instead.


https://gerrit.asterisk.org/#/c/8209/4/main/media_cache.c@147
PS4, Line 147: 		strcpy(temp_ext, ext);
strcpy() could write beyond the buffer.  Use ast_string_copy().


https://gerrit.asterisk.org/#/c/8209/4/main/media_cache.c@162
PS4, Line 162: 					char *tmp = strsep(&exts, "|");
tmp is always a lousy name as it provides no meaningful information about what tmp is.

In this case ext or exten would be a better name.


https://gerrit.asterisk.org/#/c/8209/4/main/media_cache.c@164
PS4, Line 164: 					/* Not updating the original ext here to keep the key for media-cache same */
             : 					snprintf(temp_ext, strlen(temp_ext), ".%s", tmp);
Is having to truncate the extension a good thing?  It seems a bad thing to wind up renaming a file to something with a truncated format extension.



-- 
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: 4
Gerrit-Owner: Sean Bright <sean.bright at gmail.com>
Gerrit-Reviewer: Gaurav Khurana <gkhurana at godaddy.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: Sean Bright <sean.bright at gmail.com>
Gerrit-Comment-Date: Wed, 14 Feb 2018 23:17:44 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180214/51e436b8/attachment-0001.html>