12.01.2019 20:58, Eric Blake wrote:
> Extract portions of nbd_negotiate_simple_meta_context() to
> a new function nbd_receive_one_meta_context() that copies the
> pattern of nbd_receive_list() for performing the argument
> validation of one reply. The error message when the server
> replies with more than one context changes slightly, but
> that shouldn't happen in the common case.
>
> Signed-off-by: Eric Blake <ebl...@redhat.com>
> Message-Id: <20181215135324.152629-15-ebl...@redhat.com>
>
> ---
> v3: rebase, without changing into a loop
> ---
> nbd/client.c | 148 +++++++++++++++++++++++++++++------------------
> nbd/trace-events | 2 +-
> 2 files changed, 92 insertions(+), 58 deletions(-)
>
> diff --git a/nbd/client.c b/nbd/client.c
> index 3c716be2719..22505199d3b 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -672,7 +672,86 @@ static int nbd_send_one_meta_context(QIOChannel *ioc,
> return ret;
> }
>
> -/* nbd_negotiate_simple_meta_context:
> +/*
> + * nbd_receive_one_meta_context:
> + * Called in a loop to receive and trace one set/list meta context reply.
> + * Pass non-NULL @name or @id to collect results back to the caller, which
> + * must eventually call g_free().
> + * return 1 if name is set and iteration must continue,
> + * 0 if iteration is complete (including if option is unsupported),
> + * -1 with errp set for any error
> + */
> +static int nbd_receive_one_meta_context(QIOChannel *ioc,
> + uint32_t opt,
> + char **name,
> + uint32_t *id,
> + Error **errp)
> +{
> + int ret;
> + NBDOptionReply reply;
> + char *local_name = NULL;
> + uint32_t local_id;
> +
> + if (nbd_receive_option_reply(ioc, opt, &reply, errp) < 0) {
> + return -1;
> + }
> +
> + ret = nbd_handle_reply_err(ioc, &reply, errp);
> + if (ret <= 0) {
> + return ret;
> + }
> +
> + if (reply.type == NBD_REP_ACK) {
> + if (reply.length != 0) {
> + error_setg(errp, "Unexpected length to ACK response");
> + nbd_send_opt_abort(ioc);
> + return -1;
> + }
> + return 0;
> + } else if (reply.type != NBD_REP_META_CONTEXT) {
> + error_setg(errp, "Unexpected reply type %u (%s), expected %u (%s)",
> + reply.type, nbd_rep_lookup(reply.type),
> + NBD_REP_META_CONTEXT,
> nbd_rep_lookup(NBD_REP_META_CONTEXT));
> + nbd_send_opt_abort(ioc);
> + return -1;
> + }
> +
> + if (reply.length <= sizeof(local_id) ||
> + reply.length > NBD_MAX_BUFFER_SIZE) {
> + error_setg(errp, "Failed to negotiate meta context, server "
> + "answered with unexpected length %" PRIu32,
> + reply.length);
> + nbd_send_opt_abort(ioc);
> + return -1;
> + }
> +
> + if (nbd_read(ioc, &local_id, sizeof(local_id), errp) < 0) {
> + return -1;
> + }
> + local_id = be32_to_cpu(local_id);
> +
> + reply.length -= sizeof(local_id);
> + local_name = g_malloc(reply.length + 1);
> + if (nbd_read(ioc, local_name, reply.length, errp) < 0) {
> + g_free(local_name);
> + return -1;
> + }
> + local_name[reply.length] = '\0';
> + trace_nbd_opt_meta_reply(nbd_opt_lookup(opt), local_name, local_id);
> +
> + if (name) {
> + *name = local_name;
> + } else {
> + g_free(local_name);
> + }
> + if (id) {
> + *id = local_id;
> + }
> + return 1;
> +}
> +
> +/*
> + * nbd_negotiate_simple_meta_context:
> * Request the server to set the meta context for export @info->name
> * using @info->x_dirty_bitmap with a fallback to "base:allocation",
> * setting @info->context_id to the resulting id. Fail if the server
> @@ -693,50 +772,21 @@ static int nbd_negotiate_simple_meta_context(QIOChannel
> *ioc,
> * function should lose the term _simple.
> */
> int ret;
> - NBDOptionReply reply;
> const char *context = info->x_dirty_bitmap ?: "base:allocation";
> bool received = false;
> + char *name = NULL;
>
> if (nbd_send_one_meta_context(ioc, NBD_OPT_SET_META_CONTEXT,
> info->name, context, errp) < 0) {
> return -1;
> }
>
> - if (nbd_receive_option_reply(ioc, NBD_OPT_SET_META_CONTEXT, &reply,
> - errp) < 0)
> - {
> + ret = nbd_receive_one_meta_context(ioc, NBD_OPT_SET_META_CONTEXT,
> + &name, &info->context_id, errp);
> + if (ret < 0) {
> return -1;
> }
> -
> - ret = nbd_handle_reply_err(ioc, &reply, errp);
> - if (ret <= 0) {
> - return ret;
> - }
> -
> - if (reply.type == NBD_REP_META_CONTEXT) {
> - char *name;
> -
> - if (reply.length != sizeof(info->context_id) + strlen(context)) {
> - error_setg(errp, "Failed to negotiate meta context '%s', server "
> - "answered with unexpected length %" PRIu32, context,
> - reply.length);
> - nbd_send_opt_abort(ioc);
> - return -1;
> - }
> -
> - if (nbd_read(ioc, &info->context_id, sizeof(info->context_id),
> - errp) < 0) {
> - return -1;
> - }
> - info->context_id = be32_to_cpu(info->context_id);
> -
> - reply.length -= sizeof(info->context_id);
> - name = g_malloc(reply.length + 1);
> - if (nbd_read(ioc, name, reply.length, errp) < 0) {
> - g_free(name);
> - return -1;
> - }
> - name[reply.length] = '\0';
> + if (ret == 1) {
> if (strcmp(context, name)) {
> error_setg(errp, "Failed to negotiate meta context '%s', server
> "
> "answered with different context '%s'", context,
> @@ -746,36 +796,20 @@ static int nbd_negotiate_simple_meta_context(QIOChannel
> *ioc,
> return -1;
> }
> g_free(name);
> -
> - trace_nbd_opt_meta_reply(context, info->context_id);
> received = true;
>
> - /* receive NBD_REP_ACK */
> - if (nbd_receive_option_reply(ioc, NBD_OPT_SET_META_CONTEXT, &reply,
> - errp) < 0)
> - {
> + ret = nbd_receive_one_meta_context(ioc, NBD_OPT_SET_META_CONTEXT,
> + &name, &info->context_id, errp);
indent, and, no reasons to use variables instead of NULL, NULL, as success here
is an error
path anyway
> + if (ret < 0) {
> return -1;
> }
> -
> - ret = nbd_handle_reply_err(ioc, &reply, errp);
> - if (ret <= 0) {
> - return ret;
> - }
> }
> -
> - if (reply.type != NBD_REP_ACK) {
> - error_setg(errp, "Unexpected reply type %u (%s), expected %u (%s)",
> - reply.type, nbd_rep_lookup(reply.type),
> - NBD_REP_ACK, nbd_rep_lookup(NBD_REP_ACK));
> + if (ret != 0) {
> + error_setg(errp, "Server answered with more than one context");
> + g_free(name);
and then, this g_free may be dropped too.
> nbd_send_opt_abort(ioc);
> return -1;
> }
> - if (reply.length) {
> - error_setg(errp, "Unexpected length to ACK response");
> - nbd_send_opt_abort(ioc);
> - return -1;
> - }
> -
> return received;
> }
>
> diff --git a/nbd/trace-events b/nbd/trace-events
> index 59521e47a3d..b4802c1570e 100644
> --- a/nbd/trace-events
> +++ b/nbd/trace-events
> @@ -13,7 +13,7 @@ nbd_receive_query_exports_success(const char *wantname)
> "Found desired export na
> nbd_receive_starttls_new_client(void) "Setting up TLS"
> nbd_receive_starttls_tls_handshake(void) "Starting TLS handshake"
> nbd_opt_meta_request(const char *optname, const char *context, const char
> *export) "Requesting %s %s for export %s"
> -nbd_opt_meta_reply(const char *context, uint32_t id) "Received mapping of
> context %s to id %" PRIu32
> +nbd_opt_meta_reply(const char *optname, const char *context, uint32_t id)
> "Received %s mapping of %s to id %" PRIu32
nbd_opt_lookup
> nbd_receive_negotiate(void *tlscreds, const char *hostname) "Receiving
> negotiation tlscreds=%p hostname=%s"
> nbd_receive_negotiate_magic(uint64_t magic) "Magic is 0x%" PRIx64
> nbd_receive_negotiate_server_flags(uint32_t globalflags) "Global flags are
> 0x%" PRIx32
>
With at least nbd_opt_lookup:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
--
Best regards,
Vladimir
