[Cluster-devel] Re: [PATCH 1/3] NLM add resume procfs file

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

On Wed, Jan 30, 2008 at 11:43:50AM -0500, Wendy Cheng wrote:
> J. Bruce Fields wrote:
>>> +	int rc;
>>> +
>>> +	rc = failover_parse_ip(file, buf, size, &server_ip);
>>> +	if (rc < 0)
>>> +		return rc;
>>>   	return nlmsvc_failover_ip(server_ip);
>>>  }
>>>     
>>
>> Looks great, but it would fit more logically with the previous patch.
>> (If you know you're going to end up using this code in two places, may
>> as well write it that way from the start.)
>>   
>
> The original unlock patch did have a shared routine for this purpose.  
> After review, its code structure got changed a little bit. Since the  
> revised version has non-trivial amount of testing efforts behind it, I  
> think it is better to do the change here, instead of the well-tested  
> unlock patch.
>
> On the other hand, I cut the resume patch into three pieces mostly for  
> review purpose. Do you think it would be easier (for your git tree  
> works) that I combine these three small patches into a big resume patch  
> after all the review comments are incorporated into the code ?

As long as they each compile and run without introducing any new bugs
(even temporarily) along the way, then I'll almost always prefer more
smaller patches to fewer larger ones.

--b.