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.
--
Simo Sorce * Red Hat, Inc * New York