Re: [PATCH 2.6.30] svcrdma: clean up error paths.

["J. Bruce Fields" <bfields@fieldses.org>]

On Wed, May 13, 2009 at 05:42:23PM -0500, Steve Wise wrote:
> Hey Bruce,
>
> J. Bruce Fields wrote:
>> On Wed, Apr 29, 2009 at 02:14:00PM -0500, Steve Wise wrote:
>>   
>>> These fixes resolved crashes due to resource leak BUG_ON checks. The
>>> resource leaks were detected by introducing asynchronous transport errors.
>>>     
>>
>> Thanks, applied for 2.6.30.  (And also appropriate for stable (2.6.29),
>> I assume?)
>>
>> But, could someone take a closer look at the error paths here?  Questions:
>>
>> 	- svc_rdma_post_recv() does a svc_rdma_put_context() on error--
>> 	  are you sure its caller needs to as well?
>>   
>
> The svc_rdma_put_context() call inside svc_rdma_post_recv() is for the  
> recv context that was allocated inside that function.  The caller, in  
> this case send_reply() also does a svc_rdma_put_context(), but that is  
> for the send context.   So I think this is correct.
>
>> 	- In send_reply, some of the cleanout is shared between the
>> 	  first return -ENOTCONN and the final err: cleanup.  Could we
>> 	  add another err: label and share some of that cleanup?
>>   
>
> The only common logic I see is the svc_rdma_put_context() call that  
> could be shared.  But one case calls it with free_pages == 1 after the  
> pages have been mapped, and the other with 0 since no pages are mapped  
> at that point (when the call to svc_rdma_post_recv() fails).   So I'm  
> not sure its worth doing?

No, I think you're probably right about both of these.  Thanks for
taking a look.

--b.