Re: [pnfs] [PATCH RFC v2 0/21] nfs4xdr cleanup v2

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, 2009-08-17 at 13:40 +0300, Boaz Harrosh wrote:
> * So I hate it that the READ32/WRITE32 is open coded. Now it looks like nothing, it
>   should have an xdr_encode/decode naming convention, all details hidden, consistent calling
>   convention with the reset of the code.

The convention is already established. Please see 15 years worth of
NFSv2/v3 xdr code. The NFSv4 code was the exception not the rule.

Now, it is obvious what we're doing: we're writing 32-bit big endian
encoded data into a memory buffer. Furthermore, the process of
converting from cpu ordered integers into 32-bit big endian is now made
obvious, making it easily verifiable both by inspection, and using
'sparse'.
Adding wrappers to do this, is just unnecessarily obfuscating the code.

> * I hate it that I have to count (p)"++" now to look for actual code advancements.

How is that _any_ different from counting WRITE32s?

> * I hate that it is called encode_hyper that a dum like me needs to look it up in the dictionary
>   "32" and "64" will do

I don't mind changing the name of xdr_encode_hyper() to
xdr_encode_uint64 or some such alternative, however the point is that we
should have only _one_ function that encodes 64 bit integers. Not one
function that everyone else uses, and then a macro that NFSv4 uses.

> * I hated it before, but I hate it even more now that the same operation is:
>   reserved(4); p + 1

Huh? What operation?

>   I wish it could be all __be32 everywhere

??? That's up to the protocol. If the protocol uses 32-bit integers,
then we can use __be32, but if it specifies a 64-bit big endian integer
in some structure, then we need to encode for that.

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@xxxxxxxxxx
www.netapp.com
--
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

[Index of Archives]     [Linux Filesystem Development]     [Linux USB Development]     [Linux Media Development]     [Video for Linux]     [Linux NILFS]     [Linux Audio Users]     [Yosemite Info]     [Linux SCSI]

  Powered by Linux