Re: [PATCH 06/14] SUNRPC: introduce rpcbind: replacement for in-kernel portmapper

[Chuck Lever <chuck.lever@oracle.com>]

[-- Attachment #1: Type: text/plain, Size: 2218 bytes --]

Christoph Hellwig wrote:
> On Thu, Jan 18, 2007 at 06:30:12PM -0500, Chuck Lever wrote:
>> Introduce a replacement for the in-kernel portmapper client that supports
>> all 3 versions of the rpcbind protocol.  This code is not used yet.
>>
>> Original code by Groupe Bull updated for the latest kernel, with multiple
>> bug fixes.
>>
>> Note that rpcb_clnt.c does not yet support registering via versions 3 and
>> 4 of the rpcbind protocol.
> 
> I don't quite like the way you introduce rcpbind.  It exports the same
> API as the old portmap service, just using the rpcb_ prefix instead
> of rpc_.  Why don't you transpar�ntly switch over the implementation to
> the new model?

Using a different name means both the old and new implementation can 
coexist in the kernel until we're sure the new one is working correctly. 
    This is a widely-used change management technique.

In addition, the new naming echoes the API naming scheme used in user space.

> More comments on the actual code:
> 
>> --- /dev/null
>> +++ b/net/sunrpc/rpcb_clnt.c
>> @@ -0,0 +1,539 @@
>> +/*
>> + * linux/net/sunrpc/rpcb_clnt.c
> 
> Please don't mention the filename in the top of file comment.  It's useless
> and even worse easily gets out of sync and confuses people then.

Done.

>> +static inline struct rpcbind_args *rpcb_map_alloc(void)
>> +{
>> +	return kzalloc(sizeof(struct rpcbind_args), GFP_ATOMIC);
>> +}
>> +
>> +static inline void rpcb_map_free(struct rpcbind_args *map)
>> +{
>> +	kfree(map);
>> +}
> 
> Please kill these useless wrappers.

I don't agree that these are useless.  This is a common defensive 
programming technique that is used throughout the Linux NFS and RPC 
implementation.  Placing these two functions next to each other 
documents in 9 lines of code which memory pool is used for rpcb_maps, 
and ensures that if the memory pool is changed, every instance of 
allocation and release is affected.  It also enforces the pointer types 
automatically.

>> +static struct rpc_clnt *rpcb_create(char *hostname, struct sockaddr *srvaddr, int proto, int version, int privileged)
> 
> Please make sure to properly break lines after 80 chars.

Done.

[-- Attachment #2: chuck.lever.vcf --]
[-- Type: text/x-vcard, Size: 276 bytes --]

begin:vcard
fn:Chuck Lever
n:Lever;Chuck
org:Oracle Corporation;Corporate Architecture Linux Projects Group
email;internet:chuck dot lever at nospam oracle dot com
title:Principle Member of Staff
tel;work:+1 248 614 5091
x-mozilla-html:FALSE
version:2.1
end:vcard


[-- Attachment #3: Type: text/plain, Size: 347 bytes --]

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

[-- Attachment #4: Type: text/plain, Size: 140 bytes --]

_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs