[pnfs] [PATCH 3/8] Added a function to read the call/reply flag

[Iyer, Rahul]

Hi Benny,
I agree. Andy, Can you add this one liner change to my patches or should
I send this in as a separate patch? 
Thanks,
Regards
Rahul


> -----Original Message-----
> From: Benny Halevy [mailto:bhalevy at panasas.com] 
> Sent: Saturday, April 14, 2007 10:55 PM
> To: Iyer, Rahul
> Cc: pnfs at linux-nfs.org
> Subject: Re: [pnfs] [PATCH 3/8] Added a function to read the 
> call/reply flag
> 
> It seems like your implementation might break if only the xid is sent.
> The current code for xs_tcp_read_request() will deal with 
> that correctly since it reads up to xprt->tcp_reclen, but 
> xs_tcp_read_calldir() simply reads
> sizeof(xprt->tcp_calldir) (similar to xs_tcp_read_xid()).  
> Therefore I suggest requiring tcp_reclen >= 8 rather than 4 
> in xs_tcp_read_fraghdr().
> I.e.:
> 
> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c 
> index 441bd53..9d679d6 100644
> --- a/net/sunrpc/xprtsock.c
> +++ b/net/sunrpc/xprtsock.c
> @@ -604,7 +604,7 @@ static inline void 
> xs_tcp_read_fraghdr(struct rpc_xprt *xprt, skb_reader_t
>         xprt->tcp_offset = 0;
> 
>         /* Sanity check of the record length */
> -       if (unlikely(xprt->tcp_reclen < 4)) {
> +       if (unlikely(xprt->tcp_reclen < 8)) {
>                 dprintk("RPC:      invalid TCP record 
> fragment length\n");
>                 xprt_disconnect(xprt);
>                 return;
> 
> 
> iyer at netapp.com wrote:
> > From: Rahul Iyer <iyer at netapp.com>
> > 
> > Added a function xs_tcp_read_calldir() to read in the 
> call/reply flag 
> > and store it in xprt->tcp_calldir.
> > 
> > Signed-off-by: Rahul Iyer <iyer at netapp.com>
> > ---
> >  net/sunrpc/xprtsock.c |   28 ++++++++++++++++++++++++++++
> >  1 files changed, 28 insertions(+), 0 deletions(-)
> > 
> > diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c index 
> > 441bd53..ab1e8fb 100644
> > --- a/net/sunrpc/xprtsock.c
> > +++ b/net/sunrpc/xprtsock.c
> > @@ -648,6 +648,34 @@ static inline void 
> xs_tcp_read_xid(struct rpc_xprt *xprt, skb_reader_t *desc)
> >  	xs_tcp_check_recm(xprt);
> >  }
> >  
> > +static inline void xs_tcp_read_calldir(struct rpc_xprt *xprt, 
> > +skb_reader_t *desc) {
> > +	size_t len, used;
> > +	u32 offset;
> > +	char *p;
> > +
> > +	/* The reason for this rather odd calculation is that we want
> > +	 * xprt->tcp_offset to be 8 at the end of this routine 
> (4 bytes for the
> > +	 * xid and 4 bytes for the call/reply flag). When 
> coming into this
> > +	 * function for the first time, xprt->tcp_offset is 4 
> (after reading
> > +	 * the xid).
> > +	 */
> > +	offset = xprt->tcp_offset - sizeof(xprt->tcp_xid);
> > +	len = sizeof(xprt->tcp_calldir) - offset;
> > +	dprintk("RPC:      reading CALL/REPLY flag (%Zu bytes)\n", len);
> > +	p = ((char *) &xprt->tcp_calldir) + offset;
> > +	used = xs_tcp_copy_data(desc, p, len);
> > +	xprt->tcp_offset += used;
> > +	if (used != len)
> > +		return;
> > +	xprt->tcp_flags &= ~XPRT_COPY_CALLDIR;
> > +	xprt->tcp_flags |= XPRT_COPY_DATA;
> > +	xprt->tcp_copied += 4;
> > +	dprintk("RPC:      reading reply for CALL/REPLY flag %08x\n",
> > +						ntohl(xprt->tcp_xid));
> > +	xs_tcp_check_recm(xprt);
> > +}
> > +
> >  static inline void xs_tcp_read_request(struct rpc_xprt *xprt, 
> > skb_reader_t *desc)  {
> >  	struct rpc_rqst *req;
>