[asterisk-dev] [Code Review] No review yet - Set channel variables with setvar= when manager user originates calls

[mjordan]

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



/trunk/include/asterisk/config.h
<https://reviewboard.asterisk.org/r/1412/#comment8335>

    Blob



/trunk/include/asterisk/config.h
<https://reviewboard.asterisk.org/r/1412/#comment8333>

    Make the in parameter const



/trunk/main/config.c
<https://reviewboard.asterisk.org/r/1412/#comment8336>

    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.



/trunk/main/config.c
<https://reviewboard.asterisk.org/r/1412/#comment8332>

    This is already in config.c, so your comment isn't needed



/trunk/main/config.c
<https://reviewboard.asterisk.org/r/1412/#comment8344>

    Remove the spaces preceeding the ';'



/trunk/main/config.c
<https://reviewboard.asterisk.org/r/1412/#comment8334>

    Blob



/trunk/main/manager.c
<https://reviewboard.asterisk.org/r/1412/#comment8337>

    Remove this comment and print out the channel variables if they exist.



/trunk/main/manager.c
<https://reviewboard.asterisk.org/r/1412/#comment8339>

    This comment probably isn't needed as it restates the previous comment.



/trunk/main/manager.c
<https://reviewboard.asterisk.org/r/1412/#comment8342>

    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.



/trunk/main/manager.c
<https://reviewboard.asterisk.org/r/1412/#comment8340>

    This comment isn't needed, since you aren't copying but appending.



/trunk/main/manager.c
<https://reviewboard.asterisk.org/r/1412/#comment8343>

    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.



/trunk/main/manager.c
<https://reviewboard.asterisk.org/r/1412/#comment8345>

    The setvar variable needs to be documented in manager.conf, and most likely in CHANGES.txt



/trunk/main/manager.c
<https://reviewboard.asterisk.org/r/1412/#comment8346>

    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.



/trunk/main/manager.c
<https://reviewboard.asterisk.org/r/1412/#comment8347>

    Memory leak - the user channel variables are never disposed of.


- mjordan


On Sept. 6, 2011, 9:35 a.m., Olle E Johansson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1412/
> -----------------------------------------------------------
> 
> (Updated Sept. 6, 2011, 9:35 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/include/asterisk/config.h 334444 
>   /trunk/main/config.c 334444 
>   /trunk/main/manager.c 334444 
> 
> 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/20110907/21573ba6/attachment-0001.htm>