From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40007) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bPjIQ-0002ND-Ol for qemu-devel@nongnu.org; Wed, 20 Jul 2016 00:39:47 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bPjIP-0000My-O5 for qemu-devel@nongnu.org; Wed, 20 Jul 2016 00:39:46 -0400 Date: Wed, 20 Jul 2016 12:39:38 +0800 From: Fam Zheng Message-ID: <20160720043938.GB7641@ad.usersys.redhat.com> References: <1468901281-22858-1-git-send-email-eblake@redhat.com> <1468901281-22858-6-git-send-email-eblake@redhat.com> <20160719051055.GD18103@ad.usersys.redhat.com> <578E3EBF.1040707@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <578E3EBF.1040707@redhat.com> Subject: Re: [Qemu-devel] [PATCH v5 05/14] nbd: Share common reply-sending code in server List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: pbonzini@redhat.com, qemu-devel@nongnu.org, qemu-block@nongnu.org On Tue, 07/19 08:52, Eric Blake wrote: > On 07/18/2016 11:10 PM, Fam Zheng wrote: > > On Mon, 07/18 22:07, Eric Blake wrote: > >> Rather than open-coding NBD_REP_SERVER, reuse the code we > >> already have by adding a length parameter. Additionally, > >> the refactoring will make adding NBD_OPT_GO in a later patch > >> easier. > >> > >> Signed-off-by: Eric Blake > >> > >> --- > >> v4: no change > >> v3: rebase to changes earlier in series > >> --- > >> nbd/server.c | 48 +++++++++++++++++++++++------------------------- > >> 1 file changed, 23 insertions(+), 25 deletions(-) > >> > >> diff --git a/nbd/server.c b/nbd/server.c > >> index 85c4f5d..c8716f1 100644 > >> --- a/nbd/server.c > >> +++ b/nbd/server.c > >> @@ -195,12 +195,15 @@ static ssize_t nbd_negotiate_drop_sync(QIOChannel *ioc, size_t size) > >> > >> */ > >> > >> -static int nbd_negotiate_send_rep(QIOChannel *ioc, uint32_t type, uint32_t opt) > >> +/* Send a reply header, including length, but no payload. > >> + * Return -errno to kill connection, 0 to continue negotiation. */ > > > > Not a show stopper but I'm not sure documenting the control logic of the > > outermost caller a few layers away is a good idea, the same question applies to > > functions below as well. > > The documentation here accurately describes this function. Or is your > complaint that the outermost caller is lacking documentation, and > therefore I should first do a patch that uniformly adds documentation, > before changing behavior, so that this function doesn't end up with > details while the outermost caller remains undocumented? No, I didn't like it because "kill connection" logic is actually in the caller and not a functionality of this function. I'd say "Return -errno if an error occured, otherwise return 0". Fam