2.6.38 nfsd bugfixes

The following bugfixes for 2.6.38 are available from:

	git://linux-nfs.org/~bfields/linux.git for-2.6.38

(Note several of the "nfsd4:" patches are just one fix for a merge-window regression, with some preperatory cleanup rather finely split out.)

--b.

Benny Halevy (1):
NFSD: use nfserr for status after decode_cb_op_status

J. Bruce Fields (9):
nfsd: don't leak dentry count on mnt_want_write failure
nfsd4: split up nfsd_break_deleg_cb
nfsd4: add helper function for lease setup
nfsd4: fix leak on allocation error
nfsd4: split lease setting into separate function
nfsd4: remove unused deleg dprintk's.
nfsd4: modify fi_delegations under recall_lock
nfsd4: acquire only one lease per file
nfsd: break lease on unlink due to rename

Konstantin Khorenko (1):
NFSD: memory corruption due to writing beyond the stat array

 fs/nfsd/nfs4callback.c |    6 +-
 fs/nfsd/nfs4state.c    |  186 +++++++++++++++++++++++++++---------------------
 fs/nfsd/state.h        |    5 +-
 fs/nfsd/vfs.c          |   21 ++++--
 4 files changed, 125 insertions(+), 93 deletions(-)

Doh--I missed one patch in my previous pull request; see

git://linux-nfs.org/~bfields/linux.git for-2.6.38

Please apply.

--b.

commit 47c85291d3dd1a51501555000b90f8e281a0458e

Author: NeilBrown <[email protected]>
Date:   Wed Feb 16 13:08:35 2011 +1100

nfsd: correctly handle return value from nfsd_map_name_to_*

These functions return an nfs status, not a host_err. So don't try to convert before returning.

This is a regression introduced by 3c726023402a2f3b28f49b9d90ebf9e71151157d; I fixed up two of the callers, but missed these two.

index 956629b..1275b86 100644
@@ -317,8 +317,8 @@ nfsd4_decode_fattr(struct nfsd4_compoundargs *argp, u32 *bmval,
 		READ_BUF(dummy32);
 		len += (XDR_QUADLEN(dummy32) << 2);
 		READMEM(buf, dummy32);
-		if ((host_err = nfsd_map_name_to_uid(argp->rqstp, buf, dummy32, &iattr->ia_uid)))
-			goto out_nfserr;
+		if ((status = nfsd_map_name_to_uid(argp->rqstp, buf, dummy32, &iattr->ia_uid)))
+			return status;
 		iattr->ia_valid |= ATTR_UID;
 	}
 	if (bmval[1] & FATTR4_WORD1_OWNER_GROUP) {

@@ -328,8 +328,8 @@ nfsd4_decode_fattr(struct nfsd4_compoundargs *argp, u32 *bmval,
 		READ_BUF(dummy32);
 		len += (XDR_QUADLEN(dummy32) << 2);
 		READMEM(buf, dummy32);
-		if ((host_err = nfsd_map_name_to_gid(argp->rqstp, buf, dummy32, &iattr->ia_gid)))
-			goto out_nfserr;
+		if ((status = nfsd_map_name_to_gid(argp->rqstp, buf, dummy32, &iattr->ia_gid)))
+			return status;
 		iattr->ia_valid |= ATTR_GID;
 	}
 	if (bmval[1] & FATTR4_WORD1_TIME_ACCESS_SET) {

Btw, can we please just agree to not doing those idiotic double parenthesis?

There is a really trivial solution to the gcc warning - write your code like a sane person, instead of some ex-LISP hacker that has withdrawal symptoms. IOW, the above should be written as

  status = nfsd_map_name_to_uid(argp->rqstp, buf, dummy32, &iattr->ia_uid);
  if (status)
    return status;

which is a hell of a lot more readable, no?

There is never any real excuse to put an assignment inside a regular if-statement.

Inside a while/for loop? Sure. There are real syntactic reasons for doing things like

   while ((c = getchar()) != EOF) {
   }

that actually make the code better and denser and avoid extra control flow crap or duplicate code.

Inside a macro expansion? Again, there may be good reasons to try to make it a single statement.

But a simple if-statement? There just isn't any reason for it, since the obvious thing is to just write it as two separate statements: the assignment, and the if-statement. So why do it and make the code uglier and harder to parse?

Linus

Fine by me; I don't write new code that way.

I already committed it like that, so would rather just do any cleanup as another patch for the next merge window; but let me know what you want.

--b.

That's fine. I just want to point these out as I see them, and ask that when people fix bugs, they also aim to make the code readable at the same time.

Linus

OK, makes sense.

--b.