On Mon, 2009-08-24 at 09:45 -0400, Chuck Lever wrote:
> On Aug 24, 2009, at 3:27 AM, Boaz Harrosh wrote:
> > 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?
>
> In addition to being consistent with xdr_{encode,decode}_hyper, this
> adds appropriate static data type checking, which we want.
OK. Let's examine that assertion:
AFAICS, xdr_encode_word() doesn't add any typechecking that wasn't
already being performed with *p++=cpu_to_be32(foo). I call "bullshit" on
that...
xdr_decode_word() does indeed force you to use a pointer to __u32
argument. So if you actually wanted to decode say a signed integer, and
store it in a long you will need xdr_decode_int(). How does that help
us?
Do we care about type checking storage of u32 into unsigned long? No
Do we care about type checking storage of u32 into unsigned int? No
Do we care about type checking storage of u32 into signed long or s64?
Maybe, so let's look at the protocol. Where does the protocol require
you to decode a signed value that might make use of this? The only
signed value I see in the protocol is for 'seconds', which is of type
int64. Does that justify introducing type checking on all those
be32_to_cpu() calls? Clearly, no.
So what are the cases where this might help us?
--
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
