Re: [Qemu-block] [PATCH v3 11/19] nbd/client: Split out nbd_receive_one_meta_context()

[Vladimir Sementsov-Ogievskiy]

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 <address@hidden>
> Message-Id: <address@hidden>
> 
> ---
> 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 <address@hidden>

-- 
Best regards,
Vladimir