[asterisk-dev] [Code Review] named_acls: Named ACLs - a system for creating and applying ACLs with named profiles which can be shared

Thu Jul 5 10:14:34 CDT 2012

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


/trunk/configs/acl.conf.sample
<https://reviewboard.asterisk.org/r/1978/#comment12510>

    s/used/use/


/trunk/configs/acl.conf.sample
<https://reviewboard.asterisk.org/r/1978/#comment12511>

    Got a red blob here.


/trunk/include/asterisk/acl.h
<https://reviewboard.asterisk.org/r/1978/#comment12512>

    The "brief" tag should contain a small description of the item, not the entire exlanation.


/trunk/include/asterisk/acl.h
<https://reviewboard.asterisk.org/r/1978/#comment12513>

    Use doxygen-style comments for these options.


/trunk/include/asterisk/acl.h
<https://reviewboard.asterisk.org/r/1978/#comment12525>

    This should be removed prior to merging.


/trunk/main/acl.c
<https://reviewboard.asterisk.org/r/1978/#comment12514>

    I've seen this construct used a few places. Seems like you could simplify a bit by writing it as:
    
    current->acl = ast_named_acl_find(current->name, &current->realtime, &current->is_invalid);


/trunk/main/acl.c
<https://reviewboard.asterisk.org/r/1978/#comment12516>

    Use ast_copy_string instead of strncpy


/trunk/main/acl.c
<https://reviewboard.asterisk.org/r/1978/#comment12515>

    I don't understand this comment. The part that gets me most is the part about "the return" since this function returns void.


/trunk/main/acl.c
<https://reviewboard.asterisk.org/r/1978/#comment12517>

    There's no need to do this since the acl was allocated with ast_calloc().


/trunk/main/acl.c
<https://reviewboard.asterisk.org/r/1978/#comment12519>

    After the ast_append_ha call, you can unlock path->list and return. This would allow you to decrease the indentation of the else block by a level. You'd just have to declare tmp and list at the top of the function instead of down here.


/trunk/main/acl.c
<https://reviewboard.asterisk.org/r/1978/#comment12520>

    This declaration of "current" shadows a previous declaration. My recommendation is to just remove this declaration.


/trunk/main/acl.c
<https://reviewboard.asterisk.org/r/1978/#comment12521>

    s/to get//


/trunk/main/acl.c
<https://reviewboard.asterisk.org/r/1978/#comment12522>

    If the errors are going to have specific meanings behind them, then I suggest defining those in an enum instead of using magic numbers.


/trunk/main/acl.c
<https://reviewboard.asterisk.org/r/1978/#comment12523>

    I think this continue statement is in error. This continue will continue the list traversal, and I think what you meant for it to do is to continue the outer strsep loop. As it is right now, if you duplicate a named ACL, then the error message will be printed and you'll end up with two ACLs with the same name defined.


/trunk/main/config.c
<https://reviewboard.asterisk.org/r/1978/#comment12527>

    I know this code was originally from another source, but we should still apply our coding guidelines to it. There are many places here where multiple statements appear on a single line, for instance.


/trunk/main/named_acl.c
<https://reviewboard.asterisk.org/r/1978/#comment12528>

    a/an/a/


/trunk/main/named_acl.c
<https://reviewboard.asterisk.org/r/1978/#comment12524>

    named_acl is never unreffed.


/trunk/main/named_acl.c
<https://reviewboard.asterisk.org/r/1978/#comment12509>

    Should this same check be applied for realtime named ACLs?


/trunk/main/named_acl.c
<https://reviewboard.asterisk.org/r/1978/#comment12529>

    named_acl is never unreffed.


/trunk/main/named_acl.c
<https://reviewboard.asterisk.org/r/1978/#comment12508>

    This does not list realtime named ACLs. Is this intentional?


/trunk/main/named_acl.c
<https://reviewboard.asterisk.org/r/1978/#comment12507>

    ACL instead of acl


- Mark


On July 3, 2012, 10:46 a.m., jrose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1978/
> -----------------------------------------------------------
> 
> (Updated July 3, 2012, 10:46 a.m.)
> 
> 
> Review request for Asterisk Developers, Mark Michelson, Terry Wilson, and Olle E Johansson.
> 
> 
> Summary
> -------
> 
> This feature is based on oej's deluxepine (or something like that) branch with a similarly named feature.  ACLs are defined in acl.conf and can be used by pretty much anything that has ACL options permit/deny (acl='aclname').  acl= works similarly to permit= and deny= in that it simply appends to the working ACL, so they can be combined with other uses of permit/deny/acl.
> 
> Also in use in this patch are twilson's new config options.
> 
> Since named acls are duplicated when used in another configuration, configurations that use named acls need to be updated if acl.conf is reloaded. This is accomplished with a new event type and the consumption of that event is demonstrated currently only in manager.conf
> If this seems like a proper approach to this problem, that will be replicated across other consumers of named acls.
> 
> NOTE: This code is very much WIP and not intended for merging.
> 
> 
> Diffs
> -----
> 
>   /trunk/include/asterisk/config.h 369406 
>   /trunk/include/asterisk/event_defs.h 369406 
>   /trunk/main/acl.c 369406 
>   /trunk/main/asterisk.c 369406 
>   /trunk/main/config.c 369406 
>   /trunk/main/loader.c 369406 
>   /trunk/main/manager.c 369406 
>   /trunk/main/named_acl.c PRE-CREATION 
>   /trunk/configs/acl.conf.sample PRE-CREATION 
>   /trunk/configs/extconfig.conf.sample 369406 
>   /trunk/CHANGES 369406 
>   /trunk/configs/manager.conf.sample 369406 
>   /trunk/include/asterisk/acl.h 369406 
> 
> Diff: https://reviewboard.asterisk.org/r/1978/diff
> 
> 
> Testing
> -------
> 
> Various tests for configuring and using named acls were performed, and a task for writing comprehensive testsuite tests is in the queue.  Additionally, various means of reloading the configuration have been performed, and so far they pan out aside from a bug with an unchanged acl.conf which is a generic problem against config options accidentally introduced a little while back.
> 
> 
> Thanks,
> 
> jrose
> 
>

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