Re: [Qemu-block] [PATCH v2 11/22] nbd/client: Change signature of nbd_negotiate_simple_meta_context()

[Richard W.M. Jones]

On Sat, Dec 15, 2018 at 07:53:13AM -0600, Eric Blake wrote:
> Pass 'info' instead of three separate parameters related to info,
> when requesting the server to set the meta context.  Update the
> NBDExportInfo struct to rename the received id field to match the
> fact that we are currently overloading the field to match whatever
> context the user supplied through the x-dirty-bitmap hack, as well
> as adding a TODO comment to remind future patches about a desire
> to request two contexts at once.
> 
> Signed-off-by: Eric Blake <address@hidden>
> ---
> v2: split patch into easier-to-review pieces [Rich, Vladimir]
> rename NBDExportInfo meta_base_allocation_id [Vladimir]
> ---
>  include/block/nbd.h |  2 +-
>  block/nbd-client.c  |  4 ++--
>  nbd/client.c        | 53 +++++++++++++++++++++------------------------
>  3 files changed, 28 insertions(+), 31 deletions(-)
> 
> diff --git a/include/block/nbd.h b/include/block/nbd.h
> index 65feff8ba96..ae5fe28f486 100644
> --- a/include/block/nbd.h
> +++ b/include/block/nbd.h
> @@ -276,7 +276,7 @@ struct NBDExportInfo {
>      uint32_t opt_block;
>      uint32_t max_block;
> 
> -    uint32_t meta_base_allocation_id;
> +    uint32_t context_id;
>  };
>  typedef struct NBDExportInfo NBDExportInfo;
> 
> diff --git a/block/nbd-client.c b/block/nbd-client.c
> index 417971d8b05..608b578e1d3 100644
> --- a/block/nbd-client.c
> +++ b/block/nbd-client.c
> @@ -246,11 +246,11 @@ static int 
> nbd_parse_blockstatus_payload(NBDClientSession *client,
>      }
> 
>      context_id = payload_advance32(&payload);
> -    if (client->info.meta_base_allocation_id != context_id) {
> +    if (client->info.context_id != context_id) {
>          error_setg(errp, "Protocol error: unexpected context id %d for "
>                           "NBD_REPLY_TYPE_BLOCK_STATUS, when negotiated 
> context "
>                           "id is %d", context_id,
> -                         client->info.meta_base_allocation_id);
> +                         client->info.context_id);
>          return -EINVAL;
>      }
> 
> diff --git a/nbd/client.c b/nbd/client.c
> index 7462fa5ae0e..bcccd5f555e 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -628,26 +628,30 @@ 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
> + * 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
> + * responds with more than one context or with a context different
> + * than the query.
> + * return 1 for successful negotiation,
>   *        0 if operation is unsupported,
>   *        -1 with errp set for any other error
>   */
>  static int nbd_negotiate_simple_meta_context(QIOChannel *ioc,
> -                                             const char *export,
> -                                             const char *context,
> -                                             uint32_t *context_id,
> +                                             NBDExportInfo *info,
>                                               Error **errp)
>  {
> +    /*
> +     * TODO: Removing the x_dirty_bitmap hack will mean refactoring
> +     * this function to request and store ids for multiple contexts
> +     * (both base:allocation and a dirty bitmap), at which point this
> +     * function should lose the term _simple.
> +     */
>      int ret;
>      NBDOptionReply reply;
> -    uint32_t received_id = 0;
> +    const char *context = info->x_dirty_bitmap ?: "base:allocation";
>      bool received = false;
> -    uint32_t export_len = strlen(export);
> +    uint32_t export_len = strlen(info->name);
>      uint32_t context_len = strlen(context);
>      uint32_t data_len = sizeof(export_len) + export_len +
>                          sizeof(uint32_t) + /* number of queries */
> @@ -655,9 +659,9 @@ static int nbd_negotiate_simple_meta_context(QIOChannel 
> *ioc,
>      char *data = g_malloc(data_len);
>      char *p = data;
> 
> -    trace_nbd_opt_meta_request(context, export);
> +    trace_nbd_opt_meta_request(context, info->name);
>      stl_be_p(p, export_len);
> -    memcpy(p += sizeof(export_len), export, export_len);
> +    memcpy(p += sizeof(export_len), info->name, 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);
> @@ -683,7 +687,7 @@ static int nbd_negotiate_simple_meta_context(QIOChannel 
> *ioc,
>      if (reply.type == NBD_REP_META_CONTEXT) {
>          char *name;
> 
> -        if (reply.length != sizeof(received_id) + context_len) {
> +        if (reply.length != sizeof(info->context_id) + context_len) {
>              error_setg(errp, "Failed to negotiate meta context '%s', server "
>                         "answered with unexpected length %" PRIu32, context,
>                         reply.length);
> @@ -691,12 +695,13 @@ static int nbd_negotiate_simple_meta_context(QIOChannel 
> *ioc,
>              return -1;
>          }
> 
> -        if (nbd_read(ioc, &received_id, sizeof(received_id), errp) < 0) {
> +        if (nbd_read(ioc, &info->context_id, sizeof(info->context_id),
> +                     errp) < 0) {
>              return -1;
>          }
> -        received_id = be32_to_cpu(received_id);
> +        info->context_id = be32_to_cpu(info->context_id);
> 
> -        reply.length -= sizeof(received_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);
> @@ -713,7 +718,7 @@ static int nbd_negotiate_simple_meta_context(QIOChannel 
> *ioc,
>          }
>          g_free(name);
> 
> -        trace_nbd_opt_meta_reply(context, received_id);
> +        trace_nbd_opt_meta_reply(context, info->context_id);
>          received = true;
> 
>          /* receive NBD_REP_ACK */
> @@ -742,12 +747,7 @@ static int nbd_negotiate_simple_meta_context(QIOChannel 
> *ioc,
>          return -1;
>      }
> 
> -    if (received) {
> -        *context_id = received_id;
> -        return 1;
> -    }
> -
> -    return 0;
> +    return received;
>  }
> 
>  int nbd_receive_negotiate(QIOChannel *ioc, QCryptoTLSCreds *tlscreds,
> @@ -846,10 +846,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, 
> QCryptoTLSCreds *tlscreds,
>              }
> 
>              if (info->structured_reply && base_allocation) {
> -                result = nbd_negotiate_simple_meta_context(
> -                        ioc, info->name,
> -                        info->x_dirty_bitmap ?: "base:allocation",
> -                        &info->meta_base_allocation_id, errp);
> +                result = nbd_negotiate_simple_meta_context(ioc, info, errp);
>                  if (result < 0) {
>                      goto fail;
>                  }

This is much clearer and smaller refactoring so:

Reviewed-by: Richard W.M. Jones <address@hidden>

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