[Asterisk-code-review] res_http_media_cache: Allow to customize parameters (asterisk[20])

[N A]

Attention is currently required from: Holger Hans Peter Freyther.

N A has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/19658 )

Change subject: res_http_media_cache: Allow to customize parameters
......................................................................


Patch Set 1: Code-Review-1

(9 comments)

Commit Message:

https://gerrit.asterisk.org/c/asterisk/+/19658/comment/3ecdfde8_cc00b946 
PS1, Line 10: but don't make it mandatory yet.
So it will be mandatory in the future?


Patchset:

PS1: 
This also needs a CHANGES entry


File res/res_http_media_cache.c:

https://gerrit.asterisk.org/c/asterisk/+/19658/comment/fefeff50_81243934 
PS1, Line 40: 				<configOption name="timeout_seconds">
You should specify the default as well, e.g. name="timeout_secs" default="180".


https://gerrit.asterisk.org/c/asterisk/+/19658/comment/be25e3ff_1855423d 
PS1, Line 52: 			</configObject>
You submitted 3 other reviews for this config, why are they not all part of the same commit?


https://gerrit.asterisk.org/c/asterisk/+/19658/comment/6ab743af_77b8d735 
PS1, Line 70: static int cache_curl_timeout = 180;
It would be nice if these were documented briefly (just a /*! \brief desc */ is fine)


https://gerrit.asterisk.org/c/asterisk/+/19658/comment/ae4113bb_c24fd103 
PS1, Line 360: 	curl_easy_setopt(curl, CURLOPT_USERAGENT, cache_curl_useragent != NULL ? cache_curl_useragent : AST_CURL_USER_AGENT);
You could use the S_OR macro here


https://gerrit.asterisk.org/c/asterisk/+/19658/comment/83d3e878_ae36cd6e 
PS1, Line 570: static int load_config(void)
It might be more logical to return 0 on success and 1 on failure, instead of 1 on success as you do here.


https://gerrit.asterisk.org/c/asterisk/+/19658/comment/33d62b36_4eceb040 
PS1, Line 578: 	if (cfg == CONFIG_STATUS_FILEMISSING) {
There should be some log message here (perhaps NOTICE) that the file is missing, otherwise there won't be any message as to why the module declined to load


https://gerrit.asterisk.org/c/asterisk/+/19658/comment/7a8bddfc_1732a8de 
PS1, Line 592: 		if (strcasecmp(cat, "general") == 0) {
use !strcasecmp instead of == 0



-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/19658
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 20
Gerrit-Change-Id: I2eb02ef44190e026716720419bcbdbcc8125777b
Gerrit-Change-Number: 19658
Gerrit-PatchSet: 1
Gerrit-Owner: Holger Hans Peter Freyther <automatic at freyther.de>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: N A <asterisk at phreaknet.org>
Gerrit-Attention: Holger Hans Peter Freyther <automatic at freyther.de>
Gerrit-Comment-Date: Fri, 09 Dec 2022 13:22:14 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20221209/d57440af/attachment.html>