On Fri, Oct 26, 2012 at 12:34:27PM +0200, Pavel Březina wrote:
On 10/26/2012 12:21 PM, Michal Židek wrote:
>On 10/26/2012 10:49 AM, Pavel Březina wrote:
>>On 10/24/2012 05:26 PM, Jakub Hrozek wrote:
>>>On Fri, Oct 19, 2012 at 01:07:36PM +0200, Michal Židek wrote:
>>>>On 10/18/2012 09:04 PM, Michal Židek wrote:
>>>>>On 10/15/2012 05:12 PM, Pavel Březina wrote:
>>>>>>On 10/15/2012 03:34 PM, Michal Židek wrote:
>>>>>>>This patch introduces use_talloc_logs function. After this
>>>>>>>function is
>>>>>>>called, talloc log messages are sended to DEBUG macro and
shown in
>>>>>>>our
>>>>>>>logs.
>>>>>>>
>>>>>>>https://fedorahosted.org/sssd/ticket/1495
>>>>>>>
>>>>>>>Patch is attached.
>>>>>>>
>>>>>>>Michal
>>>>>>
>>>>>>Hi,
>>>>>>I didn't do a comprehensive review yet, but I don't
really like the
>>>>>>way
>>>>>>you are using use_talloc_log(). I think it would be much better
to
>>>>>>create a function (say debug_init()) that would take care of
debug
>>>>>>initialization i.e. setting debug level and enabling talloc. It
>>>>>>should
>>>>>>basically replace debug_convert_old_level() and
>>>>>>CONVERT_AND_SET_DEBUG_LEVEL() calls.
>>>>>
>>>>>Ok. I replaced the debug_convert_old_level() and
>>>>>CONVERT_AND_SET_DEBUG_LEVEL() calls with new DEBUG_INIT() macro,
which
>>>>>also sets the talloc_log_fn properly.
>>>>>
>>>>>>
>>>>>>There are quite a few places where we are initializing our debug
>>>>>>facility so this would make it easier to maintain in the future.
>>>>>>
>>>>>
>>>>>I was also thinking about setting the debug_prg_name inside of this
>>>>>macro too. At first I thought it is a good idea, but now I don't
think
>>>>>so, because we would still need to create the string (prog. name)
>>>>>outside this macro. Now we do it in one operation with the
>>>>>assignment to
>>>>>debug_prg_name so we would actually need to do more than we do now.
As
>>>>>it is now, it looks more consistent to me.
>>>>>
>>>>>>The talloc logging itself looks al right, but can you prepend
>>>>>>"talloc: "
>>>>>>before the message?
>>>>>
>>>>>This is not needed. The messages are printed from talloc_log_fn
>>>>>function
>>>>>so they already look like this:
>>>>>
>>>>>(Thu Oct 18 20:12:21:494900 2012) [sss_debuglevel] [talloc_log_fn]
>>>>>(0x0010): talloc: access after free error - first free may be at
>>>>>src/tools/sss_debuglevel.c:94
>>>>>(Thu Oct 18 20:12:21:495177 2012) [sss_debuglevel] [talloc_log_fn]
>>>>>(0x0010): Bad talloc magic value - access after free
>>>>>
>>>>>
>>>>>New patch is in attachment.
>>>>>
>>>>>Thanks
>>>>>Michal
>>>>>
>>>>
>>>>I forgot to modify the test suite to use the new macro.
>>>>
>>>>New patch attached.
>>>>
>>>>
>>>>Michal
>>>
>>>These patches work for me, ack.
>>
>>Nack from me.
>>
>>It seems you've forgotten to edit one call to debug_covert_old_level()
>>in krb5_child-test.c:479.
>>
>>The code is fine.
>>
>>I really love it!
>>
>>[talloc_log_fn] (0x0010): talloc: access after free error - first free
>>may be at src/tools/sss_useradd.c:92
>>
>>(not a bug, I changed the code so it can happen)
>>
>
>Thanks for the review.
>
>New patch attached.
>
>Michal
Ack!
Pushed to master.