net/xfrm/xfrm_policy.c: Some small improvements

PATCH In review WANG Cong 4 participants 6 messages Dec 6, 2007 – Dec 7, 2007

Subsystems (3)

Networking Network drivers Networking IPsec

lore

Share & tools

lore

WANG Cong

author

2007-12-06 11:05 UTC #0 lore raw

PATCH 1/1 net/xfrm/xfrm_policy.c: Some small improvements

This patch contains the following changes.

- Use 'bool' instead of 'int' for booleans.
- Use 'size_t' instead of 'int' for 'sizeof' return value.
- Some style fixes.

Cc Herbert Xu <[email protected]>

Cc David Miller <[email protected]>

Signed-off-by WANG Cong <[email protected]>

---
 net/xfrm/xfrm_policy.c |   23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

net/xfrm/xfrm_policy.c +12 ~~-11~~

index 5d6a81d..311b08f 100644
@@ -476,17 +476,17 @@ static u32 xfrm_gen_index(u8 type, int dir)
 		struct hlist_head *list;
 		struct xfrm_policy *p;
 		u32 idx;
-		int found;
+		bool found;
 
 		idx = (idx_generator | dir);
 		idx_generator += 8;
 		if (idx == 0)
 			idx = 8;
 		list = xfrm_policy_byidx + idx_hash(idx);
-		found = 0;
+		found = false;
 		hlist_for_each_entry(p, entry, list, byidx) {
 			if (p->index == idx) {
-				found = 1;
+				found = true;
 				break;
 			}
 		}

@@ -499,8 +499,8 @@ static inline int selector_cmp(struct xfrm_selector *s1, struct xfrm_selector *s
 {
 	u32 *p1 = (u32 *) s1;
 	u32 *p2 = (u32 *) s2;
-	int len = sizeof(struct xfrm_selector) / sizeof(u32);
-	int i;
+	size_t len = sizeof(struct xfrm_selector) / sizeof(u32);
+	size_t i;
 
 	for (i = 0; i < len; i++) {
 		if (p1[i] != p2[i])

@@ -953,7 +953,7 @@ static int xfrm_policy_lookup(struct flowi *fl, u16 family, u8 dir,
 #ifdef CONFIG_XFRM_SUB_POLICY
 end:
 #endif
-	if ((*objp = (void *) pol) != NULL)
+	if ((*objp = pol) != NULL)
 		*obj_refp = &pol->refcnt;
 	return err;
 }

@@ -1137,7 +1137,7 @@ xfrm_tmpl_resolve_one(struct xfrm_policy *policy, struct flowi *fl,
 	xfrm_address_t *saddr = xfrm_flowi_saddr(fl, family);
 	xfrm_address_t tmp;
 
-	for (nx=0, i = 0; i < policy->xfrm_nr; i++) {
+	for (nx = 0, i = 0; i < policy->xfrm_nr; i++) {
 		struct xfrm_state *x;
 		xfrm_address_t *remote = daddr;
 		xfrm_address_t *local  = saddr;

@@ -1395,7 +1395,7 @@ free_dst:
 }
 
 static int inline
-xfrm_dst_alloc_copy(void **target, void *src, int size)
+xfrm_dst_alloc_copy(void **target, void *src, size_t size)
 {
 	if (!*target) {
 		*target = kmalloc(size, GFP_ATOMIC);

@@ -1554,7 +1554,7 @@ restart:
 #endif
 		nx = xfrm_tmpl_resolve(pols, npols, fl, xfrm, family);
 
-		if (unlikely(nx<0)) {
+		if (unlikely(nx < 0)) {
 			err = nx;
 			if (err == -EAGAIN && sysctl_xfrm_larval_drop) {
 				/* EREMOTE tells the caller to generate

@@ -1688,7 +1688,8 @@ xfrm_state_ok(struct xfrm_tmpl *tmpl, struct xfrm_state *x,
 	      unsigned short family)
 {
 	if (xfrm_state_kern(x))
-		return tmpl->optional && !xfrm_state_addr_cmp(tmpl, x, tmpl->encap_family);
+		return tmpl->optional &&
+			!xfrm_state_addr_cmp(tmpl, x, tmpl->encap_family);
 	return	x->id.proto == tmpl->id.proto &&
 		(x->id.spi == tmpl->id.spi || !tmpl->id.spi) &&
 		(x->props.reqid == tmpl->reqid || !tmpl->reqid) &&

@@ -1777,7 +1778,7 @@ int __xfrm_policy_check(struct sock *sk, int dir, struct sk_buff *skb,
 	if (skb->sp) {
 		int i;
 
-		for (i=skb->sp->len-1; i>=0; i--) {
+		for (i = skb->sp->len-1; i >= 0; i--) {
 			struct xfrm_state *x = skb->sp->xvec[i];
 			if (!xfrm_selector_match(&x->sel, &fl, family))
 				return 0;

David Miller

maintainer

2007-12-06 11:14 UTC #1 lore raw

From: WANG Cong <[email protected]>
Date: Thu, 6 Dec 2007 19:01:23 +0800

Quoted 10 lines

This patch contains the following changes.
- Use 'bool' instead of 'int' for booleans.
- Use 'size_t' instead of 'int' for 'sizeof' return value.
- Some style fixes.
Cc Herbert Xu <[email protected]>
Cc David Miller <[email protected]>
Signed-off-by WANG Cong <[email protected]>

Normally I would let a patch like this sit in my mailbox for a week and then delete it.

But this time I'll just let you know up front that I don't see much value in this patch. It is not a clear improvement to replace int's with bool's in my mind and the other changes are just whitespace changes.

And thus I can delete the patch from my mailbox immediately :-)

Sorry.

Richard Knutsson

2007-12-06 14:35 UTC #2 lore raw

Quoted 10 lines · David Miller

From: WANG Cong <[email protected]>
Date: Thu, 6 Dec 2007 19:01:23 +0800
Quoted 6 lines
This patch contains the following changes.
- Use 'bool' instead of 'int' for booleans.
- Use 'size_t' instead of 'int' for 'sizeof' return value.
- Some style fixes.

Quoted 8 lines

Quoted 4 lines
Cc Herbert Xu <[email protected]>
Cc David Miller <[email protected]>
Signed-off-by WANG Cong <[email protected]>
Normally I would let a patch like this sit in my mailbox for a week and then delete it.

That is evil! ;)

Quoted 5 lines

But this time I'll just let you know up front that I don't see much value in this patch. It is not a clear improvement to replace int's with bool's in my mind and the other changes are just whitespace changes.

Is it not an improvement to distinct booleans from actual values? Do you use integers for ASCII characters too? It can also avoid some potential bugs like the 'if (i == TRUE)'... What is wrong with 'size_t' (since it is unsigned, compared to (some) 'int')?

/Richard Knutsson

Herbert Xu

maintainer

2007-12-06 17:53 UTC #3 lore raw

Quoted 6 lines · Richard Knutsson

Is it not an improvement to distinct booleans from actual values? Do you use integers for ASCII characters too? It can also avoid some potential bugs like the 'if (i == TRUE)'...
What is wrong with 'size_t' (since it is unsigned, compared to (some) 'int')?

I agree with Dave. There are so many useful things that we can do (and need to do) in IPsec that bool/size_t conversions just add churn without adding much value.

Thanks,

Signature · 4 lines

Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

David Miller

maintainer

2007-12-07 03:22 UTC #4 lore raw

In reply to Richard Knutsson

From: Richard Knutsson <[email protected]>
Date: Thu, 06 Dec 2007 15:37:46 +0100

Quoted 11 lines

Quoted 5 lines · David Miller
But this time I'll just let you know up front that I don't see much value in this patch. It is not a clear improvement to replace int's with bool's in my mind and the other changes are just whitespace changes.
Is it not an improvement to distinct booleans from actual values? Do you use integers for ASCII characters too? It can also avoid some potential bugs like the 'if (i == TRUE)'...
What is wrong with 'size_t' (since it is unsigned, compared to (some) 'int')?

When you say "int found;" is there any doubt in your mind that this integer is going to hold a 1 or a 0 depending upon whether we "found" something?

That's the problem I have with these kinds of patches, they do not increase clarity, it's just pure mindless edits.

In new code, fine, use booleans if you want.

I would even accept that it helps to change to boolean for arguments to functions that are global in scope.

But not for function local variables in cases like this.

Richard Knutsson

2007-12-07 15:39 UTC #5 lore raw

Quoted 16 lines · David Miller

From: Richard Knutsson <[email protected]>
Date: Thu, 06 Dec 2007 15:37:46 +0100
Quoted 11 lines
Quoted 6 lines · David Miller
But this time I'll just let you know up front that I don't see much value in this patch. It is not a clear improvement to replace int's with bool's in my mind and the other changes are just whitespace changes.
Is it not an improvement to distinct booleans from actual values? Do you use integers for ASCII characters too? It can also avoid some potential bugs like the 'if (i == TRUE)'... What is wrong with 'size_t' (since it is unsigned, compared to (some) 'int')?

Quoted 7 lines

When you say "int found;" is there any doubt in your mind that this integer is going to hold a 1 or a 0 depending upon whether we "found" something?
That's the problem I have with these kinds of patches, they do not increase clarity, it's just pure mindless edits.

But is there not a good thing if also the compiler knows + names are sometime not as clear as that one?

Quoted 7 lines

In new code, fine, use booleans if you want.
I would even accept that it helps to change to boolean for arguments to functions that are global in scope.
But not for function local variables in cases like this.

Oh, I see your point now. Believed it to be yet another 'booleans is not C idiom'.

Sorry about the noise
Richard Knutsson