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

Michal Židek mzidek at redhat.com
Fri Oct 26 10:21:42 UTC 2012


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


-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Include-talloc-log-in-our-debug-facility.patch
Type: text/x-patch
Size: 19384 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20121026/ee00337d/attachment.bin>


More information about the sssd-devel mailing list