From mboxrd@z Thu Jan 1 00:00:00 1970 From: Wendy Cheng Date: Wed, 30 Jan 2008 11:43:50 -0500 Subject: [Cluster-devel] Re: [PATCH 1/3] NLM add resume procfs file In-Reply-To: <20080129021910.GF16785@fieldses.org> References: <47997059.704@redhat.com> <20080129021910.GF16785@fieldses.org> Message-ID: <47A0A946.5030405@redhat.com> List-Id: To: cluster-devel.redhat.com MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit 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 ? -- Wendy