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 <ebl...@redhat.com> > >> > >> --- > >> 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
