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