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

Pavel Březina pbrezina at redhat.com
Fri Oct 26 10:34:27 UTC 2012


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!




More information about the sssd-devel mailing list