[Asterisk-code-review] chan_sip, chan_iax2, chan_dahdi: Allow updating variable value. (...asterisk[master])

[Sean Bright]

Sean Bright has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/10930 )

Change subject: chan_sip, chan_iax2, chan_dahdi: Allow updating variable value.
......................................................................


Patch Set 7: Code-Review-1

(10 comments)

https://gerrit.asterisk.org/#/c/10930/7/channels/chan_iax2.c 
File channels/chan_iax2.c:

https://gerrit.asterisk.org/#/c/10930/7/channels/chan_iax2.c@3875 
PS7, Line 3875: 			for (v = peer->vars; v; v = v->next)
              : 				ast_cli(a->fd, "                 %s = %s\n", v->name, v->value);
'for' loop needs braces.


https://gerrit.asterisk.org/#/c/10930/7/include/asterisk/config.h 
File include/asterisk/config.h:

https://gerrit.asterisk.org/#/c/10930/7/include/asterisk/config.h@372 
PS7, Line 372: * \param variable list to update
             :  * \param variable name to search
             :  * \param variable new value
You need to replace 'variable' with the name of the actual parameter.


https://gerrit.asterisk.org/#/c/10930/7/include/asterisk/config.h@377 
PS7, Line 377:  * Goes through a lst of variables and updates it's value
lst -> list
it's -> its


https://gerrit.asterisk.org/#/c/10930/7/include/asterisk/config.h@382 
PS7, Line 382: int ast_var_update(struct ast_variable *vars, char *varname, char *varval);
Needs to take a 'struct ast_variable **' or return a 'struct variable *' - see note in config.c.

Also, make the arguments 'name' and 'value' for consistency with 'ast_variable_new'


https://gerrit.asterisk.org/#/c/10930/7/main/config.c 
File main/config.c:

https://gerrit.asterisk.org/#/c/10930/7/main/config.c@839 
PS7, Line 839: int ast_var_update(struct ast_variable *vars, char *varname, char *varval)
Because you could be adding or replacing the head of this list, this needs to either take a struct ast_variable **, or return a struct ast_variable *.


https://gerrit.asterisk.org/#/c/10930/7/main/config.c@841 
PS7, Line 841: 	struct ast_variable *cur = NULL, *prev = NULL, *tmpvar = NULL;
Defaulting cur and tmpvar is unnecessary as they will be initialized before use.


https://gerrit.asterisk.org/#/c/10930/7/main/config.c@843 
PS7, Line 843: if(!(tmpvar = ast_variable_new(varname, varval, ""))) {
             : 		return -1;
             : 	}
Split this into two lines. The assignment to tmpvar, and then the NULL check.


https://gerrit.asterisk.org/#/c/10930/7/main/config.c@848 
PS7, Line 848: 		if(!strcmp(varname, cur->name)) {
Space after 'if'


https://gerrit.asterisk.org/#/c/10930/7/main/config.c@850 
PS7, Line 850: 			if(prev == NULL)
Space after 'if' and this if/else block needs braces.


https://gerrit.asterisk.org/#/c/10930/7/main/config.c@861 
PS7, Line 861: 	vars = tmpvar;
See comment above, this is not going to update the caller's variable list.



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

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: Ie1897a96c82b8945e752733612ee963686f32839
Gerrit-Change-Number: 10930
Gerrit-PatchSet: 7
Gerrit-Owner: Guido Falsi <madpilot at freebsd.org>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Guido Falsi <madpilot at freebsd.org>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Sean Bright <sean.bright at gmail.com>
Gerrit-Comment-Date: Tue, 19 Mar 2019 14:05:57 +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/20190319/bfe289da/attachment.html>