[SSSD] [PATCH] Include talloc log in our debug facility

Pavel Březina pbrezina at redhat.com
Fri Oct 26 08:49:56 UTC 2012


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)




More information about the sssd-devel mailing list