[asterisk-dev] [Code Review]: Plenty of review done - Set channel variables with setvar= when manager user originates calls

[Olle E Johansson]

> On Sept. 7, 2011, 9:20 a.m., mjordan wrote:
> > /trunk/main/manager.c, line 6743
> > <https://reviewboard.asterisk.org/r/1412/diff/1/?file=19959#file19959line6743>
> >
> >     Memory leak - the user channel variables are never disposed of.

I don't agree - the user->chanvars is chained in to the tmpvar->next and is thus still in use.


> On Sept. 7, 2011, 9:20 a.m., mjordan wrote:
> > /trunk/include/asterisk/config.h, line 449
> > <https://reviewboard.asterisk.org/r/1412/diff/1/?file=19957#file19957line449>
> >
> >     Blob

Fixed.


> On Sept. 7, 2011, 9:20 a.m., mjordan wrote:
> > /trunk/main/config.c, line 521
> > <https://reviewboard.asterisk.org/r/1412/diff/1/?file=19958#file19958line521>
> >
> >     While this method works for your channel variables, it will not preserve all of the information for any ast_variable.  Since its name implies that it can be used to copy any ast_variable, it should copy all of the information in an ast_variable, as opposed to just the name and value.

Fixed. Added comment in doxygen about ignoring comments.


> On Sept. 7, 2011, 9:20 a.m., mjordan wrote:
> > /trunk/main/config.c, line 523
> > <https://reviewboard.asterisk.org/r/1412/diff/1/?file=19958#file19958line523>
> >
> >     This is already in config.c, so your comment isn't needed

Removed.


> On Sept. 7, 2011, 9:20 a.m., mjordan wrote:
> > /trunk/main/config.c, line 527
> > <https://reviewboard.asterisk.org/r/1412/diff/1/?file=19958#file19958line527>
> >
> >     Remove the spaces preceeding the ';'

Reclaimed spaces.


> On Sept. 7, 2011, 9:20 a.m., mjordan wrote:
> > /trunk/main/config.c, line 533
> > <https://reviewboard.asterisk.org/r/1412/diff/1/?file=19958#file19958line533>
> >
> >     Blob

Killed.


> On Sept. 7, 2011, 9:20 a.m., mjordan wrote:
> > /trunk/main/manager.c, line 1579
> > <https://reviewboard.asterisk.org/r/1412/diff/1/?file=19959#file19959line1579>
> >
> >     Remove this comment and print out the channel variables if they exist.

Done.


> On Sept. 7, 2011, 9:20 a.m., mjordan wrote:
> > /trunk/main/manager.c, line 4004
> > <https://reviewboard.asterisk.org/r/1412/diff/1/?file=19959#file19959line4004>
> >
> >     This comment probably isn't needed as it restates the previous comment.

Removed.


> On Sept. 7, 2011, 9:20 a.m., mjordan wrote:
> > /trunk/main/manager.c, line 4011
> > <https://reviewboard.asterisk.org/r/1412/diff/1/?file=19959#file19959line4011>
> >
> >     The comment is a little confusing - are the originate variables the variables from the manager command, or from the session variables that are used for the originate command?  If its the former, then this should be reworded to state 'manager command'; if its the latter then the comment is wrong about what variables are appended.

Changed, but not as you said. ;-)


> On Sept. 7, 2011, 9:20 a.m., mjordan wrote:
> > /trunk/main/manager.c, line 4015
> > <https://reviewboard.asterisk.org/r/1412/diff/1/?file=19959#file19959line4015>
> >
> >     This comment isn't needed, since you aren't copying but appending.

Removed.


> On Sept. 7, 2011, 9:20 a.m., mjordan wrote:
> > /trunk/main/manager.c, line 4017
> > <https://reviewboard.asterisk.org/r/1412/diff/1/?file=19959#file19959line4017>
> >
> >     This loop could be simplified to:
> >     
> >     for (v = vars; v->next; v = v->next);
> >     v->next = old;
> >     
> >     Note that the for loop contains no body but just iterates to the end of the session variables.  You can then point the next pointer to the manager command variables.

Done.


> On Sept. 7, 2011, 9:20 a.m., mjordan wrote:
> > /trunk/main/manager.c, line 6734
> > <https://reviewboard.asterisk.org/r/1412/diff/1/?file=19959#file19959line6734>
> >
> >     The setvar variable needs to be documented in manager.conf, and most likely in CHANGES.txt

Of course. THat was part of my original patch.


> On Sept. 7, 2011, 9:20 a.m., mjordan wrote:
> > /trunk/main/manager.c, line 6735
> > <https://reviewboard.asterisk.org/r/1412/diff/1/?file=19959#file19959line6735>
> >
> >     What is the actual format of your setvar?  It looks as if manager.conf would have something like:
> >     
> >     setvar => var1=value
> >     setvar => var2=value
> >     
> >     While there's nothing inherently wrong with this (save that the config can have multiple variables with the same name, which may affect config.c in ways I'm not familiar with), it seems a bit different then how this is usually handled in configs, at least that I'm familiar with.  I would have expected this to be delineated with some token, e.g.,
> >     
> >     setvar => var1=value,var2=value,var3=value
> >     
> >     Note that if the first approach is taken, a sample manager.conf is definitely needed.
> 
> mjordan wrote:
>     I reread this and this does match sip.conf's approach, so disregard.

No comments.


> On Sept. 7, 2011, 9:20 a.m., mjordan wrote:
> > /trunk/include/asterisk/config.h, line 452
> > <https://reviewboard.asterisk.org/r/1412/diff/1/?file=19957#file19957line452>
> >
> >     Make the in parameter const

Tried to fix, but got compile errors as I assign v = in and v is not a const.


- Olle E


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/1412/#review4255
-----------------------------------------------------------


On Sept. 15, 2011, 10:11 a.m., Olle E Johansson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1412/
> -----------------------------------------------------------
> 
> (Updated Sept. 15, 2011, 10:11 a.m.)
> 
> 
> Review request for Asterisk Developers and Tilghman Lesher.
> 
> 
> Summary
> -------
> 
> When the manager originates calls, there's no way to add channel variables. THis patch adds a setvar= option in manager.conf for a specific account, much like in sip.conf
> 
> This is different form variables in the originate in that these are set on every call, regardless of settings in the manager client software. Good for usage in combination with the manager event filters.
> 
> 
> This addresses bug 18143.
>     https://issues.asterisk.org/jira/browse/18143
> 
> 
> Diffs
> -----
> 
>   /trunk/CHANGES 336090 
>   /trunk/Makefile 336090 
>   /trunk/configs/manager.conf.sample 336090 
>   /trunk/include/asterisk/config.h 336090 
>   /trunk/main/config.c 336090 
>   /trunk/main/manager.c 336090 
> 
> Diff: https://reviewboard.asterisk.org/r/1412/diff
> 
> 
> Testing
> -------
> 
> In-house testing while developing. Channel variables added (proved by dumpchan() )
> 
> 
> Thanks,
> 
> Olle E
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20110915/dab10cd7/attachment-0001.htm>