On Jan 6, 2014, at 17:53, Dr Fields James Bruce <bfields@xxxxxxxxxxxx> 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@xxxxxxxxxxxx> 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. I really don’t like those rpc_create_args hacks that introduce fields that are completely private to nfsd. Trond-- 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
