Re: [PATCH] Security: key: keyring: fix code style issues

[James Kosin <jkosin@intcomgrp.com>]

See below my comments,

James Kosin

On 3/8/2010 1:20 PM, Chihau Chau wrote:
> From: Chihau Chau <chihau@gmail.com>
> 
> This fixes some code style issues like to include <linux/uaccess.h> instead
> <asm/uaccess.h> and to remove some innecessary braces.
> 
> Signed-off-by: Chihau Chau <chihau@gmail.com>
> ---
>  security/keys/keyring.c |   11 ++++-------
>  1 files changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/security/keys/keyring.c b/security/keys/keyring.c
> index e814d21..5a44965 100644
> --- a/security/keys/keyring.c
> +++ b/security/keys/keyring.c
> @@ -17,7 +17,7 @@
>  #include <linux/seq_file.h>
>  #include <linux/err.h>
>  #include <keys/keyring-type.h>
> -#include <asm/uaccess.h>
> +#include <linux/uaccess.h>
>  #include "internal.h"
>  
>  /*
> @@ -170,12 +170,10 @@ static void keyring_describe(const struct key *keyring, struct seq_file *m)
>  {
>  	struct keyring_list *klist;
>  
> -	if (keyring->description) {
> +	if (keyring->description)
>  		seq_puts(m, keyring->description);
> -	}
> -	else {
> +	else
>  		seq_puts(m, "[anon]");
> -	}

I'm just going to put my 2-cents in here.

a)  removing the braces doesn't change the functionality here; HOWEVER,
b)  it may introduce a serious BUG later.

The main reason is some people don't look carefully at code and often
add statements inside an if else block ...  and if not caught will
change the meaning of the statements and the order of execution.  The
worst possible case is if the else {} block is matchable to another if
statement above this... then the compiler never complains about the else
without an if test.

I'm okay with changing this to be like below with the {} matching and in
the same format if you like.

>  
>  	rcu_read_lock();
>  	klist = rcu_dereference(keyring->payload.subscriptions);
> @@ -775,8 +773,7 @@ int __key_link(struct key *keyring, struct key *key)
>  		smp_wmb();
>  		klist->nkeys++;
>  		smp_wmb();
> -	}
> -	else {
> +	} else {
>  		/* grow the key list */
>  		max = 4;
>  		if (klist)