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

[Guido Falsi]

Guido Falsi 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:

(10 comments)

Going to update the patchset shortly.

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.
Ack


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.
Oops, the draft code got past me.


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 […]
Sorry, not a native english speaker, I keep maaking the it's/its error. Thanks for correcting me!


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. […]
Ack


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 a […]
I see.

I'm going to pass this a struct ast_variable **, which requires less modifications to consumers.


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.
Ack


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.
Ack


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


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.
Ack


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.
Ack



-- 
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: Sat, 23 Mar 2019 08:46:23 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Sean Bright <sean.bright at gmail.com>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20190323/3df8836a/attachment-0001.html>