[PATCH] selinux: Unify for- and while-loop style

[PATCH] selinux: Unify for- and while-loop style

[Vesa-Matti Kari]

Replace "thing != NULL" comparisons with just "thing" to make
the code look more uniform (mixed styles were used even in the
same source file).

Signed-off-by: Vesa-Matti Kari <vmkari@cc.helsinki.fi>
---
 security/selinux/ss/avtab.c       |    2 +-
 security/selinux/ss/conditional.c |   16 ++++++++--------
 security/selinux/ss/ebitmap.c     |    4 ++--
 security/selinux/ss/hashtab.c     |    6 +++---
 security/selinux/ss/services.c    |    8 ++++----
 security/selinux/ss/sidtab.c      |   12 ++++++------
 6 files changed, 24 insertions(+), 24 deletions(-)

diff --git a/security/selinux/ss/avtab.c b/security/selinux/ss/avtab.c
index e8ae812..1215b8e 100644
--- a/security/selinux/ss/avtab.c
+++ b/security/selinux/ss/avtab.c
@@ -229,7 +229,7 @@ void avtab_destroy(struct avtab *h)
 
 	for (i = 0; i < h->nslot; i++) {
 		cur = h->htable[i];
-		while (cur != NULL) {
+		while (cur) {
 			temp = cur;
 			cur = cur->next;
 			kmem_cache_free(avtab_node_cachep, temp);
diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
index f8c850a..4a4e35c 100644
--- a/security/selinux/ss/conditional.c
+++ b/security/selinux/ss/conditional.c
@@ -29,7 +29,7 @@ static int cond_evaluate_expr(struct policydb *p, struct cond_expr *expr)
 	int s[COND_EXPR_MAXDEPTH];
 	int sp = -1;
 
-	for (cur = expr; cur != NULL; cur = cur->next) {
+	for (cur = expr; cur; cur = cur->next) {
 		switch (cur->expr_type) {
 		case COND_BOOL:
 			if (sp == (COND_EXPR_MAXDEPTH - 1))
@@ -97,14 +97,14 @@ int evaluate_cond_node(struct policydb *p, struct cond_node *node)
 		if (new_state == -1)
 			printk(KERN_ERR "SELinux: expression result was undefined - disabling all rules.\n");
 		/* turn the rules on or off */
-		for (cur = node->true_list; cur != NULL; cur = cur->next) {
+		for (cur = node->true_list; cur; cur = cur->next) {
 			if (new_state <= 0)
 				cur->node->key.specified &= ~AVTAB_ENABLED;
 			else
 				cur->node->key.specified |= AVTAB_ENABLED;
 		}
 
-		for (cur = node->false_list; cur != NULL; cur = cur->next) {
+		for (cur = node->false_list; cur; cur = cur->next) {
 			/* -1 or 1 */
 			if (new_state)
 				cur->node->key.specified &= ~AVTAB_ENABLED;
@@ -128,7 +128,7 @@ int cond_policydb_init(struct policydb *p)
 static void cond_av_list_destroy(struct cond_av_list *list)
 {
 	struct cond_av_list *cur, *next;
-	for (cur = list; cur != NULL; cur = next) {
+	for (cur = list; cur; cur = next) {
 		next = cur->next;
 		/* the avtab_ptr_t node is destroy by the avtab */
 		kfree(cur);
@@ -139,7 +139,7 @@ static void cond_node_destroy(struct cond_node *node)
 {
 	struct cond_expr *cur_expr, *next_expr;
 
-	for (cur_expr = node->expr; cur_expr != NULL; cur_expr = next_expr) {
+	for (cur_expr = node->expr; cur_expr; cur_expr = next_expr) {
 		next_expr = cur_expr->next;
 		kfree(cur_expr);
 	}
@@ -155,7 +155,7 @@ static void cond_list_destroy(struct cond_node *list)
 	if (list == NULL)
 		return;
 
-	for (cur = list; cur != NULL; cur = next) {
+	for (cur = list; cur; cur = next) {
 		next = cur->next;
 		cond_node_destroy(cur);
 	}
@@ -291,7 +291,7 @@ static int cond_insertf(struct avtab *a, struct avtab_key *k, struct avtab_datum
 					goto err;
 				}
 				found = 0;
-				for (cur = other; cur != NULL; cur = cur->next) {
+				for (cur = other; cur; cur = cur->next) {
 					if (cur->node == node_ptr) {
 						found = 1;
 						break;
@@ -485,7 +485,7 @@ void cond_compute_av(struct avtab *ctab, struct avtab_key *key, struct av_decisi
 	if (!ctab || !key || !avd)
 		return;
 
-	for (node = avtab_search_node(ctab, key); node != NULL;
+	for (node = avtab_search_node(ctab, key); node;
 				node = avtab_search_node_next(node, key->specified)) {
 		if ((u16)(AVTAB_ALLOWED|AVTAB_ENABLED) ==
 		    (node->key.specified & (AVTAB_ALLOWED|AVTAB_ENABLED)))
diff --git a/security/selinux/ss/ebitmap.c b/security/selinux/ss/ebitmap.c
index ddc2754..68c7348 100644
--- a/security/selinux/ss/ebitmap.c
+++ b/security/selinux/ss/ebitmap.c
@@ -109,7 +109,7 @@ int ebitmap_netlbl_export(struct ebitmap *ebmap,
 	*catmap = c_iter;
 	c_iter->startbit = e_iter->startbit & ~(NETLBL_CATMAP_SIZE - 1);
 
-	while (e_iter != NULL) {
+	while (e_iter) {
 		for (i = 0; i < EBITMAP_UNIT_NUMS; i++) {
 			unsigned int delta, e_startbit, c_endbit;
 
@@ -197,7 +197,7 @@ int ebitmap_netlbl_import(struct ebitmap *ebmap,
 			}
 		}
 		c_iter = c_iter->next;
-	} while (c_iter != NULL);
+	} while (c_iter);
 	if (e_iter != NULL)
 		ebmap->highbit = e_iter->startbit + EBITMAP_SIZE;
 	else
diff --git a/security/selinux/ss/hashtab.c b/security/selinux/ss/hashtab.c
index 2e7788e..933e735 100644
--- a/security/selinux/ss/hashtab.c
+++ b/security/selinux/ss/hashtab.c
@@ -81,7 +81,7 @@ void *hashtab_search(struct hashtab *h, const void *key)
 
 	hvalue = h->hash_value(h, key);
 	cur = h->htable[hvalue];
-	while (cur != NULL && h->keycmp(h, key, cur->key) > 0)
+	while (cur && h->keycmp(h, key, cur->key) > 0)
 		cur = cur->next;
 
 	if (cur == NULL || (h->keycmp(h, key, cur->key) != 0))
@@ -100,7 +100,7 @@ void hashtab_destroy(struct hashtab *h)
 
 	for (i = 0; i < h->size; i++) {
 		cur = h->htable[i];
-		while (cur != NULL) {
+		while (cur) {
 			temp = cur;
 			cur = cur->next;
 			kfree(temp);
@@ -127,7 +127,7 @@ int hashtab_map(struct hashtab *h,
 
 	for (i = 0; i < h->size; i++) {
 		cur = h->htable[i];
-		while (cur != NULL) {
+		while (cur) {
 			ret = apply(cur->key, cur->datum, args);
 			if (ret)
 				return ret;
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index b52f923..5a0536b 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -356,7 +356,7 @@ static int context_struct_compute_av(struct context *scontext,
 			avkey.source_type = i + 1;
 			avkey.target_type = j + 1;
 			for (node = avtab_search_node(&policydb.te_avtab, &avkey);
-			     node != NULL;
+			     node;
 			     node = avtab_search_node_next(node, avkey.specified)) {
 				if (node->key.specified == AVTAB_ALLOWED)
 					avd->allowed |= node->datum.data;
@@ -1037,7 +1037,7 @@ static int security_compute_sid(u32 ssid,
 	/* If no permanent rule, also check for enabled conditional rules */
 	if (!avdatum) {
 		node = avtab_search_node(&policydb.te_cond_avtab, &avkey);
-		for (; node != NULL; node = avtab_search_node_next(node, specified)) {
+		for (; node; node = avtab_search_node_next(node, specified)) {
 			if (node->key.specified & AVTAB_ENABLED) {
 				avdatum = &node->datum;
 				break;
@@ -2050,7 +2050,7 @@ int security_set_bools(int len, int *values)
 			policydb.bool_val_to_struct[i]->state = 0;
 	}
 
-	for (cur = policydb.cond_list; cur != NULL; cur = cur->next) {
+	for (cur = policydb.cond_list; cur; cur = cur->next) {
 		rc = evaluate_cond_node(&policydb, cur);
 		if (rc)
 			goto out;
@@ -2102,7 +2102,7 @@ static int security_preserve_bools(struct policydb *p)
 		if (booldatum)
 			booldatum->state = bvalues[i];
 	}
-	for (cur = p->cond_list; cur != NULL; cur = cur->next) {
+	for (cur = p->cond_list; cur; cur = cur->next) {
 		rc = evaluate_cond_node(p, cur);
 		if (rc)
 			goto out;
diff --git a/security/selinux/ss/sidtab.c b/security/selinux/ss/sidtab.c
index a81ded1..e817989 100644
--- a/security/selinux/ss/sidtab.c
+++ b/security/selinux/ss/sidtab.c
@@ -43,7 +43,7 @@ int sidtab_insert(struct sidtab *s, u32 sid, struct context *context)
 	hvalue = SIDTAB_HASH(sid);
 	prev = NULL;
 	cur = s->htable[hvalue];
-	while (cur != NULL && sid > cur->sid) {
+	while (cur && sid > cur->sid) {
 		prev = cur;
 		cur = cur->next;
 	}
@@ -92,7 +92,7 @@ static struct context *sidtab_search_core(struct sidtab *s, u32 sid, int force)
 
 	hvalue = SIDTAB_HASH(sid);
 	cur = s->htable[hvalue];
-	while (cur != NULL && sid > cur->sid)
+	while (cur && sid > cur->sid)
 		cur = cur->next;
 
 	if (force && cur && sid == cur->sid && cur->context.len)
@@ -103,7 +103,7 @@ static struct context *sidtab_search_core(struct sidtab *s, u32 sid, int force)
 		sid = SECINITSID_UNLABELED;
 		hvalue = SIDTAB_HASH(sid);
 		cur = s->htable[hvalue];
-		while (cur != NULL && sid > cur->sid)
+		while (cur && sid > cur->sid)
 			cur = cur->next;
 		if (!cur || sid != cur->sid)
 			return NULL;
@@ -136,7 +136,7 @@ int sidtab_map(struct sidtab *s,
 
 	for (i = 0; i < SIDTAB_SIZE; i++) {
 		cur = s->htable[i];
-		while (cur != NULL) {
+		while (cur) {
 			rc = apply(cur->sid, &cur->context, args);
 			if (rc)
 				goto out;
@@ -155,7 +155,7 @@ static inline u32 sidtab_search_context(struct sidtab *s,
 
 	for (i = 0; i < SIDTAB_SIZE; i++) {
 		cur = s->htable[i];
-		while (cur != NULL) {
+		while (cur) {
 			if (context_cmp(&cur->context, context))
 				return cur->sid;
 			cur = cur->next;
@@ -242,7 +242,7 @@ void sidtab_destroy(struct sidtab *s)
 
 	for (i = 0; i < SIDTAB_SIZE; i++) {
 		cur = s->htable[i];
-		while (cur != NULL) {
+		while (cur) {
 			temp = cur;
 			cur = cur->next;
 			context_destroy(&temp->context);
-- 
1.5.4.1


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] selinux: Unify for- and while-loop style

[Paul Moore]

On Wednesday 06 August 2008 8:18:20 pm Vesa-Matti Kari wrote:
> Replace "thing != NULL" comparisons with just "thing" to make
> the code look more uniform (mixed styles were used even in the
> same source file).
>
> Signed-off-by: Vesa-Matti Kari <vmkari@cc.helsinki.fi>
> ---
>  security/selinux/ss/avtab.c       |    2 +-
>  security/selinux/ss/conditional.c |   16 ++++++++--------
>  security/selinux/ss/ebitmap.c     |    4 ++--
>  security/selinux/ss/hashtab.c     |    6 +++---
>  security/selinux/ss/services.c    |    8 ++++----
>  security/selinux/ss/sidtab.c      |   12 ++++++------

In my opinion this suffers from the same problems as the variable 
renaming patches.  I vote "no".

-- 
paul moore
linux @ hp

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] selinux: Unify for- and while-loop style

[Vesa-Matti J Kari]

Hello,

On Thu, 7 Aug 2008, Paul Moore wrote:

> On Wednesday 06 August 2008 8:18:20 pm Vesa-Matti Kari wrote:
> > Replace "thing != NULL" comparisons with just "thing" to make
> > the code look more uniform (mixed styles were used even in the
> > same source file).
> >
> > Signed-off-by: Vesa-Matti Kari <vmkari@cc.helsinki.fi>
> > ---
> >  security/selinux/ss/avtab.c       |    2 +-
> >  security/selinux/ss/conditional.c |   16 ++++++++--------
> >  security/selinux/ss/ebitmap.c     |    4 ++--
> >  security/selinux/ss/hashtab.c     |    6 +++---
> >  security/selinux/ss/services.c    |    8 ++++----
> >  security/selinux/ss/sidtab.c      |   12 ++++++------
>
> In my opinion this suffers from the same problems as the variable
> renaming patches.  I vote "no".

Hmmm. To avoid wasting my time, I asked beforehand whether such a trivial
unifying patch was acceptable. I did this on the selinux mailing list and
below you can see my original message followed by Stephen Smalley's
response (it was the only reply that I got):

==== begin quote ===

On Mon, 2008-07-21 at 12:19 +0300, Vesa-Matti J Kari wrote:
> Hello,
>
> I'm planning to do more trivial fixes to the SELinux kernel subsystem to
> make the coding style look as unified as possible. Before continuing,
> however, I would like to ask for an opinion, because I do not want to
> waste my time.
>
> There currently exist mixed styles such as:
>
>   while (cur != NULL)
>
> vs
>
>   while (cur)
>
> and of course the related "if"- and "for"-variants.
>
> My questions:
>
> 1) Is there a general agreement that only one style should be used?

I'd agree with that, yes.

> 2) If answer to the question 1 is "yes", which one is the preferred
style?

I'm inclined to the latter as it is more concise and loses nothing in
semantics, but I'm open to hearing the other side of it.

A note however:  the core selinux developers are likely to be somewhat
unresponsive to non-critical patches right now because we are at the
SELinux mini-summit and Linux Symposium this week.

==== end quote ===

Best regards,
vmk
-- 
************************************************************************
               Tietotekniikkaosasto / Helsingin yliopisto
                 IT Department / University of Helsinki
************************************************************************

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] selinux: Unify for- and while-loop style

[Paul Moore]

On Friday 08 August 2008 6:45:26 am Vesa-Matti J Kari wrote:
> Hello,
>
> On Thu, 7 Aug 2008, Paul Moore wrote:
> > On Wednesday 06 August 2008 8:18:20 pm Vesa-Matti Kari wrote:
> > > Replace "thing != NULL" comparisons with just "thing" to make
> > > the code look more uniform (mixed styles were used even in the
> > > same source file).
> > >
> > > Signed-off-by: Vesa-Matti Kari <vmkari@cc.helsinki.fi>
> > > ---
> > >  security/selinux/ss/avtab.c       |    2 +-
> > >  security/selinux/ss/conditional.c |   16 ++++++++--------
> > >  security/selinux/ss/ebitmap.c     |    4 ++--
> > >  security/selinux/ss/hashtab.c     |    6 +++---
> > >  security/selinux/ss/services.c    |    8 ++++----
> > >  security/selinux/ss/sidtab.c      |   12 ++++++------
> >
> > In my opinion this suffers from the same problems as the variable
> > renaming patches.  I vote "no".
>
> Hmmm. To avoid wasting my time, I asked beforehand whether such a
> trivial unifying patch was acceptable. I did this on the selinux
> mailing list and below you can see my original message followed by
> Stephen Smalley's response (it was the only reply that I got):

I didn't respond to that other thread because I was responding to your 
other thread at the same time regarding the variable renaming issue.  
Perhaps I should have replied to that particular thread as well but 
considering there was no code/patch attached I chose to spend my time 
on other higher priority issues including your other thread.

Yes, I can see Stephen replied to your original email and indicated he 
agreed on having one style.  It isn't clear to me that he necessarily 
agrees with patches that do nothing more than change coding style, but 
I don't want to speak for Stephen.  As far as I'm concerned patches 
which do nothing but change coding style or rename variables do more 
harm than good and for that reason I'm NACK'ing this patch.

The good news for you is that I'm just one person, and while ultimately 
it is the maintainer's call (James or Stephen probably, I'm actually 
not sure these days) the entire community can voice their opinion.

-- 
paul moore
linux @ hp

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] selinux: Unify for- and while-loop style

[Stephen Smalley]

On Thu, 2008-08-07 at 03:18 +0300, Vesa-Matti Kari wrote:
> Replace "thing != NULL" comparisons with just "thing" to make
> the code look more uniform (mixed styles were used even in the
> same source file).

Looks fine to me.

Acked-by:  Stephen Smalley <sds@tycho.nsa.gov>

> 
> Signed-off-by: Vesa-Matti Kari <vmkari@cc.helsinki.fi>
> ---
>  security/selinux/ss/avtab.c       |    2 +-
>  security/selinux/ss/conditional.c |   16 ++++++++--------
>  security/selinux/ss/ebitmap.c     |    4 ++--
>  security/selinux/ss/hashtab.c     |    6 +++---
>  security/selinux/ss/services.c    |    8 ++++----
>  security/selinux/ss/sidtab.c      |   12 ++++++------
>  6 files changed, 24 insertions(+), 24 deletions(-)
> 
> diff --git a/security/selinux/ss/avtab.c b/security/selinux/ss/avtab.c
> index e8ae812..1215b8e 100644
> --- a/security/selinux/ss/avtab.c
> +++ b/security/selinux/ss/avtab.c
> @@ -229,7 +229,7 @@ void avtab_destroy(struct avtab *h)
>  
>  	for (i = 0; i < h->nslot; i++) {
>  		cur = h->htable[i];
> -		while (cur != NULL) {
> +		while (cur) {
>  			temp = cur;
>  			cur = cur->next;
>  			kmem_cache_free(avtab_node_cachep, temp);
> diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
> index f8c850a..4a4e35c 100644
> --- a/security/selinux/ss/conditional.c
> +++ b/security/selinux/ss/conditional.c
> @@ -29,7 +29,7 @@ static int cond_evaluate_expr(struct policydb *p, struct cond_expr *expr)
>  	int s[COND_EXPR_MAXDEPTH];
>  	int sp = -1;
>  
> -	for (cur = expr; cur != NULL; cur = cur->next) {
> +	for (cur = expr; cur; cur = cur->next) {
>  		switch (cur->expr_type) {
>  		case COND_BOOL:
>  			if (sp == (COND_EXPR_MAXDEPTH - 1))
> @@ -97,14 +97,14 @@ int evaluate_cond_node(struct policydb *p, struct cond_node *node)
>  		if (new_state == -1)
>  			printk(KERN_ERR "SELinux: expression result was undefined - disabling all rules.\n");
>  		/* turn the rules on or off */
> -		for (cur = node->true_list; cur != NULL; cur = cur->next) {
> +		for (cur = node->true_list; cur; cur = cur->next) {
>  			if (new_state <= 0)
>  				cur->node->key.specified &= ~AVTAB_ENABLED;
>  			else
>  				cur->node->key.specified |= AVTAB_ENABLED;
>  		}
>  
> -		for (cur = node->false_list; cur != NULL; cur = cur->next) {
> +		for (cur = node->false_list; cur; cur = cur->next) {
>  			/* -1 or 1 */
>  			if (new_state)
>  				cur->node->key.specified &= ~AVTAB_ENABLED;
> @@ -128,7 +128,7 @@ int cond_policydb_init(struct policydb *p)
>  static void cond_av_list_destroy(struct cond_av_list *list)
>  {
>  	struct cond_av_list *cur, *next;
> -	for (cur = list; cur != NULL; cur = next) {
> +	for (cur = list; cur; cur = next) {
>  		next = cur->next;
>  		/* the avtab_ptr_t node is destroy by the avtab */
>  		kfree(cur);
> @@ -139,7 +139,7 @@ static void cond_node_destroy(struct cond_node *node)
>  {
>  	struct cond_expr *cur_expr, *next_expr;
>  
> -	for (cur_expr = node->expr; cur_expr != NULL; cur_expr = next_expr) {
> +	for (cur_expr = node->expr; cur_expr; cur_expr = next_expr) {
>  		next_expr = cur_expr->next;
>  		kfree(cur_expr);
>  	}
> @@ -155,7 +155,7 @@ static void cond_list_destroy(struct cond_node *list)
>  	if (list == NULL)
>  		return;
>  
> -	for (cur = list; cur != NULL; cur = next) {
> +	for (cur = list; cur; cur = next) {
>  		next = cur->next;
>  		cond_node_destroy(cur);
>  	}
> @@ -291,7 +291,7 @@ static int cond_insertf(struct avtab *a, struct avtab_key *k, struct avtab_datum
>  					goto err;
>  				}
>  				found = 0;
> -				for (cur = other; cur != NULL; cur = cur->next) {
> +				for (cur = other; cur; cur = cur->next) {
>  					if (cur->node == node_ptr) {
>  						found = 1;
>  						break;
> @@ -485,7 +485,7 @@ void cond_compute_av(struct avtab *ctab, struct avtab_key *key, struct av_decisi
>  	if (!ctab || !key || !avd)
>  		return;
>  
> -	for (node = avtab_search_node(ctab, key); node != NULL;
> +	for (node = avtab_search_node(ctab, key); node;
>  				node = avtab_search_node_next(node, key->specified)) {
>  		if ((u16)(AVTAB_ALLOWED|AVTAB_ENABLED) ==
>  		    (node->key.specified & (AVTAB_ALLOWED|AVTAB_ENABLED)))
> diff --git a/security/selinux/ss/ebitmap.c b/security/selinux/ss/ebitmap.c
> index ddc2754..68c7348 100644
> --- a/security/selinux/ss/ebitmap.c
> +++ b/security/selinux/ss/ebitmap.c
> @@ -109,7 +109,7 @@ int ebitmap_netlbl_export(struct ebitmap *ebmap,
>  	*catmap = c_iter;
>  	c_iter->startbit = e_iter->startbit & ~(NETLBL_CATMAP_SIZE - 1);
>  
> -	while (e_iter != NULL) {
> +	while (e_iter) {
>  		for (i = 0; i < EBITMAP_UNIT_NUMS; i++) {
>  			unsigned int delta, e_startbit, c_endbit;
>  
> @@ -197,7 +197,7 @@ int ebitmap_netlbl_import(struct ebitmap *ebmap,
>  			}
>  		}
>  		c_iter = c_iter->next;
> -	} while (c_iter != NULL);
> +	} while (c_iter);
>  	if (e_iter != NULL)
>  		ebmap->highbit = e_iter->startbit + EBITMAP_SIZE;
>  	else
> diff --git a/security/selinux/ss/hashtab.c b/security/selinux/ss/hashtab.c
> index 2e7788e..933e735 100644
> --- a/security/selinux/ss/hashtab.c
> +++ b/security/selinux/ss/hashtab.c
> @@ -81,7 +81,7 @@ void *hashtab_search(struct hashtab *h, const void *key)
>  
>  	hvalue = h->hash_value(h, key);
>  	cur = h->htable[hvalue];
> -	while (cur != NULL && h->keycmp(h, key, cur->key) > 0)
> +	while (cur && h->keycmp(h, key, cur->key) > 0)
>  		cur = cur->next;
>  
>  	if (cur == NULL || (h->keycmp(h, key, cur->key) != 0))
> @@ -100,7 +100,7 @@ void hashtab_destroy(struct hashtab *h)
>  
>  	for (i = 0; i < h->size; i++) {
>  		cur = h->htable[i];
> -		while (cur != NULL) {
> +		while (cur) {
>  			temp = cur;
>  			cur = cur->next;
>  			kfree(temp);
> @@ -127,7 +127,7 @@ int hashtab_map(struct hashtab *h,
>  
>  	for (i = 0; i < h->size; i++) {
>  		cur = h->htable[i];
> -		while (cur != NULL) {
> +		while (cur) {
>  			ret = apply(cur->key, cur->datum, args);
>  			if (ret)
>  				return ret;
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index b52f923..5a0536b 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -356,7 +356,7 @@ static int context_struct_compute_av(struct context *scontext,
>  			avkey.source_type = i + 1;
>  			avkey.target_type = j + 1;
>  			for (node = avtab_search_node(&policydb.te_avtab, &avkey);
> -			     node != NULL;
> +			     node;
>  			     node = avtab_search_node_next(node, avkey.specified)) {
>  				if (node->key.specified == AVTAB_ALLOWED)
>  					avd->allowed |= node->datum.data;
> @@ -1037,7 +1037,7 @@ static int security_compute_sid(u32 ssid,
>  	/* If no permanent rule, also check for enabled conditional rules */
>  	if (!avdatum) {
>  		node = avtab_search_node(&policydb.te_cond_avtab, &avkey);
> -		for (; node != NULL; node = avtab_search_node_next(node, specified)) {
> +		for (; node; node = avtab_search_node_next(node, specified)) {
>  			if (node->key.specified & AVTAB_ENABLED) {
>  				avdatum = &node->datum;
>  				break;
> @@ -2050,7 +2050,7 @@ int security_set_bools(int len, int *values)
>  			policydb.bool_val_to_struct[i]->state = 0;
>  	}
>  
> -	for (cur = policydb.cond_list; cur != NULL; cur = cur->next) {
> +	for (cur = policydb.cond_list; cur; cur = cur->next) {
>  		rc = evaluate_cond_node(&policydb, cur);
>  		if (rc)
>  			goto out;
> @@ -2102,7 +2102,7 @@ static int security_preserve_bools(struct policydb *p)
>  		if (booldatum)
>  			booldatum->state = bvalues[i];
>  	}
> -	for (cur = p->cond_list; cur != NULL; cur = cur->next) {
> +	for (cur = p->cond_list; cur; cur = cur->next) {
>  		rc = evaluate_cond_node(p, cur);
>  		if (rc)
>  			goto out;
> diff --git a/security/selinux/ss/sidtab.c b/security/selinux/ss/sidtab.c
> index a81ded1..e817989 100644
> --- a/security/selinux/ss/sidtab.c
> +++ b/security/selinux/ss/sidtab.c
> @@ -43,7 +43,7 @@ int sidtab_insert(struct sidtab *s, u32 sid, struct context *context)
>  	hvalue = SIDTAB_HASH(sid);
>  	prev = NULL;
>  	cur = s->htable[hvalue];
> -	while (cur != NULL && sid > cur->sid) {
> +	while (cur && sid > cur->sid) {
>  		prev = cur;
>  		cur = cur->next;
>  	}
> @@ -92,7 +92,7 @@ static struct context *sidtab_search_core(struct sidtab *s, u32 sid, int force)
>  
>  	hvalue = SIDTAB_HASH(sid);
>  	cur = s->htable[hvalue];
> -	while (cur != NULL && sid > cur->sid)
> +	while (cur && sid > cur->sid)
>  		cur = cur->next;
>  
>  	if (force && cur && sid == cur->sid && cur->context.len)
> @@ -103,7 +103,7 @@ static struct context *sidtab_search_core(struct sidtab *s, u32 sid, int force)
>  		sid = SECINITSID_UNLABELED;
>  		hvalue = SIDTAB_HASH(sid);
>  		cur = s->htable[hvalue];
> -		while (cur != NULL && sid > cur->sid)
> +		while (cur && sid > cur->sid)
>  			cur = cur->next;
>  		if (!cur || sid != cur->sid)
>  			return NULL;
> @@ -136,7 +136,7 @@ int sidtab_map(struct sidtab *s,
>  
>  	for (i = 0; i < SIDTAB_SIZE; i++) {
>  		cur = s->htable[i];
> -		while (cur != NULL) {
> +		while (cur) {
>  			rc = apply(cur->sid, &cur->context, args);
>  			if (rc)
>  				goto out;
> @@ -155,7 +155,7 @@ static inline u32 sidtab_search_context(struct sidtab *s,
>  
>  	for (i = 0; i < SIDTAB_SIZE; i++) {
>  		cur = s->htable[i];
> -		while (cur != NULL) {
> +		while (cur) {
>  			if (context_cmp(&cur->context, context))
>  				return cur->sid;
>  			cur = cur->next;
> @@ -242,7 +242,7 @@ void sidtab_destroy(struct sidtab *s)
>  
>  	for (i = 0; i < SIDTAB_SIZE; i++) {
>  		cur = s->htable[i];
> -		while (cur != NULL) {
> +		while (cur) {
>  			temp = cur;
>  			cur = cur->next;
>  			context_destroy(&temp->context);
-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] selinux: Unify for- and while-loop style

[Stephen Smalley]

On Fri, 2008-08-08 at 10:07 -0400, Paul Moore wrote:
> On Friday 08 August 2008 6:45:26 am Vesa-Matti J Kari wrote:
> > Hello,
> >
> > On Thu, 7 Aug 2008, Paul Moore wrote:
> > > On Wednesday 06 August 2008 8:18:20 pm Vesa-Matti Kari wrote:
> > > > Replace "thing != NULL" comparisons with just "thing" to make
> > > > the code look more uniform (mixed styles were used even in the
> > > > same source file).
> > > >
> > > > Signed-off-by: Vesa-Matti Kari <vmkari@cc.helsinki.fi>
> > > > ---
> > > >  security/selinux/ss/avtab.c       |    2 +-
> > > >  security/selinux/ss/conditional.c |   16 ++++++++--------
> > > >  security/selinux/ss/ebitmap.c     |    4 ++--
> > > >  security/selinux/ss/hashtab.c     |    6 +++---
> > > >  security/selinux/ss/services.c    |    8 ++++----
> > > >  security/selinux/ss/sidtab.c      |   12 ++++++------
> > >
> > > In my opinion this suffers from the same problems as the variable
> > > renaming patches.  I vote "no".
> >
> > Hmmm. To avoid wasting my time, I asked beforehand whether such a
> > trivial unifying patch was acceptable. I did this on the selinux
> > mailing list and below you can see my original message followed by
> > Stephen Smalley's response (it was the only reply that I got):
> 
> I didn't respond to that other thread because I was responding to your 
> other thread at the same time regarding the variable renaming issue.  
> Perhaps I should have replied to that particular thread as well but 
> considering there was no code/patch attached I chose to spend my time 
> on other higher priority issues including your other thread.
> 
> Yes, I can see Stephen replied to your original email and indicated he 
> agreed on having one style.  It isn't clear to me that he necessarily 
> agrees with patches that do nothing more than change coding style, but 
> I don't want to speak for Stephen.  As far as I'm concerned patches 
> which do nothing but change coding style or rename variables do more 
> harm than good and for that reason I'm NACK'ing this patch.
> 
> The good news for you is that I'm just one person, and while ultimately 
> it is the maintainer's call (James or Stephen probably, I'm actually 
> not sure these days) the entire community can voice their opinion.

Coding style cleanups are just part of life in the Linux kernel, and
while they make life a bit harder for developers by conflicting with
pending patches, I think they are viewed nonetheless as a win by the
Linux kernel developer community in order to promote readability and
long term maintainability by more than just the original developer(s).

So I have no problem with these or other similar coding style cleanups
to any of my code - I'm not ashamed to admit I botched the coding style
in the first place ;)
 
-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] selinux: Unify for- and while-loop style

[James Morris]

On Thu, 14 Aug 2008, Stephen Smalley wrote:

> 
> On Thu, 2008-08-07 at 03:18 +0300, Vesa-Matti Kari wrote:
> > Replace "thing != NULL" comparisons with just "thing" to make
> > the code look more uniform (mixed styles were used even in the
> > same source file).
> 
> Looks fine to me.
> 
> Acked-by:  Stephen Smalley <sds@tycho.nsa.gov>

Applied to:

git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/security-testing-2.6#next

FWIW, thes kinds of cleanups are not a problem per se, but do require time 
upstream to review, integrate and test them, where such time is often in 
short supply.  I think there's better value in working on finding/fixing 
functional bugs, picking items off the todo list [1] and generally looking 
for ways to make the code better for end users.  One example of the latter 
would be to look for ways to improve performance. There are also several 
works in progress with patches posted asking for review and feedback.


[1]
http://selinuxproject.org/page/Kernel_Development


-- 
James Morris
<jmorris@namei.org>

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.