[Freeipa-devel] [PATCH 0098] Log failures detected in CHECK() macro

[Petr Spacek]

On 12/13/2012 03:07 PM, Adam Tkac wrote:
> On Tue, Dec 04, 2012 at 01:24:39PM +0100, Petr Spacek wrote:
>> >On 11/22/2012 02:05 PM, Adam Tkac wrote:
>>> > >On Tue, Nov 20, 2012 at 02:44:49PM +0100, Petr Spacek wrote:
>>>> > >>Hello,
>>>> > >>
>>>> > >>     Log failures detected in CHECK() macro.
>>>> > >>
>>>> > >>     Function ldap_query() can return ISC_R_NOTFOUND legitimately.
>>>> > >>     For this and similar cases CHECK_CONDLOG macro was introduced.
>>>> > >>     It will not log if result != ISC_R_SUCCESS but == ignored_code.
>>>> > >>     Nested condition will be eliminated by optimizing compiler
>>>> > >>     in cases where ignored_code == ISC_R_SUCCESS.
>>>> > >>
>>>> > >>     Function add_soa_record() is now called only for zones to prevent
>>>> > >>     false error messages.
>>> > >
>>> > >Nack.
>>> > >
>>> > >I don't like second part of the patch much, it adds huge amount of logging
>>> > >and now we will log every error twice because we already log errors explicitly.
>>> > >
>>> > >In my opinion better will be to add new configuration option, for example
>>> > >"debug", and with this option we can emit log messages from CHECK macros (I
>>> > >haven't though about implementation details, yet). Otherwise we should avoid
>>> > >logging because it's useless to log all errors, they are expected in production
>>> > >environment.
>>> > >
>>> > >I also don't like CHECK_CONDLOG macro, it's not intuitive and with it we can end
>>> > >with so called spaghetti code... As I wrote above I would log every CHECK
>>> > >failure with debugging on.
>>> > >
>>> > >However the first patch of the patch is fine (the add_soa_record part).
>> >Ok, reworked patch is attached. Logging is enabled only if
>> >configuration option 'verbose_checks yes' is present. I
>> >decommissioned CHECK_CONDLOG(), so each request for non-existing
>> >record will log failure: not found (when verbose mode is enabled).
> This looks fine for me. In future we might consider to add some rate-limiting
> patch for log_error_position() calls because in production environment debug log
> can be too huge but this is not blocker for the patch.
>
> Ack
I would wait before we hit some really huge log, otherwise it is not worth to 
spend time on it...

Pushed to master and v2: 12850de76b6111c97ed7c40d85ddab6ee9fabe57

-- 
Petr^2 Spacek