From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47929) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bPNJD-0004kY-Sl for qemu-devel@nongnu.org; Tue, 19 Jul 2016 01:11:09 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bPNJB-0007bf-In for qemu-devel@nongnu.org; Tue, 19 Jul 2016 01:11:06 -0400 Date: Tue, 19 Jul 2016 13:10:55 +0800 From: Fam Zheng Message-ID: <20160719051055.GD18103@ad.usersys.redhat.com> References: <1468901281-22858-1-git-send-email-eblake@redhat.com> <1468901281-22858-6-git-send-email-eblake@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1468901281-22858-6-git-send-email-eblake@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: qemu-devel@nongnu.org, pbonzini@redhat.com, qemu-block@nongnu.org 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. > +static int nbd_negotiate_send_rep_len(QIOChannel *ioc, uint32_t type, > + uint32_t opt, uint32_t len) > { > uint64_t magic; > - uint32_t len; > > - TRACE("Reply opt=%" PRIx32 " type=%" PRIx32, type, opt); > + TRACE("Reply opt=%" PRIx32 " type=%" PRIx32 " len=%" PRIu32, > + type, opt, len); > > magic = cpu_to_be64(NBD_REP_MAGIC); > if (nbd_negotiate_write(ioc, &magic, sizeof(magic)) != sizeof(magic)) { > @@ -217,7 +220,7 @@ static int nbd_negotiate_send_rep(QIOChannel *ioc, uint32_t type, uint32_t opt) > LOG("write failed (rep type)"); > return -EINVAL; > } > - len = cpu_to_be32(0); > + len = cpu_to_be32(len); > if (nbd_negotiate_write(ioc, &len, sizeof(len)) != sizeof(len)) { > LOG("write failed (rep data length)"); > return -EINVAL; > @@ -225,37 +228,32 @@ static int nbd_negotiate_send_rep(QIOChannel *ioc, uint32_t type, uint32_t opt) > return 0; > } > > +/* Send a reply header with default 0 length. > + * Return -errno to kill connection, 0 to continue negotiation. */ > +static int nbd_negotiate_send_rep(QIOChannel *ioc, uint32_t type, uint32_t opt) > +{ > + return nbd_negotiate_send_rep_len(ioc, type, opt, 0); > +} > + > +/* Send an NBD_REP_SERVER reply to NBD_OPT_LIST, including payload. > + * Return -errno to kill connection, 0 to continue negotiation. */ > static int nbd_negotiate_send_rep_list(QIOChannel *ioc, NBDExport *exp) > { > - uint64_t magic; > size_t name_len, desc_len; > - uint32_t opt, type, len; > + uint32_t len; > const char *name = exp->name ? exp->name : ""; > const char *desc = exp->description ? exp->description : ""; > + int rc; > > TRACE("Advertising export name '%s' description '%s'", name, desc); > name_len = strlen(name); > desc_len = strlen(desc); > - magic = cpu_to_be64(NBD_REP_MAGIC); > - if (nbd_negotiate_write(ioc, &magic, sizeof(magic)) != sizeof(magic)) { > - LOG("write failed (magic)"); > - return -EINVAL; > - } > - opt = cpu_to_be32(NBD_OPT_LIST); > - if (nbd_negotiate_write(ioc, &opt, sizeof(opt)) != sizeof(opt)) { > - LOG("write failed (opt)"); > - return -EINVAL; > - } > - type = cpu_to_be32(NBD_REP_SERVER); > - if (nbd_negotiate_write(ioc, &type, sizeof(type)) != sizeof(type)) { > - LOG("write failed (reply type)"); > - return -EINVAL; > - } > - len = cpu_to_be32(name_len + desc_len + sizeof(len)); > - if (nbd_negotiate_write(ioc, &len, sizeof(len)) != sizeof(len)) { > - LOG("write failed (length)"); > - return -EINVAL; > + len = name_len + desc_len + sizeof(len); > + rc = nbd_negotiate_send_rep_len(ioc, NBD_REP_SERVER, NBD_OPT_LIST, len); > + if (rc < 0) { > + return rc; > } > + > len = cpu_to_be32(name_len); > if (nbd_negotiate_write(ioc, &len, sizeof(len)) != sizeof(len)) { > LOG("write failed (name length)"); > -- > 2.5.5 > >