[PATCH 1/2] NFS:Prevent infinite loop in decode_attr_fs_locations.

[PATCH 1/2] NFS:Prevent infinite loop in decode_attr_fs_locations.

[Dean Hildebrand]

An infinite loop could occur if n > NFS4_FS_LOCATIONS_MAXENTRIES.

Signed-off-by: Dean Hildebrand <dhildeb@us.ibm.com>
---
 fs/nfs/nfs4xdr.c |   14 ++++++++++++--
 1 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
index b916297..5e59481 100644
--- a/fs/nfs/nfs4xdr.c
+++ b/fs/nfs/nfs4xdr.c
@@ -2577,6 +2577,16 @@ static int decode_attr_fs_locations(struct xdr_stream *xdr, uint32_t *bitmap, st
 	READ32(n);
 	if (n <= 0)
 		goto out_eio;
+
+	if (n > NFS4_FS_LOCATIONS_MAXENTRIES) {
+		dprintk("%s: using first %u of %d fs locations\n",
+			__func__, NFS4_FS_LOCATIONS_MAXENTRIES, n);
+		n = NFS4_FS_LOCATIONS_MAXENTRIES;
+	} else {
+		dprintk("%s: using %d fs locations\n",
+			__func__, n);
+	}
+
 	res->nlocations = 0;
 	while (res->nlocations < n) {
 		u32 m;
@@ -2614,8 +2624,8 @@ static int decode_attr_fs_locations(struct xdr_stream *xdr, uint32_t *bitmap, st
 		status = decode_pathname(xdr, &loc->rootpath);
 		if (unlikely(status != 0))
 			goto out_eio;
-		if (res->nlocations < NFS4_FS_LOCATIONS_MAXENTRIES)
-			res->nlocations++;
+
+		res->nlocations++;
 	}
 out:
 	dprintk("%s: fs_locations done, error = %d\n", __func__, status);
-- 
1.5.3.3

[PATCH 2/2] NFS: Cleanup decode_attr_fs_locations function.

[Dean Hildebrand]

a) Reduce nesting level of loops.
b) Use correct data types.
c) Use nloc and nserv instead of n and m variable names.
d) Try to clean up formatting of debugging statements.
e) Move while loops to for loops.

Signed-off-by: Dean Hildebrand <dhildeb@us.ibm.com>
---
 fs/nfs/nfs4xdr.c |   71 +++++++++++++++++++++++++++++-------------------------
 1 files changed, 38 insertions(+), 33 deletions(-)

diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
index 5e59481..0e78fbd 100644
--- a/fs/nfs/nfs4xdr.c
+++ b/fs/nfs/nfs4xdr.c
@@ -2560,7 +2560,8 @@ out_eio:
 
 static int decode_attr_fs_locations(struct xdr_stream *xdr, uint32_t *bitmap, struct nfs4_fs_locations *res)
 {
-	int n;
+	u32 nloc;
+	unsigned int i;
 	__be32 *p;
 	int status = -EIO;
 
@@ -2574,53 +2575,57 @@ static int decode_attr_fs_locations(struct xdr_stream *xdr, uint32_t *bitmap, st
 	if (unlikely(status != 0))
 		goto out;
 	READ_BUF(4);
-	READ32(n);
-	if (n <= 0)
+	READ32(nloc);
+	if (nloc <= 0)
 		goto out_eio;
-
-	if (n > NFS4_FS_LOCATIONS_MAXENTRIES) {
-		dprintk("%s: using first %u of %d fs locations\n",
-			__func__, NFS4_FS_LOCATIONS_MAXENTRIES, n);
-		n = NFS4_FS_LOCATIONS_MAXENTRIES;
+	if (nloc > NFS4_FS_LOCATIONS_MAXENTRIES) {
+		dprintk("%s: using first %u of %u fs locations\n",
+			__func__, NFS4_FS_LOCATIONS_MAXENTRIES, nloc);
+		nloc = NFS4_FS_LOCATIONS_MAXENTRIES;
 	} else {
-		dprintk("%s: using %d fs locations\n",
-			__func__, n);
+		dprintk("%s: using %u fs locations\n",
+			__func__, nloc);
 	}
 
 	res->nlocations = 0;
-	while (res->nlocations < n) {
-		u32 m;
-		struct nfs4_fs_location *loc = &res->locations[res->nlocations];
+	for (i = 0; i < nloc; i++) {
+		u32 nserv;
+		unsigned int totalserv, j;
+		struct nfs4_fs_location *loc = &res->locations[i];
 
 		READ_BUF(4);
-		READ32(m);
+		READ32(nserv);
+
+		totalserv = nserv;
+		if (nserv >  NFS4_FS_LOCATION_MAXSERVERS) {
+			dprintk("%s: Using first %u of %u servers "
+				"returned for location %u\n",
+				__func__, NFS4_FS_LOCATION_MAXSERVERS,
+				nserv, i);
+			nserv = NFS4_FS_LOCATION_MAXSERVERS;
+		}
 
 		loc->nservers = 0;
-		dprintk("%s: servers ", __func__);
-		while (loc->nservers < m) {
+		dprintk("%s: Servers ", __func__);
+		for (j = 0; j < nserv; j++) {
 			struct nfs4_string *server = &loc->servers[loc->nservers];
 			status = decode_opaque_inline(xdr, &server->len, &server->data);
 			if (unlikely(status != 0))
 				goto out_eio;
 			dprintk("%s ", server->data);
-			if (loc->nservers < NFS4_FS_LOCATION_MAXSERVERS)
-				loc->nservers++;
-			else {
-				unsigned int i;
-				dprintk("%s: using first %u of %u servers "
-					"returned for location %u\n",
-						__func__,
-						NFS4_FS_LOCATION_MAXSERVERS,
-						m, res->nlocations);
-				for (i = loc->nservers; i < m; i++) {
-					unsigned int len;
-					char *data;
-					status = decode_opaque_inline(xdr, &len, &data);
-					if (unlikely(status != 0))
-						goto out_eio;
-				}
-			}
+			loc->nservers++;
+		}
+		dprintk("\n");
+
+		/* Decode and ignore overflow servers */
+		for (j = loc->nservers; j < totalserv; j++) {
+			unsigned int len;
+			char *data;
+			status = decode_opaque_inline(xdr, &len, &data);
+			if (unlikely(status != 0))
+				goto out_eio;
 		}
+
 		status = decode_pathname(xdr, &loc->rootpath);
 		if (unlikely(status != 0))
 			goto out_eio;
-- 
1.5.3.3

Re: [PATCH 1/2] NFS:Prevent infinite loop in decode_attr_fs_locations.

[J. Bruce Fields]

On Fri, Oct 17, 2008 at 11:17:47AM -0700, Dean Hildebrand wrote:
> An infinite loop could occur if n > NFS4_FS_LOCATIONS_MAXENTRIES.
> 
> Signed-off-by: Dean Hildebrand <dhildeb@us.ibm.com>
> ---
>  fs/nfs/nfs4xdr.c |   14 ++++++++++++--
>  1 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
> index b916297..5e59481 100644
> --- a/fs/nfs/nfs4xdr.c
> +++ b/fs/nfs/nfs4xdr.c
> @@ -2577,6 +2577,16 @@ static int decode_attr_fs_locations(struct xdr_stream *xdr, uint32_t *bitmap, st
>  	READ32(n);
>  	if (n <= 0)
>  		goto out_eio;
> +
> +	if (n > NFS4_FS_LOCATIONS_MAXENTRIES) {
> +		dprintk("%s: using first %u of %d fs locations\n",
> +			__func__, NFS4_FS_LOCATIONS_MAXENTRIES, n);
> +		n = NFS4_FS_LOCATIONS_MAXENTRIES;
> +	} else {
> +		dprintk("%s: using %d fs locations\n",
> +			__func__, n);
> +	}

I think the first case is the interesting one, so I'd drop the else
clause.

Looks fine otherwise.

The inner loop has the same problem, I assume.

--b.

> +
>  	res->nlocations = 0;
>  	while (res->nlocations < n) {
>  		u32 m;
> @@ -2614,8 +2624,8 @@ static int decode_attr_fs_locations(struct xdr_stream *xdr, uint32_t *bitmap, st
>  		status = decode_pathname(xdr, &loc->rootpath);
>  		if (unlikely(status != 0))
>  			goto out_eio;
> -		if (res->nlocations < NFS4_FS_LOCATIONS_MAXENTRIES)
> -			res->nlocations++;
> +
> +		res->nlocations++;
>  	}
>  out:
>  	dprintk("%s: fs_locations done, error = %d\n", __func__, status);
> -- 
> 1.5.3.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] NFS: Cleanup decode_attr_fs_locations function.

[J. Bruce Fields]

On Fri, Oct 17, 2008 at 11:17:48AM -0700, Dean Hildebrand wrote:
> a) Reduce nesting level of loops.
> b) Use correct data types.
> c) Use nloc and nserv instead of n and m variable names.
> d) Try to clean up formatting of debugging statements.
> e) Move while loops to for loops.

Does this also fix a possible infinite loop in the inner loop?

--b.

> 
> Signed-off-by: Dean Hildebrand <dhildeb@us.ibm.com>
> ---
>  fs/nfs/nfs4xdr.c |   71 +++++++++++++++++++++++++++++-------------------------
>  1 files changed, 38 insertions(+), 33 deletions(-)
> 
> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
> index 5e59481..0e78fbd 100644
> --- a/fs/nfs/nfs4xdr.c
> +++ b/fs/nfs/nfs4xdr.c
> @@ -2560,7 +2560,8 @@ out_eio:
>  
>  static int decode_attr_fs_locations(struct xdr_stream *xdr, uint32_t *bitmap, struct nfs4_fs_locations *res)
>  {
> -	int n;
> +	u32 nloc;
> +	unsigned int i;
>  	__be32 *p;
>  	int status = -EIO;
>  
> @@ -2574,53 +2575,57 @@ static int decode_attr_fs_locations(struct xdr_stream *xdr, uint32_t *bitmap, st
>  	if (unlikely(status != 0))
>  		goto out;
>  	READ_BUF(4);
> -	READ32(n);
> -	if (n <= 0)
> +	READ32(nloc);
> +	if (nloc <= 0)
>  		goto out_eio;
> -
> -	if (n > NFS4_FS_LOCATIONS_MAXENTRIES) {
> -		dprintk("%s: using first %u of %d fs locations\n",
> -			__func__, NFS4_FS_LOCATIONS_MAXENTRIES, n);
> -		n = NFS4_FS_LOCATIONS_MAXENTRIES;
> +	if (nloc > NFS4_FS_LOCATIONS_MAXENTRIES) {
> +		dprintk("%s: using first %u of %u fs locations\n",
> +			__func__, NFS4_FS_LOCATIONS_MAXENTRIES, nloc);
> +		nloc = NFS4_FS_LOCATIONS_MAXENTRIES;
>  	} else {
> -		dprintk("%s: using %d fs locations\n",
> -			__func__, n);
> +		dprintk("%s: using %u fs locations\n",
> +			__func__, nloc);
>  	}
>  
>  	res->nlocations = 0;
> -	while (res->nlocations < n) {
> -		u32 m;
> -		struct nfs4_fs_location *loc = &res->locations[res->nlocations];
> +	for (i = 0; i < nloc; i++) {
> +		u32 nserv;
> +		unsigned int totalserv, j;
> +		struct nfs4_fs_location *loc = &res->locations[i];
>  
>  		READ_BUF(4);
> -		READ32(m);
> +		READ32(nserv);
> +
> +		totalserv = nserv;
> +		if (nserv >  NFS4_FS_LOCATION_MAXSERVERS) {
> +			dprintk("%s: Using first %u of %u servers "
> +				"returned for location %u\n",
> +				__func__, NFS4_FS_LOCATION_MAXSERVERS,
> +				nserv, i);
> +			nserv = NFS4_FS_LOCATION_MAXSERVERS;
> +		}
>  
>  		loc->nservers = 0;
> -		dprintk("%s: servers ", __func__);
> -		while (loc->nservers < m) {
> +		dprintk("%s: Servers ", __func__);
> +		for (j = 0; j < nserv; j++) {
>  			struct nfs4_string *server = &loc->servers[loc->nservers];
>  			status = decode_opaque_inline(xdr, &server->len, &server->data);
>  			if (unlikely(status != 0))
>  				goto out_eio;
>  			dprintk("%s ", server->data);
> -			if (loc->nservers < NFS4_FS_LOCATION_MAXSERVERS)
> -				loc->nservers++;
> -			else {
> -				unsigned int i;
> -				dprintk("%s: using first %u of %u servers "
> -					"returned for location %u\n",
> -						__func__,
> -						NFS4_FS_LOCATION_MAXSERVERS,
> -						m, res->nlocations);
> -				for (i = loc->nservers; i < m; i++) {
> -					unsigned int len;
> -					char *data;
> -					status = decode_opaque_inline(xdr, &len, &data);
> -					if (unlikely(status != 0))
> -						goto out_eio;
> -				}
> -			}
> +			loc->nservers++;
> +		}
> +		dprintk("\n");
> +
> +		/* Decode and ignore overflow servers */
> +		for (j = loc->nservers; j < totalserv; j++) {
> +			unsigned int len;
> +			char *data;
> +			status = decode_opaque_inline(xdr, &len, &data);
> +			if (unlikely(status != 0))
> +				goto out_eio;
>  		}
> +
>  		status = decode_pathname(xdr, &loc->rootpath);
>  		if (unlikely(status != 0))
>  			goto out_eio;
> -- 
> 1.5.3.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] NFS: Cleanup decode_attr_fs_locations function.

[Dean Hildebrand]

J. Bruce Fields wrote:
> On Fri, Oct 17, 2008 at 11:17:48AM -0700, Dean Hildebrand wrote:
>   
>> a) Reduce nesting level of loops.
>> b) Use correct data types.
>> c) Use nloc and nserv instead of n and m variable names.
>> d) Try to clean up formatting of debugging statements.
>> e) Move while loops to for loops.
>>     
>
> Does this also fix a possible infinite loop in the inner loop?
>   
Doh, I put that fix in this patch instead of the first.  I will move it 
to the first. and apply your comments from
the other email.
Dean
>     1
>> Signed-off-by: Dean Hildebrand <dhildeb@us.ibm.com>
>> ---
>>  fs/nfs/nfs4xdr.c |   71 +++++++++++++++++++++++++++++-------------------------
>>  1 files changed, 38 insertions(+), 33 deletions(-)
>>
>> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
>> index 5e59481..0e78fbd 100644
>> --- a/fs/nfs/nfs4xdr.c
>> +++ b/fs/nfs/nfs4xdr.c
>> @@ -2560,7 +2560,8 @@ out_eio:
>>  
>>  static int decode_attr_fs_locations(struct xdr_stream *xdr, uint32_t *bitmap, struct nfs4_fs_locations *res)
>>  {
>> -	int n;
>> +	u32 nloc;
>> +	unsigned int i;
>>  	__be32 *p;
>>  	int status = -EIO;
>>  
>> @@ -2574,53 +2575,57 @@ static int decode_attr_fs_locations(struct xdr_stream *xdr, uint32_t *bitmap, st
>>  	if (unlikely(status != 0))
>>  		goto out;
>>  	READ_BUF(4);
>> -	READ32(n);
>> -	if (n <= 0)
>> +	READ32(nloc);
>> +	if (nloc <= 0)
>>  		goto out_eio;
>> -
>> -	if (n > NFS4_FS_LOCATIONS_MAXENTRIES) {
>> -		dprintk("%s: using first %u of %d fs locations\n",
>> -			__func__, NFS4_FS_LOCATIONS_MAXENTRIES, n);
>> -		n = NFS4_FS_LOCATIONS_MAXENTRIES;
>> +	if (nloc > NFS4_FS_LOCATIONS_MAXENTRIES) {
>> +		dprintk("%s: using first %u of %u fs locations\n",
>> +			__func__, NFS4_FS_LOCATIONS_MAXENTRIES, nloc);
>> +		nloc = NFS4_FS_LOCATIONS_MAXENTRIES;
>>  	} else {
>> -		dprintk("%s: using %d fs locations\n",
>> -			__func__, n);
>> +		dprintk("%s: using %u fs locations\n",
>> +			__func__, nloc);
>>  	}
>>  
>>  	res->nlocations = 0;
>> -	while (res->nlocations < n) {
>> -		u32 m;
>> -		struct nfs4_fs_location *loc = &res->locations[res->nlocations];
>> +	for (i = 0; i < nloc; i++) {
>> +		u32 nserv;
>> +		unsigned int totalserv, j;
>> +		struct nfs4_fs_location *loc = &res->locations[i];
>>  
>>  		READ_BUF(4);
>> -		READ32(m);
>> +		READ32(nserv);
>> +
>> +		totalserv = nserv;
>> +		if (nserv >  NFS4_FS_LOCATION_MAXSERVERS) {
>> +			dprintk("%s: Using first %u of %u servers "
>> +				"returned for location %u\n",
>> +				__func__, NFS4_FS_LOCATION_MAXSERVERS,
>> +				nserv, i);
>> +			nserv = NFS4_FS_LOCATION_MAXSERVERS;
>> +		}
>>  
>>  		loc->nservers = 0;
>> -		dprintk("%s: servers ", __func__);
>> -		while (loc->nservers < m) {
>> +		dprintk("%s: Servers ", __func__);
>> +		for (j = 0; j < nserv; j++) {
>>  			struct nfs4_string *server = &loc->servers[loc->nservers];
>>  			status = decode_opaque_inline(xdr, &server->len, &server->data);
>>  			if (unlikely(status != 0))
>>  				goto out_eio;
>>  			dprintk("%s ", server->data);
>> -			if (loc->nservers < NFS4_FS_LOCATION_MAXSERVERS)
>> -				loc->nservers++;
>> -			else {
>> -				unsigned int i;
>> -				dprintk("%s: using first %u of %u servers "
>> -					"returned for location %u\n",
>> -						__func__,
>> -						NFS4_FS_LOCATION_MAXSERVERS,
>> -						m, res->nlocations);
>> -				for (i = loc->nservers; i < m; i++) {
>> -					unsigned int len;
>> -					char *data;
>> -					status = decode_opaque_inline(xdr, &len, &data);
>> -					if (unlikely(status != 0))
>> -						goto out_eio;
>> -				}
>> -			}
>> +			loc->nservers++;
>> +		}
>> +		dprintk("\n");
>> +
>> +		/* Decode and ignore overflow servers */
>> +		for (j = loc->nservers; j < totalserv; j++) {
>> +			unsigned int len;
>> +			char *data;
>> +			status = decode_opaque_inline(xdr, &len, &data);
>> +			if (unlikely(status != 0))
>> +				goto out_eio;
>>  		}
>> +
>>  		status = decode_pathname(xdr, &loc->rootpath);
>>  		if (unlikely(status != 0))
>>  			goto out_eio;
>> -- 
>> 1.5.3.3
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>     

[PATCH 2/2] NFS: Cleanup decode_attr_fs_locations function.

[Dean Hildebrand]

a) Use correct data types.
b) Use nloc and nserv instead of n and m variable names.
c) Try to clean up formatting of debugging statements.
d) Move while loops to for loops.

Signed-off-by: Dean Hildebrand <dhildeb@us.ibm.com>
---
 fs/nfs/nfs4xdr.c |   38 ++++++++++++++++++++------------------
 1 files changed, 20 insertions(+), 18 deletions(-)

diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
index 0b4c565..b7de923 100644
--- a/fs/nfs/nfs4xdr.c
+++ b/fs/nfs/nfs4xdr.c
@@ -2560,7 +2560,8 @@ out_eio:
 
 static int decode_attr_fs_locations(struct xdr_stream *xdr, uint32_t *bitmap, struct nfs4_fs_locations *res)
 {
-	int n;
+	u32 nloc;
+	unsigned int i;
 	__be32 *p;
 	int status = -EIO;
 
@@ -2574,37 +2575,37 @@ static int decode_attr_fs_locations(struct xdr_stream *xdr, uint32_t *bitmap, st
 	if (unlikely(status != 0))
 		goto out;
 	READ_BUF(4);
-	READ32(n);
-	if (n <= 0)
+	READ32(nloc);
+	if (nloc <= 0)
 		goto out_eio;
 
-	if (n > NFS4_FS_LOCATIONS_MAXENTRIES) {
-		dprintk("\n%s: Using first %u of %d fs locations\n",
-			__func__, NFS4_FS_LOCATIONS_MAXENTRIES, n);
-		n = NFS4_FS_LOCATIONS_MAXENTRIES;
+	if (nloc > NFS4_FS_LOCATIONS_MAXENTRIES) {
+		dprintk("\n%s: Using first %u of %u fs locations\n",
+			__func__, NFS4_FS_LOCATIONS_MAXENTRIES, nloc);
+		nloc = NFS4_FS_LOCATIONS_MAXENTRIES;
 	}
 
 	res->nlocations = 0;
-	while (res->nlocations < n) {
-		u32 m;
-		unsigned int totalserv, i;
-		struct nfs4_fs_location *loc = &res->locations[res->nlocations];
+	for (i = 0; i < nloc; i++) {
+		u32 nserv;
+		unsigned int totalserv, j;
+		struct nfs4_fs_location *loc = &res->locations[i];
 
 		READ_BUF(4);
-		READ32(m);
+		READ32(nserv);
 
-		totalserv = m;
-		if (m >  NFS4_FS_LOCATION_MAXSERVERS) {
+		totalserv = nserv;
+		if (nserv >  NFS4_FS_LOCATION_MAXSERVERS) {
 			dprintk("\n%s: Using first %u of %u servers "
 				"returned for location %u\n",
 				__func__, NFS4_FS_LOCATION_MAXSERVERS,
-				m, res->nlocations);
-			m = NFS4_FS_LOCATION_MAXSERVERS;
+				nserv, i);
+			nserv = NFS4_FS_LOCATION_MAXSERVERS;
 		}
 
 		loc->nservers = 0;
 		dprintk("%s: servers ", __func__);
-		while (loc->nservers < m) {
+		for (j = 0; j < nserv; j++) {
 			struct nfs4_string *server = &loc->servers[loc->nservers];
 			status = decode_opaque_inline(xdr, &server->len, &server->data);
 			if (unlikely(status != 0))
@@ -2612,9 +2613,10 @@ static int decode_attr_fs_locations(struct xdr_stream *xdr, uint32_t *bitmap, st
 			dprintk("%s ", server->data);
 			loc->nservers++;
 		}
+		dprintk("\n");
 
 		/* Decode and ignore overflow servers */
-		for (i = loc->nservers; i < totalserv; i++) {
+		for (j = loc->nservers; j < totalserv; j++) {
 			unsigned int len;
 			char *data;
 			status = decode_opaque_inline(xdr, &len, &data);
-- 
1.5.3.3

Re: [PATCH 2/2] NFS: Cleanup decode_attr_fs_locations function.

[Benny Halevy]

On Oct. 18, 2008, 0:52 +0200, Dean Hildebrand <seattleplus@gmail.com> wrote:
> a) Use correct data types.
> b) Use nloc and nserv instead of n and m variable names.
> c) Try to clean up formatting of debugging statements.
> d) Move while loops to for loops.
> 
> Signed-off-by: Dean Hildebrand <dhildeb@us.ibm.com>
> ---
>  fs/nfs/nfs4xdr.c |   38 ++++++++++++++++++++------------------
>  1 files changed, 20 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
> index 0b4c565..b7de923 100644
> --- a/fs/nfs/nfs4xdr.c
> +++ b/fs/nfs/nfs4xdr.c
> @@ -2560,7 +2560,8 @@ out_eio:
>  
>  static int decode_attr_fs_locations(struct xdr_stream *xdr, uint32_t *bitmap, struct nfs4_fs_locations *res)
>  {
> -	int n;
> +	u32 nloc;
> +	unsigned int i;
>  	__be32 *p;
>  	int status = -EIO;
>  
> @@ -2574,37 +2575,37 @@ static int decode_attr_fs_locations(struct xdr_stream *xdr, uint32_t *bitmap, st
>  	if (unlikely(status != 0))
>  		goto out;
>  	READ_BUF(4);
> -	READ32(n);
> -	if (n <= 0)
> +	READ32(nloc);
> +	if (nloc <= 0)
>  		goto out_eio;
>  
> -	if (n > NFS4_FS_LOCATIONS_MAXENTRIES) {
> -		dprintk("\n%s: Using first %u of %d fs locations\n",
> -			__func__, NFS4_FS_LOCATIONS_MAXENTRIES, n);
> -		n = NFS4_FS_LOCATIONS_MAXENTRIES;
> +	if (nloc > NFS4_FS_LOCATIONS_MAXENTRIES) {
> +		dprintk("\n%s: Using first %u of %u fs locations\n",
> +			__func__, NFS4_FS_LOCATIONS_MAXENTRIES, nloc);
> +		nloc = NFS4_FS_LOCATIONS_MAXENTRIES;
>  	}
>  
>  	res->nlocations = 0;
> -	while (res->nlocations < n) {
> -		u32 m;
> -		unsigned int totalserv, i;
> -		struct nfs4_fs_location *loc = &res->locations[res->nlocations];
> +	for (i = 0; i < nloc; i++) {

you could also keep using res->nlocations as iterator, e.g.

-	res->nlocations = 0;
-	while (res->nlocations < n) {
+	for (res->nlocations = 0; res->nlocations < nloc; res->nlocations++) {

Since it is incremented every time we go through the loop
anyway using the auxiliary variable is useless.
(It could possibly improve performance a bit for long
arrays if the compiler would've used a register for the local
variable, but then you should assign its final value
to res->nlocations, not increment it every iteration.
However, I don't think it's worth it in this case)

> +		u32 nserv;
> +		unsigned int totalserv, j;
> +		struct nfs4_fs_location *loc = &res->locations[i];
>  
>  		READ_BUF(4);
> -		READ32(m);
> +		READ32(nserv);
>  
> -		totalserv = m;
> -		if (m >  NFS4_FS_LOCATION_MAXSERVERS) {
> +		totalserv = nserv;
> +		if (nserv >  NFS4_FS_LOCATION_MAXSERVERS) {
>  			dprintk("\n%s: Using first %u of %u servers "
>  				"returned for location %u\n",
>  				__func__, NFS4_FS_LOCATION_MAXSERVERS,
> -				m, res->nlocations);
> -			m = NFS4_FS_LOCATION_MAXSERVERS;
> +				nserv, i);
> +			nserv = NFS4_FS_LOCATION_MAXSERVERS;
>  		}
>  
>  		loc->nservers = 0;
>  		dprintk("%s: servers ", __func__);
> -		while (loc->nservers < m) {
> +		for (j = 0; j < nserv; j++) {

ditto for loc->nservers.

Benny

>  			struct nfs4_string *server = &loc->servers[loc->nservers];
>  			status = decode_opaque_inline(xdr, &server->len, &server->data);
>  			if (unlikely(status != 0))
> @@ -2612,9 +2613,10 @@ static int decode_attr_fs_locations(struct xdr_stream *xdr, uint32_t *bitmap, st
>  			dprintk("%s ", server->data);
>  			loc->nservers++;
>  		}
> +		dprintk("\n");
>  
>  		/* Decode and ignore overflow servers */
> -		for (i = loc->nservers; i < totalserv; i++) {
> +		for (j = loc->nservers; j < totalserv; j++) {
>  			unsigned int len;
>  			char *data;
>  			status = decode_opaque_inline(xdr, &len, &data);

Re: [PATCH 2/2] NFS: Cleanup decode_attr_fs_locations function.

[Dean Hildebrand]

Benny Halevy wrote:
> On Oct. 18, 2008, 0:52 +0200, Dean Hildebrand <seattleplus@gmail.com> wrote:
>   
>> a) Use correct data types.
>> b) Use nloc and nserv instead of n and m variable names.
>> c) Try to clean up formatting of debugging statements.
>> d) Move while loops to for loops.
>>
>> Signed-off-by: Dean Hildebrand <dhildeb@us.ibm.com>
>> ---
>>  fs/nfs/nfs4xdr.c |   38 ++++++++++++++++++++------------------
>>  1 files changed, 20 insertions(+), 18 deletions(-)
>>
>> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
>> index 0b4c565..b7de923 100644
>> --- a/fs/nfs/nfs4xdr.c
>> +++ b/fs/nfs/nfs4xdr.c
>> @@ -2560,7 +2560,8 @@ out_eio:
>>  
>>  static int decode_attr_fs_locations(struct xdr_stream *xdr, uint32_t *bitmap, struct nfs4_fs_locations *res)
>>  {
>> -	int n;
>> +	u32 nloc;
>> +	unsigned int i;
>>  	__be32 *p;
>>  	int status = -EIO;
>>  
>> @@ -2574,37 +2575,37 @@ static int decode_attr_fs_locations(struct xdr_stream *xdr, uint32_t *bitmap, st
>>  	if (unlikely(status != 0))
>>  		goto out;
>>  	READ_BUF(4);
>> -	READ32(n);
>> -	if (n <= 0)
>> +	READ32(nloc);
>> +	if (nloc <= 0)
>>  		goto out_eio;
>>  
>> -	if (n > NFS4_FS_LOCATIONS_MAXENTRIES) {
>> -		dprintk("\n%s: Using first %u of %d fs locations\n",
>> -			__func__, NFS4_FS_LOCATIONS_MAXENTRIES, n);
>> -		n = NFS4_FS_LOCATIONS_MAXENTRIES;
>> +	if (nloc > NFS4_FS_LOCATIONS_MAXENTRIES) {
>> +		dprintk("\n%s: Using first %u of %u fs locations\n",
>> +			__func__, NFS4_FS_LOCATIONS_MAXENTRIES, nloc);
>> +		nloc = NFS4_FS_LOCATIONS_MAXENTRIES;
>>  	}
>>  
>>  	res->nlocations = 0;
>> -	while (res->nlocations < n) {
>> -		u32 m;
>> -		unsigned int totalserv, i;
>> -		struct nfs4_fs_location *loc = &res->locations[res->nlocations];
>> +	for (i = 0; i < nloc; i++) {
>>     
>
> you could also keep using res->nlocations as iterator, e.g.
>
> -	res->nlocations = 0;
> -	while (res->nlocations < n) {
> +	for (res->nlocations = 0; res->nlocations < nloc; res->nlocations++) {
>   
Sounds reasonable, although I don't want to hear any flak for exceeding 
80 chars.
Dean
> Since it is incremented every time we go through the loop
> anyway using the auxiliary variable is useless.
> (It could possibly improve performance a bit for long
> arrays if the compiler would've used a register for the local
> variable, but then you should assign its final value
> to res->nlocations, not increment it every iteration.
> However, I don't think it's worth it in this case)
>
>   
>> +		u32 nserv;
>> +		unsigned int totalserv, j;
>> +		struct nfs4_fs_location *loc = &res->locations[i];
>>  
>>  		READ_BUF(4);
>> -		READ32(m);
>> +		READ32(nserv);
>>  
>> -		totalserv = m;
>> -		if (m >  NFS4_FS_LOCATION_MAXSERVERS) {
>> +		totalserv = nserv;
>> +		if (nserv >  NFS4_FS_LOCATION_MAXSERVERS) {
>>  			dprintk("\n%s: Using first %u of %u servers "
>>  				"returned for location %u\n",
>>  				__func__, NFS4_FS_LOCATION_MAXSERVERS,
>> -				m, res->nlocations);
>> -			m = NFS4_FS_LOCATION_MAXSERVERS;
>> +				nserv, i);
>> +			nserv = NFS4_FS_LOCATION_MAXSERVERS;
>>  		}
>>  
>>  		loc->nservers = 0;
>>  		dprintk("%s: servers ", __func__);
>> -		while (loc->nservers < m) {
>> +		for (j = 0; j < nserv; j++) {
>>     
>
> ditto for loc->nservers.
>
> Benny
>
>   
>>  			struct nfs4_string *server = &loc->servers[loc->nservers];
>>  			status = decode_opaque_inline(xdr, &server->len, &server->data);
>>  			if (unlikely(status != 0))
>> @@ -2612,9 +2613,10 @@ static int decode_attr_fs_locations(struct xdr_stream *xdr, uint32_t *bitmap, st
>>  			dprintk("%s ", server->data);
>>  			loc->nservers++;
>>  		}
>> +		dprintk("\n");
>>  
>>  		/* Decode and ignore overflow servers */
>> -		for (i = loc->nservers; i < totalserv; i++) {
>> +		for (j = loc->nservers; j < totalserv; j++) {
>>  			unsigned int len;
>>  			char *data;
>>  			status = decode_opaque_inline(xdr, &len, &data);
>>     

[PATCH 2/2] NFS: Cleanup decode_attr_fs_locations function.

[Dean Hildebrand]

a) Use correct data types.
b) Use nloc and nserv instead of n and m variable names.
c) Try to clean up formatting of debugging statements.
d) Move while loops to for loops.

Signed-off-by: Dean Hildebrand <dhildeb@us.ibm.com>
---
 fs/nfs/nfs4xdr.c |   40 ++++++++++++++++++----------------------
 1 files changed, 18 insertions(+), 22 deletions(-)

diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
index 0b4c565..421609f 100644
--- a/fs/nfs/nfs4xdr.c
+++ b/fs/nfs/nfs4xdr.c
@@ -2560,7 +2560,7 @@ out_eio:
 
 static int decode_attr_fs_locations(struct xdr_stream *xdr, uint32_t *bitmap, struct nfs4_fs_locations *res)
 {
-	int n;
+	u32 nloc;
 	__be32 *p;
 	int status = -EIO;
 
@@ -2574,47 +2574,45 @@ static int decode_attr_fs_locations(struct xdr_stream *xdr, uint32_t *bitmap, st
 	if (unlikely(status != 0))
 		goto out;
 	READ_BUF(4);
-	READ32(n);
-	if (n <= 0)
+	READ32(nloc);
+	if (nloc <= 0)
 		goto out_eio;
 
-	if (n > NFS4_FS_LOCATIONS_MAXENTRIES) {
-		dprintk("\n%s: Using first %u of %d fs locations\n",
-			__func__, NFS4_FS_LOCATIONS_MAXENTRIES, n);
-		n = NFS4_FS_LOCATIONS_MAXENTRIES;
+	if (nloc > NFS4_FS_LOCATIONS_MAXENTRIES) {
+		dprintk("\n%s: Using first %u of %u fs locations\n",
+			__func__, NFS4_FS_LOCATIONS_MAXENTRIES, nloc);
+		nloc = NFS4_FS_LOCATIONS_MAXENTRIES;
 	}
 
-	res->nlocations = 0;
-	while (res->nlocations < n) {
-		u32 m;
-		unsigned int totalserv, i;
+	for (res->nlocations = 0; res->nlocations < nloc; res->nlocations++) {
+		u32 nserv;
+		unsigned int totalserv, j;
 		struct nfs4_fs_location *loc = &res->locations[res->nlocations];
 
 		READ_BUF(4);
-		READ32(m);
+		READ32(nserv);
 
-		totalserv = m;
-		if (m >  NFS4_FS_LOCATION_MAXSERVERS) {
+		totalserv = nserv;
+		if (nserv >  NFS4_FS_LOCATION_MAXSERVERS) {
 			dprintk("\n%s: Using first %u of %u servers "
 				"returned for location %u\n",
 				__func__, NFS4_FS_LOCATION_MAXSERVERS,
-				m, res->nlocations);
-			m = NFS4_FS_LOCATION_MAXSERVERS;
+				nserv, res->nlocations);
+			nserv = NFS4_FS_LOCATION_MAXSERVERS;
 		}
 
-		loc->nservers = 0;
 		dprintk("%s: servers ", __func__);
-		while (loc->nservers < m) {
+		for (loc->nservers = 0; loc->nservers < nserv; loc->nservers++) {
 			struct nfs4_string *server = &loc->servers[loc->nservers];
 			status = decode_opaque_inline(xdr, &server->len, &server->data);
 			if (unlikely(status != 0))
 				goto out_eio;
 			dprintk("%s ", server->data);
-			loc->nservers++;
 		}
+		dprintk("\n");
 
 		/* Decode and ignore overflow servers */
-		for (i = loc->nservers; i < totalserv; i++) {
+		for (j = loc->nservers; j < totalserv; j++) {
 			unsigned int len;
 			char *data;
 			status = decode_opaque_inline(xdr, &len, &data);
@@ -2625,8 +2623,6 @@ static int decode_attr_fs_locations(struct xdr_stream *xdr, uint32_t *bitmap, st
 		status = decode_pathname(xdr, &loc->rootpath);
 		if (unlikely(status != 0))
 			goto out_eio;
-
-		res->nlocations++;
 	}
 out:
 	dprintk("%s: fs_locations done, error = %d\n", __func__, status);
-- 
1.5.4.5