Re: [Qemu-devel] [PATCH v2 09/22] nbd/client: Refactor nbd_receive_list()
15.12.2018 16:53, Eric Blake wrote: > Right now, nbd_receive_list() is only called by > nbd_receive_query_exports(), which in turn is only called if the > server lacks NBD_OPT_GO but has working option negotiation, and is > merely used as a quality-of-implementation trick since servers > can't give decent errors for NBD_OPT_EXPORT_NAME. However, servers > that lack NBD_OPT_GO are becoming increasingly rare (nbdkit was a > latecomer, in Aug 2018, but qemu has been such a server since commit > f37708f6 in July 2017 and released in 2.10), so it no longer makes > sense to micro-optimize that function for performance. > > Furthermore, when debugging a server's implementation, tracing the > full reply (both names and descriptions) is useful, not to mention > that upcoming patches adding 'qemu-nbd --list' will want to collect > that data. And when you consider that a server can send an export > name up to the NBD protocol length limit of 4k; but our current > NBD_MAX_NAME_SIZE is only 256, we can't trace all valid server > names without more storage, but 4k is large enough that the heap > is better than the stack for long names. > > Thus, I'm changing the division of labor, with nbd_receive_list() > now always malloc'ing a result on success (the malloc is bounded > by the fact that we reject servers with a reply length larger > than 32M), and moving the comparison to 'wantname' to the caller. > > There is a minor change in behavior where a server with 0 exports > (an immediate NBD_REP_ACK reply) is now no longer distinguished > from a server without LIST support (NBD_REP_ERR_UNSUP); this > information could be preserved with a complication to the calling > contract to provide a bit more information, but I didn't see the > point. After all, the worst that can happen if our guess at a > match is wrong is that the caller will get a cryptic disconnect > when NBD_OPT_EXPORT_NAME fails (which is no different from what > would happen if we had not tried LIST), while treating an empty > list as immediate failure would prevent connecting to really old > servers that really did lack LIST. Besides, NBD servers with 0 > exports are rare (qemu can do it when using QMP nbd-server-start > without nbd-server-add - but qemu understands NBD_OPT_GO and > thus won't tickle this change in behavior). > > Signed-off-by: Eric Blake > > --- > v2: Rewrite in a different manner (move the comparison to the > caller) > Fix free to g_free [Vladimir] > --- > nbd/client.c | 89 +++- > nbd/trace-events | 1 + > 2 files changed, 58 insertions(+), 32 deletions(-) > > diff --git a/nbd/client.c b/nbd/client.c > index 1a3a620fb6d..28f5a286cba 100644 > --- a/nbd/client.c > +++ b/nbd/client.c > @@ -232,18 +232,24 @@ static int nbd_handle_reply_err(QIOChannel *ioc, > NBDOptionReply *reply, > return result; > } > > -/* Process another portion of the NBD_OPT_LIST reply. Set *@match if > - * the current reply matches @want or if the server does not support > - * NBD_OPT_LIST, otherwise leave @match alone. Return 0 if iteration > - * is complete, positive if more replies are expected, or negative > - * with @errp set if an unrecoverable error occurred. */ > -static int nbd_receive_list(QIOChannel *ioc, const char *want, bool *match, > +/* nbd_receive_list: > + * Process another portion of the NBD_OPT_LIST reply, populating any > + * name received into *@name. If @description is non-NULL, and the > + * server provided a description, that is also populated. The caller > + * must eventually call g_free() on success. > + * Returns 1 if a name was received and iteration must continue, > + * 0 if iteration is complete, maybe, something like: if iteration is complete, or NBD_OPT_LIST is unsupported. @name and @description are not set. (it's not obvious thing, that iteration complete assumes no new name received, and that 0 serves unsupported too) > + * -1 with @errp set if an unrecoverable error occurred. > + */ > +static int nbd_receive_list(QIOChannel *ioc, char **name, char **description, > Error **errp) > { > +int ret = -1; > NBDOptionReply reply; > uint32_t len; > uint32_t namelen; > -char name[NBD_MAX_NAME_SIZE + 1]; > +char *local_name = NULL; > +char *local_desc = NULL; > int error; > > if (nbd_receive_option_reply(ioc, NBD_OPT_LIST, , errp) < 0) { > @@ -251,9 +257,6 @@ static int nbd_receive_list(QIOChannel *ioc, const char > *want, bool *match, > } > error = nbd_handle_reply_err(ioc, , errp); > if (error <= 0) { > -/* The server did not support NBD_OPT_LIST, so set *match on > - * the assumption that any name will be accepted. */ > -*match = true; hm, actually, match was wrongly set to true on negative error, but it doesn't matter. > return error; > } > len = reply.length; > @@ -290,33 +293,38 @@ static int
Re: [Qemu-devel] [PATCH v2 09/22] nbd/client: Refactor nbd_receive_list()
On Sat, Dec 15, 2018 at 07:53:11AM -0600, Eric Blake wrote: > Right now, nbd_receive_list() is only called by > nbd_receive_query_exports(), which in turn is only called if the > server lacks NBD_OPT_GO but has working option negotiation, and is > merely used as a quality-of-implementation trick since servers > can't give decent errors for NBD_OPT_EXPORT_NAME. However, servers > that lack NBD_OPT_GO are becoming increasingly rare (nbdkit was a > latecomer, in Aug 2018, but qemu has been such a server since commit > f37708f6 in July 2017 and released in 2.10), so it no longer makes > sense to micro-optimize that function for performance. > > Furthermore, when debugging a server's implementation, tracing the > full reply (both names and descriptions) is useful, not to mention > that upcoming patches adding 'qemu-nbd --list' will want to collect > that data. And when you consider that a server can send an export > name up to the NBD protocol length limit of 4k; but our current > NBD_MAX_NAME_SIZE is only 256, we can't trace all valid server > names without more storage, but 4k is large enough that the heap > is better than the stack for long names. > > Thus, I'm changing the division of labor, with nbd_receive_list() > now always malloc'ing a result on success (the malloc is bounded > by the fact that we reject servers with a reply length larger > than 32M), and moving the comparison to 'wantname' to the caller. > > There is a minor change in behavior where a server with 0 exports > (an immediate NBD_REP_ACK reply) is now no longer distinguished > from a server without LIST support (NBD_REP_ERR_UNSUP); this > information could be preserved with a complication to the calling > contract to provide a bit more information, but I didn't see the > point. After all, the worst that can happen if our guess at a > match is wrong is that the caller will get a cryptic disconnect > when NBD_OPT_EXPORT_NAME fails (which is no different from what > would happen if we had not tried LIST), while treating an empty > list as immediate failure would prevent connecting to really old > servers that really did lack LIST. Besides, NBD servers with 0 > exports are rare (qemu can do it when using QMP nbd-server-start > without nbd-server-add - but qemu understands NBD_OPT_GO and > thus won't tickle this change in behavior). > > Signed-off-by: Eric Blake > > --- > v2: Rewrite in a different manner (move the comparison to the > caller) > Fix free to g_free [Vladimir] > --- > nbd/client.c | 89 +++- > nbd/trace-events | 1 + > 2 files changed, 58 insertions(+), 32 deletions(-) > > diff --git a/nbd/client.c b/nbd/client.c > index 1a3a620fb6d..28f5a286cba 100644 > --- a/nbd/client.c > +++ b/nbd/client.c > @@ -232,18 +232,24 @@ static int nbd_handle_reply_err(QIOChannel *ioc, > NBDOptionReply *reply, > return result; > } > > -/* Process another portion of the NBD_OPT_LIST reply. Set *@match if > - * the current reply matches @want or if the server does not support > - * NBD_OPT_LIST, otherwise leave @match alone. Return 0 if iteration > - * is complete, positive if more replies are expected, or negative > - * with @errp set if an unrecoverable error occurred. */ > -static int nbd_receive_list(QIOChannel *ioc, const char *want, bool *match, > +/* nbd_receive_list: > + * Process another portion of the NBD_OPT_LIST reply, populating any > + * name received into *@name. If @description is non-NULL, and the > + * server provided a description, that is also populated. The caller > + * must eventually call g_free() on success. > + * Returns 1 if a name was received and iteration must continue, > + * 0 if iteration is complete, > + * -1 with @errp set if an unrecoverable error occurred. > + */ > +static int nbd_receive_list(QIOChannel *ioc, char **name, char **description, > Error **errp) > { > +int ret = -1; > NBDOptionReply reply; > uint32_t len; > uint32_t namelen; > -char name[NBD_MAX_NAME_SIZE + 1]; > +char *local_name = NULL; > +char *local_desc = NULL; > int error; > > if (nbd_receive_option_reply(ioc, NBD_OPT_LIST, , errp) < 0) { > @@ -251,9 +257,6 @@ static int nbd_receive_list(QIOChannel *ioc, const char > *want, bool *match, > } > error = nbd_handle_reply_err(ioc, , errp); > if (error <= 0) { > -/* The server did not support NBD_OPT_LIST, so set *match on > - * the assumption that any name will be accepted. */ > -*match = true; > return error; > } > len = reply.length; > @@ -290,33 +293,38 @@ static int nbd_receive_list(QIOChannel *ioc, const char > *want, bool *match, > nbd_send_opt_abort(ioc); > return -1; > } > -if (namelen != strlen(want)) { > -if (nbd_drop(ioc, len, errp) < 0) { > -error_prepend(errp, > - "failed to skip export name with wrong
[Qemu-devel] [PATCH v2 09/22] nbd/client: Refactor nbd_receive_list()
Right now, nbd_receive_list() is only called by nbd_receive_query_exports(), which in turn is only called if the server lacks NBD_OPT_GO but has working option negotiation, and is merely used as a quality-of-implementation trick since servers can't give decent errors for NBD_OPT_EXPORT_NAME. However, servers that lack NBD_OPT_GO are becoming increasingly rare (nbdkit was a latecomer, in Aug 2018, but qemu has been such a server since commit f37708f6 in July 2017 and released in 2.10), so it no longer makes sense to micro-optimize that function for performance. Furthermore, when debugging a server's implementation, tracing the full reply (both names and descriptions) is useful, not to mention that upcoming patches adding 'qemu-nbd --list' will want to collect that data. And when you consider that a server can send an export name up to the NBD protocol length limit of 4k; but our current NBD_MAX_NAME_SIZE is only 256, we can't trace all valid server names without more storage, but 4k is large enough that the heap is better than the stack for long names. Thus, I'm changing the division of labor, with nbd_receive_list() now always malloc'ing a result on success (the malloc is bounded by the fact that we reject servers with a reply length larger than 32M), and moving the comparison to 'wantname' to the caller. There is a minor change in behavior where a server with 0 exports (an immediate NBD_REP_ACK reply) is now no longer distinguished from a server without LIST support (NBD_REP_ERR_UNSUP); this information could be preserved with a complication to the calling contract to provide a bit more information, but I didn't see the point. After all, the worst that can happen if our guess at a match is wrong is that the caller will get a cryptic disconnect when NBD_OPT_EXPORT_NAME fails (which is no different from what would happen if we had not tried LIST), while treating an empty list as immediate failure would prevent connecting to really old servers that really did lack LIST. Besides, NBD servers with 0 exports are rare (qemu can do it when using QMP nbd-server-start without nbd-server-add - but qemu understands NBD_OPT_GO and thus won't tickle this change in behavior). Signed-off-by: Eric Blake --- v2: Rewrite in a different manner (move the comparison to the caller) Fix free to g_free [Vladimir] --- nbd/client.c | 89 +++- nbd/trace-events | 1 + 2 files changed, 58 insertions(+), 32 deletions(-) diff --git a/nbd/client.c b/nbd/client.c index 1a3a620fb6d..28f5a286cba 100644 --- a/nbd/client.c +++ b/nbd/client.c @@ -232,18 +232,24 @@ static int nbd_handle_reply_err(QIOChannel *ioc, NBDOptionReply *reply, return result; } -/* Process another portion of the NBD_OPT_LIST reply. Set *@match if - * the current reply matches @want or if the server does not support - * NBD_OPT_LIST, otherwise leave @match alone. Return 0 if iteration - * is complete, positive if more replies are expected, or negative - * with @errp set if an unrecoverable error occurred. */ -static int nbd_receive_list(QIOChannel *ioc, const char *want, bool *match, +/* nbd_receive_list: + * Process another portion of the NBD_OPT_LIST reply, populating any + * name received into *@name. If @description is non-NULL, and the + * server provided a description, that is also populated. The caller + * must eventually call g_free() on success. + * Returns 1 if a name was received and iteration must continue, + * 0 if iteration is complete, + * -1 with @errp set if an unrecoverable error occurred. + */ +static int nbd_receive_list(QIOChannel *ioc, char **name, char **description, Error **errp) { +int ret = -1; NBDOptionReply reply; uint32_t len; uint32_t namelen; -char name[NBD_MAX_NAME_SIZE + 1]; +char *local_name = NULL; +char *local_desc = NULL; int error; if (nbd_receive_option_reply(ioc, NBD_OPT_LIST, , errp) < 0) { @@ -251,9 +257,6 @@ static int nbd_receive_list(QIOChannel *ioc, const char *want, bool *match, } error = nbd_handle_reply_err(ioc, , errp); if (error <= 0) { -/* The server did not support NBD_OPT_LIST, so set *match on - * the assumption that any name will be accepted. */ -*match = true; return error; } len = reply.length; @@ -290,33 +293,38 @@ static int nbd_receive_list(QIOChannel *ioc, const char *want, bool *match, nbd_send_opt_abort(ioc); return -1; } -if (namelen != strlen(want)) { -if (nbd_drop(ioc, len, errp) < 0) { -error_prepend(errp, - "failed to skip export name with wrong length: "); -nbd_send_opt_abort(ioc); -return -1; -} -return 1; -} -assert(namelen < sizeof(name)); -if (nbd_read(ioc, name, namelen, errp) < 0) { +local_name = g_malloc(namelen + 1); +if (nbd_read(ioc, local_name,