[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-nfs
Subject:    Re: [PATCH] NFSD: Combine decode operations for v4 and v4.1
From:       "J. Bruce Fields" <bfields () fieldses ! org>
Date:       2013-10-29 21:24:02
Message-ID: 20131029212402.GB3227 () fieldses ! org
[Download RAW message or body]

On Tue, Oct 29, 2013 at 05:13:25PM -0400, Anna Schumaker wrote:
> On Tue 29 Oct 2013 05:06:59 PM EDT, J. Bruce Fields wrote:
> > On Tue, Oct 29, 2013 at 05:04:36PM -0400, Anna Schumaker wrote:
> >> We were using a different array of function pointers to represent each
> >> minor version.  This makes adding a new minor version tedious, since it
> >> needs a step to copy, paste and modify a new version of the same
> >> functions.
> >>
> >> This patch combines the v4 and v4.1 arrays into a single instance and
> >> will check minor version support inside each decoder function.
> >
> > This also needs checks in all the new operations.  (E.g. exchange_id
> > needs to return nfserr_notsupp on minorversion=0.)
> >
> 
> nfsd4_verify_opnum() should do some of that, but I'll add the checks to 
> the new operations since it probably makes more sense there.  Should I 
> return nfserr_notsupp or nfserr_illegal in this case?

Doh, sorry I was obviously reading too fast!  What you've got looks
fine, no need to add checks to each operation.

As a nit, I'd rather avoid when possible returning an error value that's
ignored:

> >> +static __be32 nfsd4_verify_opnum(struct nfsd4_compoundargs *argp, struct nfsd4_op *op)
> >> +{
> >> +	if (op->opnum < FIRST_NFS4_OP || op->opnum > LAST_NFS4_OP)
> >> +		return nfserr_op_illegal;
> >> +	else if (argp->minorversion == 0 && op->opnum > OP_RELEASE_LOCKOWNER)
> >> +		return nfserr_op_illegal;
> >> +	else if (argp->minorversion == 1 && op->opnum > OP_RECLAIM_COMPLETE)
> >> +		return nfserr_op_illegal;
> >> +	return nfs_ok;
> >> +}
...
> >> +		if (nfsd4_verify_opnum(argp, op) == nfs_ok)
> >> +			op->status = nfsd4_dec_ops[op->opnum](argp, &op->u);
> >>  		else {
> >>  			op->opnum = OP_ILLEGAL;
> >>  			op->status = nfserr_op_illegal;

How about making it something like "static bool nfsd4_opnum_in_range"?

--b.
--
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
[prev in list] [next in list] [prev in thread] [next in thread]