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