2008-05-05 22:41:41

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [PATCH 7/17] svcrdma: Fix error handling during listening endpoint creation

On Fri, May 02, 2008 at 11:28:39AM -0500, Tom Tucker wrote:
> A listening endpoint isn't known to the generic transport switch until
> the svc_create_xprt function returns without error. Calling
> svc_xprt_put within the xpo_create function causes the module reference
> count to be erroneously decremented.

There's some redundant code in these three error paths; would the usual
kernel-style "goto cleanup" thing help?

--b.

>
> Signed-off-by: Tom Tucker <[email protected]>
>
> ---
> net/sunrpc/xprtrdma/svc_rdma_transport.c | 6 +++---
> 1 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c b/net/sunrpc/xprtrdma/svc_rdma_transport.c
> index 83818cf..0d9a828 100644
> --- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
> +++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
> @@ -669,7 +669,7 @@ static struct svc_xprt *svc_rdma_create(struct svc_serv *serv,
>
> listen_id = rdma_create_id(rdma_listen_handler, cma_xprt, RDMA_PS_TCP);
> if (IS_ERR(listen_id)) {
> - svc_xprt_put(&cma_xprt->sc_xprt);
> + kfree(cma_xprt);
> dprintk("svcrdma: rdma_create_id failed = %ld\n",
> PTR_ERR(listen_id));
> return (void *)listen_id;
> @@ -677,7 +677,7 @@ static struct svc_xprt *svc_rdma_create(struct svc_serv *serv,
> ret = rdma_bind_addr(listen_id, sa);
> if (ret) {
> rdma_destroy_id(listen_id);
> - svc_xprt_put(&cma_xprt->sc_xprt);
> + kfree(cma_xprt);
> dprintk("svcrdma: rdma_bind_addr failed = %d\n", ret);
> return ERR_PTR(ret);
> }
> @@ -686,7 +686,7 @@ static struct svc_xprt *svc_rdma_create(struct svc_serv *serv,
> ret = rdma_listen(listen_id, RPCRDMA_LISTEN_BACKLOG);
> if (ret) {
> rdma_destroy_id(listen_id);
> - svc_xprt_put(&cma_xprt->sc_xprt);
> + kfree(cma_xprt);
> dprintk("svcrdma: rdma_listen failed = %d\n", ret);
> return ERR_PTR(ret);
> }


2008-05-06 14:48:21

by Tom Tucker

[permalink] [raw]
Subject: Re: [PATCH 7/17] svcrdma: Fix error handling during listening endpoint creation




On 5/5/08 5:41 PM, "J. Bruce Fields" <[email protected]> wrote:

> On Fri, May 02, 2008 at 11:28:39AM -0500, Tom Tucker wrote:
>> A listening endpoint isn't known to the generic transport switch until
>> the svc_create_xprt function returns without error. Calling
>> svc_xprt_put within the xpo_create function causes the module reference
>> count to be erroneously decremented.
>
> There's some redundant code in these three error paths; would the usual
> kernel-style "goto cleanup" thing help?

I think code-size it's a wash, but it might look more familiar. BTW, I
noticed that the error path above returns a positive errno :-\, so that
needs to be fixed too.

>
> --b.
>
>>
>> Signed-off-by: Tom Tucker <[email protected]>
>>
>> ---
>> net/sunrpc/xprtrdma/svc_rdma_transport.c | 6 +++---
>> 1 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c
>> b/net/sunrpc/xprtrdma/svc_rdma_transport.c
>> index 83818cf..0d9a828 100644
>> --- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
>> +++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
>> @@ -669,7 +669,7 @@ static struct svc_xprt *svc_rdma_create(struct svc_serv
>> *serv,
>>
>> listen_id = rdma_create_id(rdma_listen_handler, cma_xprt, RDMA_PS_TCP);
>> if (IS_ERR(listen_id)) {
>> - svc_xprt_put(&cma_xprt->sc_xprt);
>> + kfree(cma_xprt);
>> dprintk("svcrdma: rdma_create_id failed = %ld\n",
>> PTR_ERR(listen_id));
>> return (void *)listen_id;
>> @@ -677,7 +677,7 @@ static struct svc_xprt *svc_rdma_create(struct svc_serv
>> *serv,
>> ret = rdma_bind_addr(listen_id, sa);
>> if (ret) {
>> rdma_destroy_id(listen_id);
>> - svc_xprt_put(&cma_xprt->sc_xprt);
>> + kfree(cma_xprt);
>> dprintk("svcrdma: rdma_bind_addr failed = %d\n", ret);
>> return ERR_PTR(ret);
>> }
>> @@ -686,7 +686,7 @@ static struct svc_xprt *svc_rdma_create(struct svc_serv
>> *serv,
>> ret = rdma_listen(listen_id, RPCRDMA_LISTEN_BACKLOG);
>> if (ret) {
>> rdma_destroy_id(listen_id);
>> - svc_xprt_put(&cma_xprt->sc_xprt);
>> + kfree(cma_xprt);
>> dprintk("svcrdma: rdma_listen failed = %d\n", ret);
>> return ERR_PTR(ret);
>> }
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html



2008-05-06 21:22:50

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [PATCH 7/17] svcrdma: Fix error handling during listening endpoint creation

On Tue, May 06, 2008 at 09:48:06AM -0500, Tom Tucker wrote:
>
>
>
> On 5/5/08 5:41 PM, "J. Bruce Fields" <[email protected]> wrote:
>
> > On Fri, May 02, 2008 at 11:28:39AM -0500, Tom Tucker wrote:
> >> A listening endpoint isn't known to the generic transport switch until
> >> the svc_create_xprt function returns without error. Calling
> >> svc_xprt_put within the xpo_create function causes the module reference
> >> count to be erroneously decremented.
> >
> > There's some redundant code in these three error paths; would the usual
> > kernel-style "goto cleanup" thing help?
>
> I think code-size it's a wash, but it might look more familiar.

Yeah, we try to stick to the same idioms as used elsewhere when it's
easy to.

Doesn't have to be done now, though.

> BTW, I noticed that the error path above returns a positive errno :-\,
> so that needs to be fixed too.

Oops, didn't catch that. I'll expect a patch?

--b.

2008-05-07 00:48:16

by Tom Tucker

[permalink] [raw]
Subject: Re: [PATCH 7/17] svcrdma: Fix error handling during listening endpoint creation




On 5/6/08 4:22 PM, "J. Bruce Fields" <[email protected]> wrote:

> On Tue, May 06, 2008 at 09:48:06AM -0500, Tom Tucker wrote:
>>
>>
>>
>> On 5/5/08 5:41 PM, "J. Bruce Fields" <[email protected]> wrote:
>>
>>> On Fri, May 02, 2008 at 11:28:39AM -0500, Tom Tucker wrote:
>>>> A listening endpoint isn't known to the generic transport switch until
>>>> the svc_create_xprt function returns without error. Calling
>>>> svc_xprt_put within the xpo_create function causes the module reference
>>>> count to be erroneously decremented.
>>>
>>> There's some redundant code in these three error paths; would the usual
>>> kernel-style "goto cleanup" thing help?
>>
>> I think code-size it's a wash, but it might look more familiar.
>
> Yeah, we try to stick to the same idioms as used elsewhere when it's
> easy to.
>
> Doesn't have to be done now, though.
>
>> BTW, I noticed that the error path above returns a positive errno :-\,
>> so that needs to be fixed too.
>
> Oops, didn't catch that. I'll expect a patch?

I've got updates to three of the patches. I'm just retesting to make sure I
didn't inadvertently break something. I'll get the three updated patches to
you by tomorrow.

Tom

>
> --b.



2008-05-07 01:30:04

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [PATCH 7/17] svcrdma: Fix error handling during listening endpoint creation

On Tue, May 06, 2008 at 07:48:02PM -0500, Tom Tucker wrote:
>
>
>
> On 5/6/08 4:22 PM, "J. Bruce Fields" <[email protected]> wrote:
>
> > On Tue, May 06, 2008 at 09:48:06AM -0500, Tom Tucker wrote:
> >>
> >>
> >>
> >> On 5/5/08 5:41 PM, "J. Bruce Fields" <[email protected]> wrote:
> >>
> >>> On Fri, May 02, 2008 at 11:28:39AM -0500, Tom Tucker wrote:
> >>>> A listening endpoint isn't known to the generic transport switch until
> >>>> the svc_create_xprt function returns without error. Calling
> >>>> svc_xprt_put within the xpo_create function causes the module reference
> >>>> count to be erroneously decremented.
> >>>
> >>> There's some redundant code in these three error paths; would the usual
> >>> kernel-style "goto cleanup" thing help?
> >>
> >> I think code-size it's a wash, but it might look more familiar.
> >
> > Yeah, we try to stick to the same idioms as used elsewhere when it's
> > easy to.
> >
> > Doesn't have to be done now, though.
> >
> >> BTW, I noticed that the error path above returns a positive errno :-\,
> >> so that needs to be fixed too.
> >
> > Oops, didn't catch that. I'll expect a patch?
>
> I've got updates to three of the patches. I'm just retesting to make sure I
> didn't inadvertently break something. I'll get the three updated patches to
> you by tomorrow.

OK! Thanks. I'm travelling tommorow (then at connectathon for a week)
so apologies in advance if I'm slow.

--b.