Re: [Qemu-devel] [PATCH 07/14] nbd/client: Refactor nbd_negotiate_simple_meta_context()

[Vladimir Sementsov-Ogievskiy]

01.12.2018 1:03, Eric Blake wrote:
> Change the signature to make it easier for a future patch to
> reuse this function for calling NBD_OPT_LIST_META_CONTEXT with
> 0 or 1 queries.  Also, always allocate space for the received
> name, even if it doesn't match expected lengths (no point
> trying to optimize the unlikely error case, and tracing the
> received rather than expected name can help debug a server
> implementation).
> 
> While there are now slightly different traces, and the error
> message for a server replying with too many contexts is
> different, there are no runtime-observable changes in behavior
> for the more common case of the lone caller interacting with a
> compliant server.
> 
> Signed-off-by: Eric Blake <address@hidden>
> ---
>   nbd/client.c     | 105 +++++++++++++++++++++++++++--------------------
>   nbd/trace-events |   2 +-
>   2 files changed, 61 insertions(+), 46 deletions(-)
> 
> diff --git a/nbd/client.c b/nbd/client.c
> index b5818a99d21..1dc8f83e19a 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -603,49 +603,57 @@ static QIOChannel *nbd_receive_starttls(QIOChannel *ioc,
>   }
> 
>   /* nbd_negotiate_simple_meta_context:
> - * Set one meta context. Simple means that reply must contain zero (not
> - * negotiated) or one (negotiated) contexts. More contexts would be 
> considered
> - * as a protocol error. It's also implied that meta-data query equals queried
> - * context name, so, if server replies with something different than 
> @context,
> - * it is considered an error too.
> - * return 1 for successful negotiation, context_id is set
> - *        0 if operation is unsupported,
> + * List or set meta context data for export @info->name, based on @opt.

hm, just list or set meta context? What is "data" about?

> + * For list, leave @context NULL for 0 queries, or supplied for a single
> + * query; all replies are ignored, and the call merely traces server 
> behavior.
> + * For set, @context must result in at most one matching server reply, in
> + * which case @info->meta_base_allocation_id is set to the resulting id.

Hmm, looks too cheating. Then it should be renamed to
nbd_negotiate_base_allocation

and parameter @context should be renamed to @x_dirty_bitmap,

and if it is unset, we'll use "base:allocation" here.

but in this case, it still weird about opt=list case.. So, it should be named
like nbd_negotiation_helper, as it is doing several different things, which
I can't describe in one word.

> + * return 1 for successful negotiation,
> + *        0 if operation is unsupported or context unavailable,
>    *        -1 with errp set for any other error

this return value description looks not very related to opt=list case

>    */
>   static int nbd_negotiate_simple_meta_context(QIOChannel *ioc,
> -                                             const char *export,
> +                                             int32_t opt,
>                                                const char *context,
> -                                             uint32_t *context_id,
> +                                             NBDExportInfo *info,
>                                                Error **errp)
>   {
>       int ret;
>       NBDOptionReply reply;
>       uint32_t received_id = 0;
>       bool received = false;
> -    uint32_t export_len = strlen(export);
> -    uint32_t context_len = strlen(context);
> -    uint32_t data_len = sizeof(export_len) + export_len +
> -                        sizeof(uint32_t) + /* number of queries */
> -                        sizeof(context_len) + context_len;
> -    char *data = g_malloc(data_len);
> -    char *p = data;
> +    uint32_t export_len = strlen(info->name);
> +    uint32_t context_len;
> +    uint32_t data_len = sizeof(export_len) + export_len + sizeof(uint32_t);

I'd prefer to leave the comment /* number of queries */

> +    char *data;
> +    char *p;
> 
> -    trace_nbd_opt_meta_request(context, export);
> +    if (!context) {
> +        assert(opt == NBD_OPT_LIST_META_CONTEXT);
> +    } else {
> +        context_len = strlen(context);
> +        data_len += sizeof(context_len) + context_len;
> +    }
> +    data = g_malloc(data_len);
> +    p = data;
> +
> +    trace_nbd_opt_meta_request(nbd_opt_lookup(opt), context ?: "(all)",
> +                               info->name);
>       stl_be_p(p, export_len);
> -    memcpy(p += sizeof(export_len), export, export_len);
> -    stl_be_p(p += export_len, 1);
> -    stl_be_p(p += sizeof(uint32_t), context_len);
> -    memcpy(p += sizeof(context_len), context, context_len);
> +    memcpy(p += sizeof(export_len), info->name, export_len);
> +    stl_be_p(p += export_len, !!context);
> +    if (context) {
> +        stl_be_p(p += sizeof(uint32_t), context_len);
> +        memcpy(p += sizeof(context_len), context, context_len);
> +    }
> 
> -    ret = nbd_send_option_request(ioc, NBD_OPT_SET_META_CONTEXT, data_len, 
> data,
> -                                  errp);
> +    ret = nbd_send_option_request(ioc, opt, data_len, data, errp);
>       g_free(data);
>       if (ret < 0) {
>           return ret;
>       }
> 
> -    if (nbd_receive_option_reply(ioc, NBD_OPT_SET_META_CONTEXT, &reply,
> -                                 errp) < 0)
> +    if (nbd_receive_option_reply(ioc, opt, &reply, errp) < 0)
>       {
>           return -1;
>       }
> @@ -655,10 +663,10 @@ static int nbd_negotiate_simple_meta_context(QIOChannel 
> *ioc,
>           return ret;
>       }
> 
> -    if (reply.type == NBD_REP_META_CONTEXT) {
> +    while (reply.type == NBD_REP_META_CONTEXT) {
>           char *name;
> 
> -        if (reply.length != sizeof(received_id) + context_len) {
> +        if (reply.length <= sizeof(received_id)) {
>               error_setg(errp, "Failed to negotiate meta context '%s', server 
> "
>                          "answered with unexpected length %" PRIu32, context,
>                          reply.length);
> @@ -678,23 +686,31 @@ static int nbd_negotiate_simple_meta_context(QIOChannel 
> *ioc,
>               return -1;
>           }
>           name[reply.length] = '\0';
> -        if (strcmp(context, name)) {
> -            error_setg(errp, "Failed to negotiate meta context '%s', server "
> -                       "answered with different context '%s'", context,
> -                       name);
> -            g_free(name);
> -            nbd_send_opt_abort(ioc);
> -            return -1;
> +
> +        trace_nbd_opt_meta_reply(name, received_id);
> +        if (opt == NBD_OPT_SET_META_CONTEXT) {
> +            if (received) {
> +                error_setg(errp, "Server replied with more than one 
> context");
> +                free(name);

g_free

> +                nbd_send_opt_abort(ioc);
> +                return -1;
> +            }
> +
> +            if (strcmp(context, name)) {
> +                error_setg(errp,
> +                           "Failed to negotiate meta context '%s', server "
> +                           "answered with different context '%s'", context,
> +                           name);
> +                g_free(name);
> +                nbd_send_opt_abort(ioc);
> +                return -1;
> +            }
>           }
>           g_free(name);
> -
> -        trace_nbd_opt_meta_reply(context, received_id);
>           received = true;
> 
>           /* receive NBD_REP_ACK */
> -        if (nbd_receive_option_reply(ioc, NBD_OPT_SET_META_CONTEXT, &reply,
> -                                     errp) < 0)
> -        {
> +        if (nbd_receive_option_reply(ioc, opt, &reply, errp) < 0) {
>               return -1;
>           }
> 
> @@ -717,12 +733,11 @@ static int nbd_negotiate_simple_meta_context(QIOChannel 
> *ioc,
>           return -1;
>       }
> 
> -    if (received) {
> -        *context_id = received_id;
> -        return 1;
> +    if (received && opt == NBD_OPT_SET_META_CONTEXT) {
> +        info->meta_base_allocation_id = received_id;
>       }
> 
> -    return 0;
> +    return received || opt == NBD_OPT_LIST_META_CONTEXT;
>   }

the changes looks correct, but new semantics seems too weird for me.

> 
>   int nbd_receive_negotiate(QIOChannel *ioc, QCryptoTLSCreds *tlscreds,
> @@ -822,9 +837,9 @@ int nbd_receive_negotiate(QIOChannel *ioc, 
> QCryptoTLSCreds *tlscreds,
> 
>               if (info->structured_reply && base_allocation) {
>                   result = nbd_negotiate_simple_meta_context(
> -                        ioc, info->name,
> +                        ioc, NBD_OPT_SET_META_CONTEXT,
>                           info->x_dirty_bitmap ?: "base:allocation",
> -                        &info->meta_base_allocation_id, errp);
> +                        info, errp);
>                   if (result < 0) {
>                       goto fail;
>                   }
> diff --git a/nbd/trace-events b/nbd/trace-events
> index 289337d0dc3..5d0d202fad2 100644
> --- a/nbd/trace-events
> +++ b/nbd/trace-events
> @@ -10,7 +10,7 @@ nbd_receive_query_exports_start(const char *wantname) 
> "Querying export list for
>   nbd_receive_query_exports_success(const char *wantname) "Found desired 
> export name '%s'"
>   nbd_receive_starttls_new_client(void) "Setting up TLS"
>   nbd_receive_starttls_tls_handshake(void) "Starting TLS handshake"
> -nbd_opt_meta_request(const char *context, const char *export) "Requesting to 
> set meta context %s for export %s"
> +nbd_opt_meta_request(const char *opt, const char *context, const char 
> *export) "Requesting to %s %s for export %s"
>   nbd_opt_meta_reply(const char *context, uint32_t id) "Received mapping of 
> context %s to id %" PRIu32
>   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
> 


-- 
Best regards,
Vladimir