qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-block] [Qemu-devel] [PATCH v5 05/14] nbd: Share common reply-s


From: Fam Zheng
Subject: Re: [Qemu-block] [Qemu-devel] [PATCH v5 05/14] nbd: Share common reply-sending code in server
Date: Wed, 20 Jul 2016 12:39:38 +0800
User-agent: Mutt/1.6.1 (2016-04-27)

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 <address@hidden>
> >>
> >> ---
> >> 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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]