[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v2 11/22] nbd/client: Change signature of nbd_ne
From: |
Richard W.M. Jones |
Subject: |
Re: [Qemu-block] [PATCH v2 11/22] nbd/client: Change signature of nbd_negotiate_simple_meta_context() |
Date: |
Sat, 15 Dec 2018 15:12:15 +0000 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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
- [Qemu-block] [PATCH v2 04/22] qemu-nbd: Enhance man page, (continued)
- [Qemu-block] [PATCH v2 04/22] qemu-nbd: Enhance man page, Eric Blake, 2018/12/15
- [Qemu-block] [PATCH v2 06/22] qemu-nbd: Fail earlier for -c/-d on non-linux, Eric Blake, 2018/12/15
- [Qemu-block] [PATCH v2 07/22] qemu-nbd: Avoid strtol open-coding, Eric Blake, 2018/12/15
- [Qemu-block] [PATCH v2 11/22] nbd/client: Change signature of nbd_negotiate_simple_meta_context(), Eric Blake, 2018/12/15
- [Qemu-block] [PATCH v2 08/22] nbd/client: Drop pointless buf variable, Eric Blake, 2018/12/15
- [Qemu-block] [PATCH v2 10/22] nbd/client: Move export name into NBDExportInfo, Eric Blake, 2018/12/15
- [Qemu-block] [PATCH v2 13/22] nbd/client: Split out nbd_send_one_meta_context(), Eric Blake, 2018/12/15
- [Qemu-block] [PATCH v2 09/22] nbd/client: Refactor nbd_receive_list(), Eric Blake, 2018/12/15
- [Qemu-block] [PATCH v2 12/22] nbd/client: Improve error handling in nbd_negotiate_simple_meta_context(), Eric Blake, 2018/12/15