| Message ID | 1468901281-22858-6-git-send-email-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <eblake@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. > +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 > >
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 <eblake@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?
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 <eblake@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
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. */ +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)");
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 <eblake@redhat.com> --- v4: no change v3: rebase to changes earlier in series --- nbd/server.c | 48 +++++++++++++++++++++++------------------------- 1 file changed, 23 insertions(+), 25 deletions(-)