[asterisk-dev] [Code Review] Dialplan function for manager account checks - AMI_CLIENT()

[Tilghman Lesher]

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



/trunk/main/manager.c
<https://reviewboard.asterisk.org/r/421/#comment5348>

    I'd tend to quote the name within the string, so if it was passed as a blank or with a leading or trailing space, that becomes more obvious.  For that matter, you could probably add an ast_strip after grabbing the argument, to ensure that any leading or trailing spaces are trimmed.



/trunk/main/manager.c
<https://reviewboard.asterisk.org/r/421/#comment5349>

    It's not necessary to set the buffer if you're returning -1:  it will be set as a condition of that return value.



/trunk/main/manager.c
<https://reviewboard.asterisk.org/r/421/#comment5350>

    ast_strip(), again



/trunk/main/manager.c
<https://reviewboard.asterisk.org/r/421/#comment5351>

    Again, setting the buffer is redundant with this return value.



/trunk/main/manager.c
<https://reviewboard.asterisk.org/r/421/#comment5352>

    I would suggest that you either 1) implement only the read2 variant, which uses a dynamic string buffer, or 2) set the read_max element, to allow for maximum efficiency of the variable substitution routines.


- Tilghman


On 2010-07-17 04:01:35, Olle E Johansson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/421/
> -----------------------------------------------------------
> 
> (Updated 2010-07-17 04:01:35)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> Implementation of a dialplan function for checking manager accounts. I've made it extensible so that we can add other parameters when needed. Right now it only checks the number of logged in sessions for a manager account.
> 
> 
> Diffs
> -----
> 
>   /trunk/main/manager.c 277702 
> 
> Diff: https://reviewboard.asterisk.org/r/421/diff
> 
> 
> Testing
> -------
> 
> Tested on my Linux system and it reports manager logins properly - 0, 1 and 2 concurrent sessions...
> 
> 
> Thanks,
> 
> Olle E
> 
>