[Asterisk-code-review] res agi: Set audio format of EAGI audio via new EAGI AUDIO F... (asterisk[master])

[Joshua Colp]

Joshua Colp has posted comments on this change.

Change subject: res_agi: Set audio format of EAGI audio via new EAGI_AUDIO_FORMAT variable
......................................................................


Patch Set 1: Code-Review-1

(8 comments)

https://gerrit.asterisk.org/#/c/3325/1//COMMIT_MSG
Commit Message:

Line 14: ASTERISK-26124
You can add the #close tag here since it closes the issue.


https://gerrit.asterisk.org/#/c/3325/1/CHANGES
File CHANGES:

Line 2833:  * The audio format output on file descriptor 3 when using EAGI can now be
This needs to be placed into the 14 to 15 section, since 14 has been created.


https://gerrit.asterisk.org/#/c/3325/1/res/res_agi.c
File res/res_agi.c:

Line 4236: 	char *ret;
This should have a better name - such as requested_format_name


Line 4237: 	char tempstr[1024] = "";
You don't need a temporary buffer.


Line 4246: 	pbx_retrieve_variable(chan, "EAGI_AUDIO_FORMAT", &ret, tempstr, sizeof(tempstr), NULL);
You can use pbx_builtin_getvar_helper to get the variable instead and not need a temporary buffer


Line 4247: 	ast_verb(3, "EAGI_AUDIO_FORMAT = %s\n", tempstr);
This log message isn't really useful standalone, it doesn't include the channel name for example


Line 4249: 	if (requested_format == NULL) {
if (!requested_format) {


Line 4256: 		ast_log(LOG_WARNING, "Unable to set channel '%s' to requested mode\n", ast_channel_name(chan));
In the case of getting the format from the cache you will leak a reference to the format. You will need to decrement it using ao2_ref once done, and in the case of using ast_format_slin you will need to bump it up in your requested_format = ast_format_slin assignment using ao2_bump(ast_format_slin) otherwise you would decrement it too much and it would get prematurely freed


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I188815b0cb47b7979139151c4e5211412fd69924
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: John Fawcett <john at voipsupport.it>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-HasComments: Yes