[asterisk-dev] acl.c - option to turn off logging

[Jaco Kroon]

Hi Kevin, Joshua,

Your opinions on https://gerrit.asterisk.org/c/asterisk/+/13370 would be
greatly appreciated.

I ended up doing a combination of the described by creating and
_internal variant, where purpose basically becomes log_prefix ... and if
that's supplied it enables logging.

There are then two wrapper functions, one with identical interface to
existing, which if purpose is given as NULL it'll pass "" to _internal
(thus enabling logging as per current implementation), and _nolog
variant which will always pass log_prefix = NULL.


Kind Regards,
Jaco Kroon

On 2019/12/04 17:51, Kevin Harwell wrote:

> On Wed, Dec 4, 2019 at 5:05 AM Joshua C. Colp <jcolp at sangoma.com
> <mailto:jcolp at sangoma.com>> wrote:
>
>     On Wed, Dec 4, 2019 at 5:05 AM Jaco Kroon <jaco at uls.co.za
>     <mailto:jaco at uls.co.za>> wrote:
>
>         Hi All,
>
>         In ast_apply_acl (main/acl.c) there is two lines that's
>         issuing a LOG_WARNING when an ACL gets denied.
>
>         The first happens if the ACL is invalid.  I'm not too worried
>         about this specific one, it's probably a good thing if this
>         gets logged always.
>
>         The latter, in the case of AST_SENSE_DENY is a bit problematic
>         for me.  I've submitted patches now to use ACLs in
>         res_rtp_asterisk, and with large number of rejects this can
>         quickly spam the logs, and frankly, confuse consumers.
>
>         As I see it, there are two possible solutions:
>
>         Solution 1:
>
>         1.  Add AST_SENSE_INVALID as a possible return.
>         2.  Rename the current function to
>         ast_apply_acl_(silent|nolog), and remove the logging.
>         3.  Add a replacement ast_apply_acl function which will
>         generate the log entries as per current.
>
>         Solution 2:
>
>         Simply don't log at all if the purpose argument is NULL.
>
>         Solution two is the simpler fix, but it's probably also the
>         less ideal one.
>
>         The adding of the AST_SENSE_INVALID will also mean that the
>         replacement function will need to rewrite AST_SENSE_INVALID =>
>         AST_SENSE_DENY, or we need to audit all consumers of the
>         function (there fortunately isn't a great many of these) and
>         wherever ast_apply_acl(...) == AST_SENSE_DENY is found, it
>         should be rewritten as ast_apply_acl(...) != AST_SENSE_ALLOW.
>
>         Would dearly like some opinions on the matter.
>
>         PS:  The advantage for me on using ACL over HA is simply the
>         named ACL functionality, so in rtp.conf I can state ice_acl =
>         named_acl instead of having to embed the ACL into rtp.conf
>
>
>     I would prefer #1 however the existing function should behave as
>     it does now. Each consumer should not need to be touched, unless
>     they are to be switched to silent. We have an obligation to
>     maintain ABI and behavior of API functions as best we can in case
>     there are any outside consumers as well.
>
>
> Unfortunately due to how consumer possibly use of the current return
> value I think the only way to handle this is with #2. Or well by
> adding another function ast_apply_acl_silent, and removing the logging
> that is. Your new changes could call this function, and nothing else
> would be potentially affected.
>
> -- 
> Kevin Harwell
> Senior Software Developer
> Sangoma Technologies
> Check us out at: https://sangoma.com <https://sangoma.com/> &
> https://asterisk.org
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20191205/e86f538e/attachment.html>