[asterisk-dev] [Code Review] Patch adds ability to ami hangup to specify a regex pattern to match channels against

[Mark Michelson]

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


I certainly like the idea here.

The only comment I have that directly pertains to this review is that I'm not a fan of the "UseRegex" header. Its name does not suggest that it means to apply the regex to the channel. I have two alternative suggestions:

1. My preferred way would be to create a header called "ChannelRegex". This would be an alternative to specifying a "Channel" header in the hangup action. If someone did something silly like specify both a "Channel" and a "ChannelRegex" header, then we could either just honor the "Channel" header (and emit a warning) or honor both.
2. My less preferred way would be to simply rename the "UseRegex" header you have defined here to "ChannelRegex". "ChannelRegex" would be a boolean that would let AMI know whether the data in the "Channel" header is a regex.

As a final note, whichever way you go, update the XML documentation at the top of manager.c.

Seeing this kind of change is kind of neat. It brings forth ideas of making more generic functions in channel.h such as "ast_channel_get_by_regex()" or some such. Similarly, it makes me think that several other manager commands could benefit from allowing regexes to be used for finding channels, but that would have to be examined on a case-by-case basis.


trunk/main/manager.c
<https://reviewboard.asterisk.org/r/1827/#comment10717>

    You can replace this with:
    
    useregex_bool = ast_true(useregex);
    
    This way allows people to also specify things like "true", "1", or "on" for the value.
    
    Note this advice may be irrelevant if the boolean header ends up not being part of the change.



trunk/main/manager.c
<https://reviewboard.asterisk.org/r/1827/#comment10718>

    A few things:
    
    1. Lock the channel before altering it here.
    2. This code is pretty much a copy of code down below. This seems like a candidate for some code that can be factored into its own function.
    3. Break the long ast_debug() statement into multiple lines.


- Mark


On March 22, 2012, 10:15 p.m., kobaz wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1827/
> -----------------------------------------------------------
> 
> (Updated March 22, 2012, 10:15 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> Patch adds ability to ami hangup to specify a regex pattern to match channels against
> 
> 
> This addresses bug ASTERISK-19575.
>     https://issues.asterisk.org/jira/browse/ASTERISK-19575
> 
> 
> Diffs
> -----
> 
>   trunk/main/manager.c 360227 
> 
> Diff: https://reviewboard.asterisk.org/r/1827/diff
> 
> 
> Testing
> -------
> 
> Action:   Hangup
> UseRegex: Yes
> Channel:  Local/foo.*
> 
> Action:   Hangup
> UseRegex: Yes
> Channel:  SIP/foo.*
> 
> 
> Thanks,
> 
> kobaz
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20120323/85ef67f0/attachment.htm>