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

[Terry Wilson]

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



/trunk/channels/chan_h323.c
<https://reviewboard.asterisk.org/r/1978/#comment12003>

    According to the documentation, shouldn't the option just be "acl"? Seems to be the way you did it for the others.



/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/1978/#comment12007>

    Other modules don't do the ast_strlen_zero check. I'd just make sure that not having the check is safe for an empty string and do that. If it can't be made safe, then the other modules will need the check as well.



/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/1978/#comment12008>

    Same.



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

    s/an named_acl/a named acl/



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

    You can probably remove my comments from the patch. They were pretty informal and just intended to explain how the config options stuff worked. Otherwise, maybe just make them sound less like an informal tutorial?



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

    This should be able to be written as
    
    if (!(named_acl = named_acl_find(cfg->named_acl_list, name))) {
    
    and struct named_acl tmp removed.



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

    After review 1979 goes in, this should be:
    
    If (aco_process_config(&cfg_info, 1) == ACO_PROCESS_OK) {
    
    And shortly after that, I will probably just put in a post_apply callback where you can just put this all in a separate function.



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

    if (!(named_acl = named_acl_find(cfg->named_acl_list, name))) {
    
    and remove the tmp variable.
    
    Also, you can do
    
    RAII_VAR(struct named_acl *, named_acl, NULL, ao2_cleanup) at the top and not worry about having to unref it later.



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

    Pretty much every other core file that uses a config registers a reload handler with reload_classes[] in main/loader.c instead of having its own reload CLI command. I think we decided to move away from individual CLI reload commands a long time ago. You might check with some of the other devs to make sure I'm not lying to you.


- Terry


On June 7, 2012, 10:48 a.m., jrose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1978/
> -----------------------------------------------------------
> 
> (Updated June 7, 2012, 10:48 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.
> 
> 
> Diffs
> -----
> 
>   /trunk/CHANGES 368662 
>   /trunk/channels/chan_h323.c 368662 
>   /trunk/channels/chan_iax2.c 368662 
>   /trunk/channels/chan_mgcp.c 368662 
>   /trunk/channels/chan_sip.c 368662 
>   /trunk/channels/chan_skinny.c 368662 
>   /trunk/channels/chan_unistim.c 368662 
>   /trunk/configs/acl.conf.sample PRE-CREATION 
>   /trunk/configs/iax.conf.sample 368662 
>   /trunk/configs/manager.conf.sample 368662 
>   /trunk/configs/sip.conf.sample 368662 
>   /trunk/configs/skinny.conf.sample 368662 
>   /trunk/include/asterisk/acl.h 368662 
>   /trunk/include/asterisk/event_defs.h 368662 
>   /trunk/main/acl.c 368662 
>   /trunk/main/asterisk.c 368662 
>   /trunk/main/manager.c 368662 
>   /trunk/main/named_acl.c PRE-CREATION 
> 
> 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/20120607/9e2f638d/attachment-0001.htm>