[Asterisk-code-review] chan_sip, chan_iax2, chan_pjsip: Allow updating variable values. (...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_pjsip: Allow updating variable values.
......................................................................


Patch Set 6:

(3 comments)

I have a new patchset almost ready. I also modified the common function to report errors, and check if it can allocate the new variable BEFORE discarding the old one.

I wait instruction regarding chan_dahdi.

https://gerrit.asterisk.org/#/c/10930/6//COMMIT_MSG 
Commit Message:

https://gerrit.asterisk.org/#/c/10930/6//COMMIT_MSG@14 
PS6, Line 14: This change makes the code replace the old value with the new
            : one. The chan_dahdi driver already conforms to this, more logical,
            : behavior.
> Can you point out where in chan_dahdi this logic is similar?
In the bug report I started from there's a comment stating that there is a patch that fixed this for chan_dahdi, I actually took it for good without checking. My fault.

I can't actually find any code in chan_dahdi for this.

I can apply the same generic function in chan_dahdi too now, if it's ok, I'm unable to test that in any way though.

Should I apply the generic function to chan_dahdi too?


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

https://gerrit.asterisk.org/#/c/10930/2/channels/chan_iax2.c@13051 
PS2, Line 13051: 			} else if (!strcasecmp(v->name, "setvar")) {
               : 				char *varname = ast_strdupa(v->value);
               : 				char *varval;
               : 				if ((varval = strchr(varname, '='))) {
               : 					*varval++ = '\0';
               : 					for (cur = peer->vars; cur; prev = cur, cur = cur->next) {
               : 						if(!strcmp(varname, cur->name)) {
               : 							/* for duplicate entries use last in the user */
               : 							if(prev == NULL)
               : 								peer->vars = cur->next;
               : 							else
               : 								prev->next = cur->next;
               : 							ast_free(cur);
               : 
               : 							break;
               : 						}
               : 					}
               : 					if((tmpvar = ast_variable_new(varname, varval, ""))) {
               : 						tmpvar->next = peer->vars;
               : 						peer->vars = tmpvar;
               : 					}
               : 				}
> Unless I'm mistaken "setvar" was not valid for peers previously, only for users. […]
Ack, next patch set will fix this.


https://gerrit.asterisk.org/#/c/10930/2/channels/chan_iax2.c@13217 
PS2, Line 13217: 					for (cur = user->vars; cur; prev = cur, cur = cur->next) {
               : 						if(!strcmp(varname, cur->name)) {
               : 							/* for duplicate entries use last in the user */
               : 							if(prev == NULL)
               : 								user->vars = cur->next;
               : 							else
               : 								prev->next = cur->next;
               : 
               : 							ast_free(cur);
               : 							break;
               : 						}
               : 					}
> Agree. Will be working on this.
Ack, next patch set will fix this.



-- 
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: 6
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 C. Colp <jcolp at digium.com>
Gerrit-Reviewer: Sean Bright <sean.bright at gmail.com>
Gerrit-Comment-Date: Thu, 14 Mar 2019 18:31:59 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Sean Bright <sean.bright at gmail.com>
Comment-In-Reply-To: Joshua C. Colp <jcolp at digium.com>
Comment-In-Reply-To: Guido Falsi <madpilot at freebsd.org>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20190314/4194a7d2/attachment.html>