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

2019-12-05 Thread 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  > wrote:
>
> On Wed, Dec 4, 2019 at 5:05 AM Jaco Kroon  > 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://asterisk.org
>
-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

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

2019-12-04 Thread Kevin Harwell 
On Wed, Dec 4, 2019 at 5:05 AM Joshua C. Colp  wrote:

> On Wed, Dec 4, 2019 at 5:05 AM Jaco Kroon  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://asterisk.org
-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

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

2019-12-04 Thread Joshua C. Colp 
On Wed, Dec 4, 2019 at 5:05 AM Jaco Kroon  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.

-- 
Joshua C. Colp
Senior Software Developer
Sangoma Technologies
Check us out at www.sangoma.com and www.asterisk.org
-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev