SUNRPC: Clear xpt_bc_xprt if xs_setup_bc_tcp failed

PATCH In review Kinglong Mee 3 participants 7 messages Jan 7, 2014 – Jan 10, 2014

Subsystems (3)

Networking NFS client NFS server

lore

Share & tools

lore

Kinglong Mee

author

2014-01-07 04:41 UTC #0 lore raw

PATCH 1/1 SUNRPC: Clear xpt_bc_xprt if xs_setup_bc_tcp failed

If try_module_get failed, xpt_bc_xprt should be set to NULL, because xprt will be free.

Don't needed using xprt_put to free xprt, because it is always new.

Signed-off-by Kinglong Mee <[email protected]>

---
 net/sunrpc/xprtsock.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

net/sunrpc/xprtsock.c +2 -2

index 4fcdf74..5ed124f 100644
@@ -2986,10 +2986,10 @@ static struct rpc_xprt *xs_setup_bc_tcp(struct xprt_create *args)
 	 */
 	xprt_set_connected(xprt);
 
-
 	if (try_module_get(THIS_MODULE))
 		return xprt;
-	xprt_put(xprt);
+
+	args->bc_xprt->xpt_bc_xprt = NULL;
 	ret = ERR_PTR(-EINVAL);
 out_err:
 	xs_xprt_free(xprt);

Signature · 1 line

1.8.4.2

Trond Myklebust

2014-01-07 05:01 UTC #1 lore raw

Quoted 26 lines · Kinglong Mee

If try_module_get failed, xpt_bc_xprt should be set to NULL, because xprt will be free.
Don't needed using xprt_put to free xprt, because it is always new.
Signed-off-by Kinglong Mee <[email protected]>
---
net/sunrpc/xprtsock.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
net/sunrpc/xprtsock.c +2 -2
index 4fcdf74..5ed124f 100644
@@ -2986,10 +2986,10 @@ static struct rpc_xprt *xs_setup_bc_tcp(struct xprt_create *args)
 	 */
 	xprt_set_connected(xprt);
 
-
 	if (try_module_get(THIS_MODULE))
 		return xprt;
-	xprt_put(xprt);
+
+	args->bc_xprt->xpt_bc_xprt = NULL;
 	ret = ERR_PTR(-EINVAL);
out_err:

Quoted 1 line

	xs_xprt_free(xprt);

Just move the 'args->bc_xprt->xpt_bc_xprt = xprt’ line into the caller (see earlier discussion about xs_setup_bc_tcp()). Quite frankly, I don’t see why the client code should be modifying this variable in the first place. An svc_xprt is a server construct.

Trond

Kinglong Mee

author

2014-01-07 05:22 UTC #2 lore raw

Quoted 21 lines · Trond Myklebust

Quoted 18 lines · Kinglong Mee
If try_module_get failed, xpt_bc_xprt should be set to NULL, because xprt will be free.
Don't needed using xprt_put to free xprt, because it is always new.
Signed-off-by Kinglong Mee <[email protected]>
---
net/sunrpc/xprtsock.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
net/sunrpc/xprtsock.c +0 -0
index 4fcdf74..5ed124f 100644
@@ -2986,10 +2986,10 @@ static struct rpc_xprt *xs_setup_bc_tcp(struct xprt_create *args)
 	 */
 	xprt_set_connected(xprt);
 

Quoted 15 lines

Quoted 9 lines
net/sunrpc/xprtsock.c +2 -2
-
 	if (try_module_get(THIS_MODULE))
 		return xprt;
-	xprt_put(xprt);
+
+	args->bc_xprt->xpt_bc_xprt = NULL;
 	ret = ERR_PTR(-EINVAL);
out_err:
	xs_xprt_free(xprt);
Just move the 'args->bc_xprt->xpt_bc_xprt = xprt’ line into the caller
(see earlier discussion about xs_setup_bc_tcp()).
Quite frankly, I don’t see why the client code should be modifying this variable in the first place. An svc_xprt is a server construct.

That's really better.

I will check those codes for NFSv4.1 backchannel in sunrpc, maybe I will cleanup the client code from svc_xprt.

thanks,
Kinglong Mee

Kinglong Mee

author

2014-01-07 07:26 UTC #3 lore raw

In reply to Trond Myklebust

Quoted 21 lines · Trond Myklebust

Quoted 18 lines · Kinglong Mee
If try_module_get failed, xpt_bc_xprt should be set to NULL, because xprt will be free.
Don't needed using xprt_put to free xprt, because it is always new.
Signed-off-by Kinglong Mee <[email protected]>
---
net/sunrpc/xprtsock.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
net/sunrpc/xprtsock.c +0 -0
index 4fcdf74..5ed124f 100644
@@ -2986,10 +2986,10 @@ static struct rpc_xprt *xs_setup_bc_tcp(struct xprt_create *args)
 	 */
 	xprt_set_connected(xprt);
 

Quoted 11 lines

Quoted 9 lines
net/sunrpc/xprtsock.c +2 -2
-
 	if (try_module_get(THIS_MODULE))
 		return xprt;
-	xprt_put(xprt);
+
+	args->bc_xprt->xpt_bc_xprt = NULL;
 	ret = ERR_PTR(-EINVAL);
out_err:
	xs_xprt_free(xprt);
Just move the 'args->bc_xprt->xpt_bc_xprt = xprt’ line into the caller (see earlier discussion about xs_setup_bc_tcp()). Quite frankly, I don’t see why the client code should be modifying this variable in the first place. An svc_xprt is a server construct.

Hi Bruce,

When reviewing those codes for bc_xprt, I found commit d75faea330dbd1873c9094e9926ae306590c0998 'rpc: move sk_bc_xprt to svc_xprt'. I'd like recording bc_xprt in svc_sock.

thanks,
Kinglong Mee

Dr Fields James Bruce

2014-01-09 16:34 UTC #4 lore raw

Quoted 34 lines · Kinglong Mee

Quoted 21 lines · Trond Myklebust
Quoted 18 lines · Kinglong Mee
If try_module_get failed, xpt_bc_xprt should be set to NULL, because xprt will be free.
Don't needed using xprt_put to free xprt, because it is always new.
Signed-off-by Kinglong Mee <[email protected]>
---
net/sunrpc/xprtsock.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
net/sunrpc/xprtsock.c +0 -0
index 4fcdf74..5ed124f 100644
@@ -2986,10 +2986,10 @@ static struct rpc_xprt *xs_setup_bc_tcp(struct xprt_create *args)
 	 */
 	xprt_set_connected(xprt);
 
Quoted 11 lines
Quoted 9 lines
net/sunrpc/xprtsock.c +2 -2
-
 	if (try_module_get(THIS_MODULE))
 		return xprt;
-	xprt_put(xprt);
+
+	args->bc_xprt->xpt_bc_xprt = NULL;
 	ret = ERR_PTR(-EINVAL);
out_err:
	xs_xprt_free(xprt);
Just move the 'args->bc_xprt->xpt_bc_xprt = xprt’ line into the caller (see earlier discussion about xs_setup_bc_tcp()). Quite frankly, I don’t see why the client code should be modifying this variable in the first place. An svc_xprt is a server construct.

Quoted 4 lines

Hi Bruce,
When reviewing those codes for bc_xprt, I found commit d75faea330dbd1873c9094e9926ae306590c0998 'rpc: move sk_bc_xprt to svc_xprt'. I'd like recording bc_xprt in svc_sock.

OK, I think it would be alright to revert that commit.

--b.

Kinglong Mee

author

2014-01-10 02:43 UTC #5 lore raw

Quoted 35 lines · Dr Fields James Bruce

Quoted 34 lines · Kinglong Mee
Quoted 32 lines · Trond Myklebust
Quoted 26 lines · Kinglong Mee
If try_module_get failed, xpt_bc_xprt should be set to NULL, because xprt will be free.
Don't needed using xprt_put to free xprt, because it is always new.
Signed-off-by Kinglong Mee <[email protected]>
---
net/sunrpc/xprtsock.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
net/sunrpc/xprtsock.c +2 -2
index 4fcdf74..5ed124f 100644
@@ -2986,10 +2986,10 @@ static struct rpc_xprt *xs_setup_bc_tcp(struct xprt_create *args)
 	 */
 	xprt_set_connected(xprt);
 
-
 	if (try_module_get(THIS_MODULE))
 		return xprt;
-	xprt_put(xprt);
+
+	args->bc_xprt->xpt_bc_xprt = NULL;
 	ret = ERR_PTR(-EINVAL);
out_err:
Quoted 1 line
	xs_xprt_free(xprt);
Just move the 'args->bc_xprt->xpt_bc_xprt = xprt’ line into the caller (see earlier discussion about xs_setup_bc_tcp()). Quite frankly, I don’t see why the client code should be modifying this variable in the first place. An svc_xprt is a server construct.

Quoted 6 lines

Quoted 4 lines
Hi Bruce,
When reviewing those codes for bc_xprt, I found commit d75faea330dbd1873c9094e9926ae306590c0998 'rpc: move sk_bc_xprt to svc_xprt'. I'd like recording bc_xprt in svc_sock.
OK, I think it would be alright to revert that commit.

I will try to revert it later.

Thanks,
Kinglong Mee

Kinglong Mee

author

2014-01-10 03:22 UTC #6 lore raw

Quoted 36 lines · Kinglong Mee

Quoted 35 lines · Dr Fields James Bruce
Quoted 34 lines · Kinglong Mee
Quoted 32 lines · Trond Myklebust
Quoted 26 lines · Kinglong Mee
If try_module_get failed, xpt_bc_xprt should be set to NULL, because xprt will be free.
Don't needed using xprt_put to free xprt, because it is always new.
Signed-off-by Kinglong Mee <[email protected]>
---
net/sunrpc/xprtsock.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
net/sunrpc/xprtsock.c +2 -2
index 4fcdf74..5ed124f 100644
@@ -2986,10 +2986,10 @@ static struct rpc_xprt *xs_setup_bc_tcp(struct xprt_create *args)
 	 */
 	xprt_set_connected(xprt);
 
-
 	if (try_module_get(THIS_MODULE))
 		return xprt;
-	xprt_put(xprt);
+
+	args->bc_xprt->xpt_bc_xprt = NULL;
 	ret = ERR_PTR(-EINVAL);
out_err:
Quoted 1 line
	xs_xprt_free(xprt);
Just move the 'args->bc_xprt->xpt_bc_xprt = xprt’ line into the caller (see earlier discussion about xs_setup_bc_tcp()). Quite frankly, I don’t see why the client code should be modifying this variable in the first place. An svc_xprt is a server construct.

Quoted 8 lines

Quoted 6 lines
Quoted 4 lines
Hi Bruce,
When reviewing those codes for bc_xprt, I found commit d75faea330dbd1873c9094e9926ae306590c0998 'rpc: move sk_bc_xprt to svc_xprt'. I'd like recording bc_xprt in svc_sock.
OK, I think it would be alright to revert that commit.
I will try to revert it later.

I had try to revert it, but, I don't revert it.
I'm sorry for my above thinking.
I think it is not needed to move bc_xprt from one place to another without fix some problem, because knfsd running correctly with that.

Just let bc_xprt in svc_xprt until some problem appears.

Thanks,
Kinglong Mee