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

[Boaz Harrosh <bharrosh@panasas.com>]

On 08/24/2009 03:59 PM, Trond Myklebust wrote:
> On Mon, 2009-08-24 at 15:50 +0300, Boaz Harrosh wrote:
>> On 08/24/2009 02:56 PM, Trond Myklebust wrote:
>>> On Mon, 2009-08-24 at 10:27 +0300, Boaz Harrosh wrote:
>>>> OK I used the new stuff by now, and I'm happy with everything
>>>> but the above. I *absolutely* insist on this changing to:
>>>
>>> Feel quite free to insist, but the patch isn't going in.
>>>
>>
>> I think your being irrational here. I'm not the first that wants this, it has
>> been brought up again and again on the mailing list. The big majority of nfs/xdr
>> programmers want this. In fact you are the *only one* who's against it.
> 
> I saw your call for a show of hands, but have yet to see any replies. It
> is probably a bit early for you to start talking about big majorities...
> 
> In any case, I don't apply patches based on popular vote. I apply them
> based on my conviction that they are useful.
> 

I think you need a reality check. Just look in the mailing list archives.

>>>> 	p = xdr_encode_word(p, foo);
>>>> and 
>>>> 	p = xdr_decode_word(p, &foo);
>>>>
>>>> [xdr_{encode,decode}_word is defined differently but is only used in a couple
>>>>  of sunrpc files, the change of these places shall be added to this cleanup]
>>>>
>>>> I have checked this version of the definition:
>>>>  static inline __be32 *
>>>>  xdr_encode_word(__be32 *p, __u32 val)
>>>>  {
>>>> 	*p++ = cpu_to_be32(val);
>>>> 	return p;
>>>>  }
>>>>
>>>>  static inline __be32 *
>>>>  xdr_decode_word(__be32 *p, __u32 *valp)
>>>>  {
>>>> 	*valp = be32_to_cpu(*p++);
>>>> 	return p;
>>>>  }
>>>>
>>>> under assembly with gcc -O2 and it gives the exact same result as the open code,
>>>> so I do not see what can be said against it?
>>>
>>> It is unnecessary, 
>>
>> Yes it is necessary, for the reader. (And the writer).
>>
>> it looks ugly, 
>>
>> I think it is not, that's a matter of taste, no? The opposite is true, your
>> option is the ugly one.
>>
>>> the latter form takes 2 pointers and
>>> hides an assignment, it does an unconditional pointer increment.
>>>
>>
>> So do all the other xdr_{de,en}code_xxx I don't see a choice. Some eggs most
>> be broken when making a cake. It is totally uniform with the reset of the code
>> which is my point. I don't want an alien looking code in the mids of very uniform
>> bunch. And I don't want distracting information, I want the essence stated clearly.
>> I don't write assembly and I don't right computer language, I write English (conforming
>> to computer logic).
>>
>>> IOW: Just learn the meaning of 'pointer to __be32'.
>>>
>>
>> What? I lost you, I don't understand what you mean?
> 
> I mean that the fundamental type of XDR is a __be32. We are using that
> fundamental type in standard C code.

I heavily use beXX types in code totally unrelated to XDR. Please don't hijack
_beXX to mean XDR, for me they are just unrelated overlapping subjects.

> The fundamental Linux function for converting from a u32 to a __be32 is
> cpu_to_be32(). It doesn't need a new wrapper, and neither does
> be32_to_cpu().
> 

And that is exactlly why I use them inside "my" xdr-wrapper. But the xdr wrapper
for me is to do with calling convention, uniformity of code, and statement of intent.
My wrapper is not about byte-ness it is about calling convention and that pointer arithmetic
which you don't like, and I do.

> 

Boaz