[Sheepdog] [PATCH 1/2] sheep: cleanup request list handling

[Liu Yuan]

On 05/15/2012 02:55 PM, Christoph Hellwig wrote:

> On Tue, May 15, 2012 at 02:39:45PM +0800, Liu Yuan wrote:
>> On 05/11/2012 09:19 PM, Christoph Hellwig wrote:
>>
>>> static void queue_request(struct request *req)
>>>  {
>>> -	struct event_struct *cevent = &req->cev;
>>>  	struct sd_req *hdr = (struct sd_req *)&req->rq;
>>>  	struct sd_rsp *rsp = (struct sd_rsp *)&req->rp;
>>>  
>>> +	assert(list_empty(&req->request_list));
>>> +
>>
>>
>> request is allocated in client_rx_handler() just before calling
>> queue_request(), so I think this assertion is useless. We don't need
>> this safe-guard because there is nothing to guard. This path is quit
>> hot, so we'd better off only doing guard when necessary.
> 
> The old code removed it from a list that it was never added to, so
> I placed it in here to verify the assumption that it never should be.
> 
> I'm fine to remove it, but we probably want more asserts like this at
> least for builds with debugging enabled.  What do people think about
> adding an ASSERT macro that is compiled away for non-debug builds?
> 


Yes, assertion is useful, but would be carefully added into where is
necessary. For this very case, it is obviously unnecessary.

I don't think assert() will be compiled away for default sheepdog
non-argument 'make' command.

> I'd also like to port over the Linux kernel linked list debugging
> (CONFIG_DEBUG_LIST) to the sheepdog copy of list.h for debug builds,
> as that would catch errors like that, too.
> 


I am fine with the idea.

Thanks,
Yuan