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

[Dean Hildebrand <seattleplus@xxxxxxxxx>]

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@xxxxxxxxxx>
> > ---
> >  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@xxxxxxxxxxxxxxx
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >     
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html