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

[jrose]

> On July 6, 2012, 11:31 a.m., Mark Michelson wrote:
> > /trunk/include/asterisk/acl.h, lines 305-318
> > <https://reviewboard.asterisk.org/r/1978/diff/5/?file=29652#file29652line305>
> >
> >     These two functions aren't used anywhere yet. Are these eventually intended to replace the current behavior of manager and chan_sip of performing an entire reload when named ACLs change?

That's sort of the idea, but whether or not we are going to do that ourselves instead of just providing the ability to use these functions to handle it for people who choose to make those modifications is still a little up in the air. 

There are actually much more complicated things that can be done right now with the subscription/events for named ACL changes. If someone implements partial changes to ACLs for instance, it could send the event with a payload indicated the name of the ACl that has changed. The subscriptions themselves could also be created in a way that the callback that handles the events sent would be able to know which object owned the list that was changed. These two functions are meant to be able to work with those concepts to make updates without reloads possible.

It doesn't look like that is something we are going to be able to do before Asterisk 11 hits code freeze though.  I'd like to leave these functions in the API in case someone wants to use them.


> On July 6, 2012, 11:31 a.m., Mark Michelson wrote:
> > /trunk/main/config.c, line 837
> > <https://reviewboard.asterisk.org/r/1978/diff/5/?file=29658#file29658line837>
> >
> >     Separate this into two lines and add curly braces.

Ah, I had a feeling I was missing something while cleaning that up.

check


> On July 6, 2012, 11:31 a.m., Mark Michelson wrote:
> > /trunk/include/asterisk/acl.h, lines 75-78
> > <https://reviewboard.asterisk.org/r/1978/diff/5/?file=29652#file29652line75>
> >
> >     It's certainly not required for you to change this, but this looks a touch awkward to me, given that I know what the AST_LIST_HEAD() macro expands to.
> >     
> >     This could be written as
> >     
> >     AST_LIST_HEAD(ast_acl_list, ast_acl);
> >     
> >     instead.
> >     
> >     This would mean that if you have an ast_acl_list *foo, then you could write list operations like:
> >     
> >     ast_acl *current;
> >     AST_LIST_TRAVERSE(foo, current, list){
> >     }
> >     
> >     instead of
> >     
> >     ast_acl *current;
> >     AST_LIST_TRAVERSE(&foo->list, current, list) {
> >     }

check


- jrose


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


On July 5, 2012, 4:31 p.m., jrose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1978/
> -----------------------------------------------------------
> 
> (Updated July 5, 2012, 4:31 p.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/CHANGES 369406 
>   /trunk/channels/chan_sip.c 369406 
>   /trunk/main/acl.c 369406 
>   /trunk/include/asterisk/event_defs.h 369406 
>   /trunk/include/asterisk/config.h 369406 
>   /trunk/include/asterisk/channel.h 369406 
>   /trunk/channels/sip/include/sip.h 369406 
>   /trunk/configs/acl.conf.sample PRE-CREATION 
>   /trunk/configs/extconfig.conf.sample 369406 
>   /trunk/configs/manager.conf.sample 369406 
>   /trunk/include/asterisk/acl.h 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 
> 
> 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/20120706/178a852f/attachment-0001.htm>