[SSSD] [PATCH] new tool - sss_debuglevel

Stephen Gallagher sgallagh at redhat.com
Tue Sep 6 18:25:11 UTC 2011


On Mon, 2011-09-05 at 18:02 +0200, Jakub Hrozek wrote:
> 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 */

Pushed to master.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20110906/0a1dd546/attachment.sig>


More information about the sssd-devel mailing list