Re: [PATCH v2 2/2] NFSD: Implement SEEK

[Anna Schumaker <Anna.Schumaker@netapp.com>]

On 09/09/2014 01:56 PM, Christoph Hellwig wrote:
>> +	switch (seek->seek_whence) {
>> +	case NFS4_CONTENT_DATA:
>> +		seek->seek_pos = vfs_llseek(file, seek->seek_offset, SEEK_DATA);
>> +		break;
>> +	case NFS4_CONTENT_HOLE:
>> +		seek->seek_pos = vfs_llseek(file, seek->seek_offset, SEEK_HOLE);
>> +		break;
>> +	default:
>> +		status = nfserr_union_notsupp;
>> +		goto out;
>> +	}
> 
> nipick: maybe just assign pos in the switch, and have a single
> vfs_llseek call?
> 
> Also this might want a comment that vfs_llseek is changing file->f_pos,
> but nothing in NFSD should ever rely on file->f_pos.

Sure.

> 
>>  static __be32
>> +nfsd4_encode_seek(struct nfsd4_compoundres *resp, __be32 nfserr,
>> +		  struct nfsd4_seek *seek)
>> +{
>> +	__be32 *p;
>> +
>> +	if (nfserr)
>> +		return nfserr;
>> +
>> +	p = xdr_reserve_space(&resp->xdr, 12);
> 
> nipick: can you replace the 12 by a "4 + 8"?  I think having one
> literal for each field later encoded helps reading the XDR code a lot.
> And although it might not matter for a trivial encoder like this it
> set standards for future copy and paste code.

I can change that, too.  Thanks for looking!

Anna
>