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 <ebl...@redhat.com>
>
> ---
> 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, &reply, errp) < 0) {
> @@ -251,9 +257,6 @@ static int nbd_receive_list(QIOChannel *ioc, const char
> *want, bool *match,
> }
> error = nbd_handle_reply_err(ioc, &reply, 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 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, namelen, errp) < 0) {
> error_prepend(errp, "failed to read export name: ");
> nbd_send_opt_abort(ioc);
> - return -1;
> + goto out;
> }
> - name[namelen] = '\0';
> + local_name[namelen] = '\0';
> len -= namelen;
> - if (nbd_drop(ioc, len, errp) < 0) {
> - error_prepend(errp, "failed to read export description: ");
> - nbd_send_opt_abort(ioc);
> - return -1;
> + if (len) {
> + local_desc = g_malloc(len + 1);
> + if (nbd_read(ioc, local_desc, len, errp) < 0) {
> + error_prepend(errp, "failed to read export description: ");
> + nbd_send_opt_abort(ioc);
> + goto out;
> + }
> + local_desc[len] = '\0';
> }
> - if (!strcmp(name, want)) {
> - *match = true;
> +
> + trace_nbd_receive_list(local_name, local_desc ?: "");
> + *name = local_name;
> + local_name = NULL;
> + if (description) {
> + *description = local_desc;
> + local_desc = NULL;
> }
> - return 1;
> + ret = 1;
> +
> + out:
> + g_free(local_name);
> + g_free(local_desc);
> + return ret;
> }
>
>
> @@ -491,6 +499,7 @@ static int nbd_receive_query_exports(QIOChannel *ioc,
> const char *wantname,
> Error **errp)
> {
> + bool listEmpty = true;
> bool foundExport = false;
being here, better is to fix naming to Qemu coding style, list_empty and
found_export
>
> trace_nbd_receive_query_exports_start(wantname);
> @@ -499,14 +508,25 @@ static int nbd_receive_query_exports(QIOChannel *ioc,
> }
>
> while (1) {
> - int ret = nbd_receive_list(ioc, wantname, &foundExport, errp);
> + char *name;
> + int ret = nbd_receive_list(ioc, &name, NULL, errp);
>
> if (ret < 0) {
> /* Server gave unexpected reply */
> return -1;
> } else if (ret == 0) {
> /* Done iterating. */
> - if (!foundExport) {
> + if (listEmpty) {
> + /*
> + * We don't have enough context to tell a server that
> + * sent an empty list apart from a server that does
> + * not support the list command; but as this function
> + * is just used to trigger a nicer error message
> + * before trying NBD_OPT_EXPORT_NAME, assume the
> + * export is available.
> + */
> + return 0;
> + } else if (!foundExport) {
> error_setg(errp, "No export with name '%s' available",
> wantname);
> nbd_send_opt_abort(ioc);
> @@ -515,6 +535,11 @@ static int nbd_receive_query_exports(QIOChannel *ioc,
> trace_nbd_receive_query_exports_success(wantname);
> return 0;
> }
> + listEmpty = false;
> + if (!strcmp(name, wantname)) {
> + foundExport = true;
> + }
> + g_free(name);
> }
> }
>
> diff --git a/nbd/trace-events b/nbd/trace-events
> index 5e1d4afe8e6..8e9fc024c28 100644
> --- a/nbd/trace-events
> +++ b/nbd/trace-events
> @@ -2,6 +2,7 @@
> nbd_send_option_request(uint32_t opt, const char *name, uint32_t len)
> "Sending option request %" PRIu32" (%s), len %" PRIu32
> nbd_receive_option_reply(uint32_t option, const char *optname, uint32_t
> type, const char *typename, uint32_t length) "Received option reply %"
> PRIu32" (%s), type %" PRIu32" (%s), len %" PRIu32
> nbd_reply_err_unsup(uint32_t option, const char *name) "server doesn't
> understand request %" PRIu32 " (%s), attempting fallback"
> +nbd_receive_list(const char *name, const char *desc) "export list includes
> '%s', description '%s'"
> nbd_opt_go_start(const char *name) "Attempting NBD_OPT_GO for export '%s'"
> nbd_opt_go_success(void) "Export is good to go"
> nbd_opt_go_info_unknown(int info, const char *name) "Ignoring unknown info
> %d (%s)"
>
New semantics of nbd_receive_list is much more simple, thank you! With or
without my tiny suggestions:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
--
Best regards,
Vladimir
