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 <eblake@redhat.com>
---
 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.
+ * 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.
+ * return 1 for successful negotiation,
+ *        0 if operation is unsupported or context unavailable,
  *        -1 with errp set for any other error
  */
 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);
+    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);
+                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;
 }

 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
-- 
2.17.2

On Fri, Nov 30, 2018 at 04:03:36PM -0600, 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 <eblake@redhat.com>
> ---
>  nbd/client.c     | 105 +++++++++++++++++++++++++++--------------------
>  nbd/trace-events |   2 +-
>  2 files changed, 61 insertions(+), 46 deletions(-)

I've stared at this patch over a 12 hour period now and while I do
_not_ think it's wrong at all, and I've tested it against nbdkit, it's
not very easy to follow.  Also my unfamiliarity with the qemu block
layer / NBD client code doesn't help.

> 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.
> + * 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.
> + * return 1 for successful negotiation,
> + *        0 if operation is unsupported or context unavailable,
>   *        -1 with errp set for any other error
>   */
>  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);
> +    char *data;
> +    char *p;

I think you could add a comment about how data_len is calculated here.
The calculation is different from the previous code (in the fragment
above, but the fragment below makes the calculation the same).  The
old code at least had a brief comment.

> -    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);
> +    }

Again even though the old code wasn't commented, a brief comment
describing the data layout wouldn't go amiss.

> -    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);
> +                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;
>  }
> 
>  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
> -- 
> 2.17.2

Tested-by: Richard W.M. Jones <rjones@redhat.com>

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

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 <eblake@redhat.com>
> ---
>   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

On 12/6/18 7:20 AM, Vladimir Sementsov-Ogievskiy wrote:
> 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 <eblake@redhat.com>
>> ---
>>    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?

Okay, that's two different reviewers complaining. I'll do a bit more 
refactoring and comments for v2 to make it more obvious that I'm writing 
a common routine that can handle the bulk of the commonality between 
list and set; but maybe keep two wrappers so that the existing callers 
don't have to change their calls as drastically.

> 
>> + * 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.

No, the next patch uses "qemu:" as the context for LIST when recursing 
to work around qemu-nbd 3.0 not advertising the dirty-bitmap context 
under list with 0 queries.

> 
> 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.

Yes, I think a function rename will help.

> 
>> + * 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

It's related - but list returns 1 for a lot more cases than set (a 
successful negotiation for list meant that all server replies were 
processed, while a successful negotiation for set requires that the 
server replies with exactly the one context we requested).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org