Re: [RFC,PATCH 03/20] svc: xpt_prep_reply_hdr

[Tom Tucker <tom@opengridcomputing.com>]

On 8/29/07 12:15 PM, "Chuck Lever" <chuck.lever@oracle.com> wrote:

> Tom Tucker wrote:
>> Add a transport function that prepares the transport specific header for
>> RPC replies. UDP has none, TCP has a 4B record length. This will
>> allow the RDMA transport to prepare it's variable length reply
>> header as well.
>> 
>> Signed-off-by: Tom Tucker <tom@opengridcomputing.com>
>> ---
>> 
>>  include/linux/sunrpc/svcsock.h |    4 ++++
>>  net/sunrpc/svc.c               |    8 +++++---
>>  net/sunrpc/svcsock.c           |   15 +++++++++++++++
>>  3 files changed, 24 insertions(+), 3 deletions(-)
>> 
>> diff --git a/include/linux/sunrpc/svcsock.h b/include/linux/sunrpc/svcsock.h
>> index 27c5b1f..1da42c2 100644
>> --- a/include/linux/sunrpc/svcsock.h
>> +++ b/include/linux/sunrpc/svcsock.h
>> @@ -27,6 +27,10 @@ struct svc_xprt {
>> * destruction of a svc_sock.
>> */
>> void   (*xpt_free)(struct svc_sock *);
>> + /*
>> +  * Prepare any transport-specific RPC header.
>> +  */
>> + int                     (*xpt_prep_reply_hdr)(struct svc_rqst *);
>>  };
> 
> A shorter name for this one might be nice, but I know it's kind of hard
> to choose one.
> 
>>  /*
>> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
>> index e673ef9..72a900f 100644
>> --- a/net/sunrpc/svc.c
>> +++ b/net/sunrpc/svc.c
>> @@ -815,9 +815,11 @@ svc_process(struct svc_rqst *rqstp)
>> rqstp->rq_res.tail[0].iov_len = 0;
>> /* Will be turned off only in gss privacy case: */
>> rqstp->rq_sendfile_ok = 1;
>> - /* tcp needs a space for the record length... */
>> - if (rqstp->rq_prot == IPPROTO_TCP)
>> -  svc_putnl(resv, 0);
>> +
>> + /* setup response header. */
>> + if (rqstp->rq_sock->sk_xprt->xpt_prep_reply_hdr &&
>> +     rqstp->rq_sock->sk_xprt->xpt_prep_reply_hdr(rqstp))
>> +  goto dropit;
> 
> Although others might disagree, I like making all the transport methods
> mandatory.
> 

I think so too. Especially if some are mandatory and some are not.

> The TCP transport will be the common case here, and that will always
> have to do both the existence check and the call.
> 
> What is the purpose of the return code?  Does the RDMA header
> constructor return something other than zero, and if so, why?
>

The thought was that some transports might need to do something (allocate
memory) that might fail. For the current set of transports, they never fail.
This is probably over-design. Make it void?
 
> And finally a dumb style nit... I generally don't like the extra comment
> here -- it restates what is already clear.
>

True. Unless we shorten the function name to something cryptic ;-)
 
>> rqstp->rq_xid = svc_getu32(argv);
>> svc_putu32(resv, rqstp->rq_xid);
>> diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
>> index 4956c88..ca473ee 100644
>> --- a/net/sunrpc/svcsock.c
>> +++ b/net/sunrpc/svcsock.c
>> @@ -1326,12 +1326,27 @@ svc_tcp_sendto(struct svc_rqst *rqstp)
>> return sent;
>>  }
>>  
>> +/*
>> + * Setup response header. TCP has a 4B record length field.
>> + */
>> +static int
>> +svc_tcp_prep_reply_hdr(struct svc_rqst *rqstp)
>> +{
>> + struct kvec *resv = &rqstp->rq_res.head[0];
>> +
>> + /* tcp needs a space for the record length... */
>> + svc_putnl(resv, 0);
>> +
>> + return 0;
>> +}
>> +
>>  static const struct svc_xprt svc_tcp_xprt = {
>> .xpt_name = "tcp",
>> .xpt_recvfrom = svc_tcp_recvfrom,
>> .xpt_sendto = svc_tcp_sendto,
>> .xpt_detach = svc_sock_detach,
>> .xpt_free = svc_sock_free,
>> + .xpt_prep_reply_hdr = svc_tcp_prep_reply_hdr,
>>  };
>>  
>>  static void
>> 
>> -------------------------------------------------------------------------
>> This SF.net email is sponsored by: Splunk Inc.
>> Still grepping through log files to find problems?  Stop.
>> Now Search log events and configuration files using AJAX and a browser.
>> Download your FREE copy of Splunk now >>  http://get.splunk.com/
>> _______________________________________________
>> NFS maillist  -  NFS@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/nfs
> 



-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >>  http://get.splunk.com/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs