[asterisk-dev] [Code Review]: named_acls: Named ACLs - a system for creating and applying ACLs with named profiles which can be shared
jrose
reviewboard at asterisk.org
Fri Jun 8 15:40:14 CDT 2012
> On June 7, 2012, 12:14 p.m., Terry Wilson wrote:
> > /trunk/channels/chan_h323.c, line 1491
> > <https://reviewboard.asterisk.org/r/1978/diff/1/?file=28602#file28602line1491>
> >
> > According to the documentation, shouldn't the option just be "acl"? Seems to be the way you did it for the others.
Yeah, that's just an oversight from a while back when the option was 'nacl' instead of 'acl'. Just missed it there.
> On June 7, 2012, 12:14 p.m., Terry Wilson wrote:
> > /trunk/channels/chan_sip.c, lines 28941-28943
> > <https://reviewboard.asterisk.org/r/1978/diff/1/?file=28605#file28605line28941>
> >
> > 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.
Zero length strings are fine. I was just trying to match the odd little difference with the permit/deny options there which also aren't present elsewhere. I went ahead and removed the condition.
> On June 7, 2012, 12:14 p.m., Terry Wilson wrote:
> > /trunk/channels/chan_sip.c, lines 28953-28955
> > <https://reviewboard.asterisk.org/r/1978/diff/1/?file=28605#file28605line28953>
> >
> > Same.
check
> On June 7, 2012, 12:14 p.m., Terry Wilson wrote:
> > /trunk/include/asterisk/acl.h, line 271
> > <https://reviewboard.asterisk.org/r/1978/diff/1/?file=28613#file28613line271>
> >
> > s/an named_acl/a named acl/
check
> On June 7, 2012, 12:14 p.m., Terry Wilson wrote:
> > /trunk/main/named_acl.c, lines 184-190
> > <https://reviewboard.asterisk.org/r/1978/diff/1/?file=28618#file28618line184>
> >
> > 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.
>
> Mark Michelson wrote:
> That will only work if OBJ_KEY is passed as a flag to ao2_find and the hash and comparison functions are written to understand that they may be given object keys instead of objects. As currently written, this suggested change can't be made.
>
> Also, I much prefer having the assignment and if statements be on separate lines, but that's just me :)
>
> Terry Wilson wrote:
> The named_acl_find does an ao2 find with a tmp struct. No OBJ_KEY required here or there. named_acl_find could be modified to use OBJ_KEY if the cmp_fn was modified to support it, but it shouldn't be required.
>
> Mark Michelson wrote:
> Oh I misread your comment. I didn't see you were using named_acl_find. I thought you were calling ao2_find directly :)
Aside from that, I do prefer having the condition and the assignment on separate lines.
> On June 7, 2012, 12:14 p.m., Terry Wilson wrote:
> > /trunk/main/named_acl.c, line 212
> > <https://reviewboard.asterisk.org/r/1978/diff/1/?file=28618#file28618line212>
> >
> > 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.
check. Except for the callback part since I don't think that's there yet.
Also did Josh's request of checking for not OK and returning instead to reduce indentation.
> On June 7, 2012, 12:14 p.m., Terry Wilson wrote:
> > /trunk/main/named_acl.c, lines 31-35
> > <https://reviewboard.asterisk.org/r/1978/diff/1/?file=28618#file28618line31>
> >
> > 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?
I've been working on cutting these down a bit.
> On June 7, 2012, 12:14 p.m., Terry Wilson wrote:
> > /trunk/main/named_acl.c, lines 251-255
> > <https://reviewboard.asterisk.org/r/1978/diff/1/?file=28618#file28618line251>
> >
> > 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.
check to both things
> On June 7, 2012, 12:14 p.m., Terry Wilson wrote:
> > /trunk/main/named_acl.c, line 303
> > <https://reviewboard.asterisk.org/r/1978/diff/1/?file=28618#file28618line303>
> >
> > 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.
Yeah, I was following the example set by manager, which I think was actually reimplemented and the code was just left in tact for whatever reason. On it.
Got it, though the command is only accessible through reload acl now. I guess it doesn't make a difference and doing things the way we do them more recently is always proper.
- jrose
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/1978/#review6405
-----------------------------------------------------------
On June 8, 2012, 2:04 p.m., jrose wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1978/
> -----------------------------------------------------------
>
> (Updated June 8, 2012, 2:04 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 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/20120608/a69a2462/attachment-0001.htm>
More information about the asterisk-dev
mailing list