[SSSD] [PATCH] new tool - sss_debuglevel

Jakub Hrozek jhrozek at redhat.com
Mon Sep 5 16:02:34 UTC 2011


On Mon, Aug 29, 2011 at 09:46:40AM +0200, Pavel Březina wrote:
> Dne 26.8.2011 19:20, Simo Sorce napsal(a):
> >On Mon, 2011-08-22 at 13:46 +0200, Jakub Hrozek wrote:
> >>On Tue, Aug 16, 2011 at 02:35:42PM +0200, Pavel Březina wrote:
> >>>sss_debuglevel - change the debug level on the fly
> >>>
> >>>https://fedorahosted.org/sssd/ticket/950
> >>>
> >>>Requires "New DEBUG facility" patches.
> >>
> >>>+errno_t set_debug_level(
> >>>+    struct debuglevel_tool_ctx *tool_ctx, int debug_to_set,
> >>>+    const char *config_file
> >>>+)
> >>>+{
> >>>+    int ret;
> >>>+    int err;
> >>>+    const char *values[2];
> >>>+    char **section = NULL;
> >>>+    TALLOC_CTX *ctx = talloc_new(NULL);
> >>>+
> >>
> >>Please name temporary context tmp_ctx so it is immediatelly clear that it
> >>is a just a temporary one.
> 
> Done.
> 
> >I don't like it either and I think we have in the guidelines something
> >about how we use parenthesis. Please follow the style used in the rest
> >of the sssd code when it comes to declarations.
> 
> Sorry about that! I'm used to very different coding style and it's
> not like you can switch to this one immediately. But I'll try harder
> to keep it on minimum next time. I hope it's good now.
> 

I think the style concerns have been addressed and the patch is ready to
be ACKed.

> Also, I noticed that I forgot to ask: There are two comments on
> lines 121 and 122 /* free pc_config_file? */ and /* free
> debug_as_string? */.
> I made some tests and it looks like resources allocated in popt
> should be freed manually but I'm not sure as I don't see it anywhere
> in the code.
> There are actually statements like this (monitor.c 2380, 2397):
> get string from popt
> poptFreeContext()
> use string from popt
> but no free()
> 
> Man page is not clear enough what poptFreeContext() does.

I ran a quick test with a minimal popt program and noticed there is a
leak if the variable you get from poptGetNextOpt():
==8292== 4 bytes in 1 blocks are definitely lost in loss record 2 of 2
==8292==    at 0x4A0649D: malloc (vg_replace_malloc.c:236)
==8292==    by 0x34720037F9: poptGetNextOpt (popt.c:918)

So it seems the best practice would be to set the variables to NULL on
definition time and free them on program exit.

By the way, there is another leak which we can't do much about except
filing a popt bug:
==8292==    at 0x4A06582: realloc (vg_replace_malloc.c:525)
==8292==    by 0x3472003583: poptGetNextOpt (popt.c:905)

The popt sources say:
t = realloc(t, strlen(t) + 1);      /* XXX memory leak, hard to plug */




More information about the sssd-devel mailing list