sss_debuglevel - change the debug level on the fly
https://fedorahosted.org/sssd/ticket/950
Requires "New DEBUG facility" patches.
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.
My only other comment is that I don't really like the style of the function definitions, but that's just a subjective thing.
I would appreciate a review from somebody else as I saw and commented on the patche before they went to the list (and it was the last to be done this way, time to take the training wheels off :-))
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.
My only other comment is that I don't really like the style of the function definitions, but that's just a subjective thing.
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.
In general never start a new line right after opening a parenthesis, I see it also in DEBUG statements and function calls later. And do not close them on a new line.
I would appreciate a review from somebody else as I saw and commented on the patche before they went to the list (and it was the last to be done this way, time to take the training wheels off :-))
Also please do not use constructs like this: + config_file = pc_config_file == NULL + ? talloc_strdup(ctx, CONFDB_DEFAULT_CONFIG_FILE) + : talloc_strdup(ctx, pc_config_file);
This way of doing conditionals just makes it hard to read w/o any benefit whatosver in this case, as you are goign to have a multilined statement anyway.
Plus never assign and compare like this in the same line, it just makes things confusing and calls for errors the next time someone has to change something.
Use proper if/else this way:
if (pc_config_file) { config_file = talloc_strdup(tmp_ctx, pc_config_file); } else { config_file = talloc_strdup(tmp_ctx, CONFDB_DEFAULT_CONFIG_FILE); }
Please alwyas use curly braces even if you have a single line in a if statement body.
if (foo) { bar; }
and not
if (foo) bar;
Simo.
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.
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.
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 */
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.
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Pavel Březina -
Simo Sorce -
Stephen Gallagher