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

[Petr Spacek]

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).

-- 
Petr^2 Spacek
-------------- next part --------------
A non-text attachment was scrubbed...
Name: bind-dyndb-ldap-pspacek-0098-2-Add-option-to-log-all-failures-detected-in-CHECK-mac.patch
Type: text/x-patch
Size: 3422 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20121204/7352f70f/attachment.bin>