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