Re: [PATCH] SUNRPC: fix a memory leak for tcp NFSv4.1 backchannel

[Dr Fields James Bruce <bfields@fieldses.org>]

On Mon, Jan 06, 2014 at 06:28:38PM -0500, Trond Myklebust wrote:
> 
> On Jan 6, 2014, at 17:53, Dr Fields James Bruce <bfields@fieldses.org>
> wrote:
> 
> > On Mon, Jan 06, 2014 at 05:40:03PM -0500, Trond Myklebust wrote:
> >> 
> >> On Jan 6, 2014, at 13:49, J. Bruce Fields <bfields@fieldses.org>
> >> wrote:
> >> 
> >>> On Mon, Jan 06, 2014 at 05:33:22PM +0800, Kinglong Mee wrote:
> >>>> xs_setup_bc_tcp may return an existing xprt with non-NULL
> >>>> servername.  xprt_create_transport should not kstrdup servername
> >>>> for it.  Otherwise, those memory for servername will be leaked.
> >>> 
> >>> OK.  Applying to my tree if Trond has no objection.
> >> 
> >> Actually. Why do we go through all this code at all if
> >> xs_setup_bc_tcp() returns args->bc_xprt->xpt_bc_xprt? I’m assuming
> >> that is the only case where xprt->servername != NULL, right?
> >> 
> >> For instance, won’t calling INIT_WORK() be a source of problems?
> > 
> > Huh.  Looking at the history....  There used to be a
> > 
> > 	if (test_and_set_bit(XPRT_INITIALIZED, &xprt->state)) /* ->setup
> > 	returned a pre-initialized xprt: */ return xprt;
> > 
> > here, but it got removed by 21de0a955f3af29fa1100d96f66e6adade89e77a
> > "SUNRPC: Clean up the slot table allocation", which looks otherwise
> > unrelated.  Was that just some kind of rebasing mistake, or was
> > there a reason for that?
> 
> I probably misunderstood that bc_xprt sends a fully initialized struct
> rpc_xprt.
> 
> The obvious question if that is the case, is why we are calling
> xprt_create_transport() at all? If .bc_xprt->xpt_bc_xprt contains a
> fully initialized struct rpc_xprt, then just have rpc_create() do the
> honors.  Better yet, create a svc_create_backchannel_client() helper
> that calls rpc_new_client() with the correct parameters.

Yes, that sounds great.  Looking at rpc_create, it could be that
everything there outside the rpc_new_client call is useless.

--b.

> I really
> don’t like those rpc_create_args hacks that introduce fields that are
> completely private to nfsd.