[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 12:03:01 CDT 2012

> On July 5, 2012, 10:14 a.m., Mark Michelson wrote:
> > /trunk/main/named_acl.c, line 75
> > <https://reviewboard.asterisk.org/r/1978/diff/3/?file=29360#file29360line75>
> >
> >     a/an/a/

check

> On July 5, 2012, 10:14 a.m., Mark Michelson wrote:
> > /trunk/main/named_acl.c, lines 456-457
> > <https://reviewboard.asterisk.org/r/1978/diff/3/?file=29360#file29360line456>
> >
> >     named_acl is never unreffed.

RAII_VAR is magical

> On July 5, 2012, 10:14 a.m., Mark Michelson wrote:
> > /trunk/main/named_acl.c, line 335
> > <https://reviewboard.asterisk.org/r/1978/diff/3/?file=29360#file29360line335>
> >
> >     named_acl is never unreffed.

RAII_VAR(struct named_acl *, named_acl, NULL, ao2_cleanup);

I don't understand this as well as Terry does (I think it just runs ao2_cleanup on the variable when it loses scope), but RAII_VAR is magical.

> On July 5, 2012, 10:14 a.m., Mark Michelson wrote:
> > /trunk/main/named_acl.c, lines 368-370
> > <https://reviewboard.asterisk.org/r/1978/diff/3/?file=29360#file29360line368>
> >
> >     Should this same check be applied for realtime named ACLs?

No, I don't believe that's necessary.  The only way to have a named ACL is if it has rules and if it has improperly written rules, that will reject the whole thing and issue different errors.

This does sort of make me wonder what will happen if I make the realtime named ACL fields nullable though, so I'll check on that.

> On July 5, 2012, 10:14 a.m., Mark Michelson wrote:
> > /trunk/main/acl.c, line 543
> > <https://reviewboard.asterisk.org/r/1978/diff/3/?file=29355#file29355line543>
> >
> >     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.

Ah, yep, that's right.

fixed.

> On July 5, 2012, 10:14 a.m., Mark Michelson wrote:
> > /trunk/main/config.c, lines 776-777
> > <https://reviewboard.asterisk.org/r/1978/diff/3/?file=29357#file29357line776>
> >
> >     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.

cleaned that up at least.

> On July 5, 2012, 10:14 a.m., Mark Michelson wrote:
> > /trunk/main/acl.c, line 535
> > <https://reviewboard.asterisk.org/r/1978/diff/3/?file=29355#file29355line535>
> >
> >     s/to get//

check

> On July 5, 2012, 10:14 a.m., Mark Michelson wrote:
> > /trunk/main/acl.c, line 527
> > <https://reviewboard.asterisk.org/r/1978/diff/3/?file=29355#file29355line527>
> >
> >     This declaration of "current" shadows a previous declaration. My recommendation is to just remove this declaration.

removed redundant declaration.  Sometimes I wonder why C lets people do stuff like that -_-

> On July 5, 2012, 10:14 a.m., Mark Michelson wrote:
> > /trunk/main/acl.c, lines 509-510
> > <https://reviewboard.asterisk.org/r/1978/diff/3/?file=29355#file29355line509>
> >
> >     There's no need to do this since the acl was allocated with ast_calloc().

check

> On July 5, 2012, 10:14 a.m., Mark Michelson wrote:
> > /trunk/main/acl.c, lines 493-494
> > <https://reviewboard.asterisk.org/r/1978/diff/3/?file=29355#file29355line493>
> >
> >     I don't understand this comment. The part that gets me most is the part about "the return" since this function returns void.

Leftover comment from a time when this was not the case. It was originally based on ast_append_ha, and it originally worked on an ast_acl struct which had a next field.  When I changed to the linkedlist API, a lot of that changed.

Comment removed.

> On July 5, 2012, 10:14 a.m., Mark Michelson wrote:
> > /trunk/main/acl.c, lines 519-524
> > <https://reviewboard.asterisk.org/r/1978/diff/3/?file=29355#file29355line519>
> >
> >     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.

check

> On July 5, 2012, 10:14 a.m., Mark Michelson wrote:
> > /trunk/main/acl.c, line 484
> > <https://reviewboard.asterisk.org/r/1978/diff/3/?file=29355#file29355line484>
> >
> >     Use ast_copy_string instead of strncpy

check

> On July 5, 2012, 10:14 a.m., Mark Michelson wrote:
> > /trunk/main/named_acl.c, lines 483-504
> > <https://reviewboard.asterisk.org/r/1978/diff/3/?file=29360#file29360line483>
> >
> >     This does not list realtime named ACLs. Is this intentional?

Well, I didn't intend for this to work otherwise and this was also written before I added realtime stuff. As far as I'm aware most things are like this though.  I know sip show peers doesn't show realtime peers for instance.

> On July 5, 2012, 10:14 a.m., Mark Michelson wrote:
> > /trunk/main/named_acl.c, line 489
> > <https://reviewboard.asterisk.org/r/1978/diff/3/?file=29360#file29360line489>
> >
> >     ACL instead of acl

fixed

Actually I fixed this a while ago apparently.

> On July 5, 2012, 10:14 a.m., Mark Michelson wrote:
> > /trunk/main/acl.c, lines 314-318
> > <https://reviewboard.asterisk.org/r/1978/diff/3/?file=29355#file29355line314>
> >
> >     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);

fixed

> On July 5, 2012, 10:14 a.m., Mark Michelson wrote:
> > /trunk/include/asterisk/acl.h, lines 159-164
> > <https://reviewboard.asterisk.org/r/1978/diff/3/?file=29352#file29352line159>
> >
> >     This should be removed prior to merging.

Will do. Until the whole thing is done though, I'm leaving it.

> On July 5, 2012, 10:14 a.m., Mark Michelson wrote:
> > /trunk/include/asterisk/acl.h, lines 61-67
> > <https://reviewboard.asterisk.org/r/1978/diff/3/?file=29352#file29352line61>
> >
> >     The "brief" tag should contain a small description of the item, not the entire exlanation.

trimmed and split

> On July 5, 2012, 10:14 a.m., Mark Michelson wrote:
> > /trunk/configs/acl.conf.sample, line 66
> > <https://reviewboard.asterisk.org/r/1978/diff/3/?file=29349#file29349line66>
> >
> >     Got a red blob here.

Caught that shortly after I posted the review

> On July 5, 2012, 10:14 a.m., Mark Michelson wrote:
> > /trunk/configs/acl.conf.sample, line 20
> > <https://reviewboard.asterisk.org/r/1978/diff/3/?file=29349#file29349line20>
> >
> >     s/used/use/

check

> On July 5, 2012, 10:14 a.m., Mark Michelson wrote:
> > /trunk/include/asterisk/acl.h, lines 70-71
> > <https://reviewboard.asterisk.org/r/1978/diff/3/?file=29352#file29352line70>
> >
> >     Use doxygen-style comments for these options.

check

> On July 5, 2012, 10:14 a.m., Mark Michelson wrote:
> > /trunk/main/acl.c, line 541
> > <https://reviewboard.asterisk.org/r/1978/diff/3/?file=29355#file29355line541>
> >
> >     If the errors are going to have specific meanings behind them, then I suggest defining those in an enum instead of using magic numbers.

Actually, I didn't end up doing anything with this, so I think I'll just make it a flag.

- jrose

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

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/481ae82d/attachment-0001.htm>