On 8 Apr 2016, at 23:05, Eric Blake <ebl...@redhat.com> wrote:
> Current upstream NBD documents that requests have a 16-bit flags,
> followed by a 16-bit type integer; although older versions mentioned
> only a 32-bit field with masking to find flags. Since the protocol
> is in network order (big-endian over the wire), the ABI is unchanged;
> but dealing with the flags as a separate field rather than masking
> will make it easier to add support for upcoming NBD extensions that
> increase the number of both flags and commands.
>
> Improve some comments in nbd.h based on the current upstream
> NBD protocol (https://github.com/yoe/nbd/blob/master/doc/proto.md),
> and touch some nearby code to keep checkpatch.pl happy.
>
> Signed-off-by: Eric Blake <ebl...@redhat.com>
Reviewed-by: Alex Bligh <a...@alex.org.uk>
> ---
> include/block/nbd.h | 18 ++++++++++++------
> nbd/nbd-internal.h | 4 ++--
> block/nbd-client.c | 9 +++------
> nbd/client.c | 17 ++++++++++-------
> nbd/server.c | 35 +++++++++++++++++++----------------
> 5 files changed, 46 insertions(+), 37 deletions(-)
>
> diff --git a/include/block/nbd.h b/include/block/nbd.h
> index 3f047bf..2c61901 100644
> --- a/include/block/nbd.h
> +++ b/include/block/nbd.h
> @@ -1,4 +1,5 @@
> /*
> + * Copyright (C) 2016 Red Hat, Inc.
> * Copyright (C) 2005 Anthony Liguori <anth...@codemonkey.ws>
> *
> * Network Block Device
> @@ -27,7 +28,8 @@
>
> struct nbd_request {
> uint32_t magic;
> - uint32_t type;
> + uint16_t flags;
> + uint16_t type;
> uint64_t handle;
> uint64_t from;
> uint32_t len;
> @@ -39,6 +41,8 @@ struct nbd_reply {
> uint64_t handle;
> } QEMU_PACKED;
>
> +/* Transmission (export) flags: sent from server to client during handshake,
> + but describe what will happen during transmission */
> #define NBD_FLAG_HAS_FLAGS (1 << 0) /* Flags are there */
> #define NBD_FLAG_READ_ONLY (1 << 1) /* Device is read-only */
> #define NBD_FLAG_SEND_FLUSH (1 << 2) /* Send FLUSH */
> @@ -46,10 +50,12 @@ struct nbd_reply {
> #define NBD_FLAG_ROTATIONAL (1 << 4) /* Use elevator algorithm -
> rotational media */
> #define NBD_FLAG_SEND_TRIM (1 << 5) /* Send TRIM (discard) */
>
> -/* New-style global flags. */
> +/* New-style handshake (global) flags, sent from server to client, and
> + control what will happen during handshake phase. */
> #define NBD_FLAG_FIXED_NEWSTYLE (1 << 0) /* Fixed newstyle protocol. */
>
> -/* New-style client flags. */
> +/* New-style client flags, sent from client to server to control what happens
> + during handshake phase. */
> #define NBD_FLAG_C_FIXED_NEWSTYLE (1 << 0) /* Fixed newstyle protocol. */
>
> /* Reply types. */
> @@ -60,10 +66,10 @@ struct nbd_reply {
> #define NBD_REP_ERR_INVALID ((UINT32_C(1) << 31) | 3) /* Invalid length.
> */
> #define NBD_REP_ERR_TLS_REQD ((UINT32_C(1) << 31) | 5) /* TLS required */
>
> +/* Request flags, sent from client to server during transmission phase */
> +#define NBD_CMD_FLAG_FUA (1 << 0)
>
> -#define NBD_CMD_MASK_COMMAND 0x0000ffff
> -#define NBD_CMD_FLAG_FUA (1 << 16)
> -
> +/* Supported request types */
> enum {
> NBD_CMD_READ = 0,
> NBD_CMD_WRITE = 1,
> diff --git a/nbd/nbd-internal.h b/nbd/nbd-internal.h
> index 3791535..b663bf3 100644
> --- a/nbd/nbd-internal.h
> +++ b/nbd/nbd-internal.h
> @@ -52,10 +52,10 @@
> /* This is all part of the "official" NBD API.
> *
> * The most up-to-date documentation is available at:
> - * https://github.com/yoe/nbd/blob/master/doc/proto.txt
> + * https://github.com/yoe/nbd/blob/master/doc/proto.md
> */
>
> -#define NBD_REQUEST_SIZE (4 + 4 + 8 + 8 + 4)
> +#define NBD_REQUEST_SIZE (4 + 2 + 2 + 8 + 8 + 4)
> #define NBD_REPLY_SIZE (4 + 4 + 8)
> #define NBD_REQUEST_MAGIC 0x25609513
> #define NBD_REPLY_MAGIC 0x67446698
> diff --git a/block/nbd-client.c b/block/nbd-client.c
> index 878e879..285025d 100644
> --- a/block/nbd-client.c
> +++ b/block/nbd-client.c
> @@ -1,6 +1,7 @@
> /*
> * QEMU Block driver for NBD
> *
> + * Copyright (C) 2016 Red Hat, Inc.
> * Copyright (C) 2008 Bull S.A.S.
> * Author: Laurent Vivier <laurent.viv...@bull.net>
> *
> @@ -252,7 +253,7 @@ static int nbd_co_writev_1(BlockDriverState *bs, int64_t
> sector_num,
>
> if ((*flags & BDRV_REQ_FUA) && (client->nbdflags & NBD_FLAG_SEND_FUA)) {
> *flags &= ~BDRV_REQ_FUA;
> - request.type |= NBD_CMD_FLAG_FUA;
> + request.flags |= NBD_CMD_FLAG_FUA;
> }
>
> request.from = sector_num * 512;
> @@ -376,11 +377,7 @@ void nbd_client_attach_aio_context(BlockDriverState *bs,
> void nbd_client_close(BlockDriverState *bs)
> {
> NbdClientSession *client = nbd_get_client_session(bs);
> - struct nbd_request request = {
> - .type = NBD_CMD_DISC,
> - .from = 0,
> - .len = 0
> - };
> + struct nbd_request request = { .type = NBD_CMD_DISC };
>
> if (client->ioc == NULL) {
> return;
> diff --git a/nbd/client.c b/nbd/client.c
> index 00f9244..7fd6059 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -1,4 +1,5 @@
> /*
> + * Copyright (C) 2016 Red Hat, Inc.
> * Copyright (C) 2005 Anthony Liguori <anth...@codemonkey.ws>
> *
> * Network Block Device Client Side
> @@ -688,14 +689,16 @@ ssize_t nbd_send_request(QIOChannel *ioc, struct
> nbd_request *request)
>
> TRACE("Sending request to server: "
> "{ .from = %" PRIu64", .len = %" PRIu32 ", .handle = %" PRIu64
> - ", .type=%" PRIu16 " }",
> - request->from, request->len, request->handle, request->type);
> + ", .flags=%" PRIx16 ", .type=%" PRIu16 " }",
> + request->from, request->len, request->handle,
> + request->flags, request->type);
>
> - cpu_to_be32w((uint32_t*)buf, NBD_REQUEST_MAGIC);
> - cpu_to_be32w((uint32_t*)(buf + 4), request->type);
> - cpu_to_be64w((uint64_t*)(buf + 8), request->handle);
> - cpu_to_be64w((uint64_t*)(buf + 16), request->from);
> - cpu_to_be32w((uint32_t*)(buf + 24), request->len);
> + cpu_to_be32w((uint32_t *)buf, NBD_REQUEST_MAGIC);
> + cpu_to_be16w((uint16_t *)(buf + 4), request->flags);
> + cpu_to_be16w((uint16_t *)(buf + 6), request->type);
> + cpu_to_be64w((uint64_t *)(buf + 8), request->handle);
> + cpu_to_be64w((uint64_t *)(buf + 16), request->from);
> + cpu_to_be32w((uint32_t *)(buf + 24), request->len);
>
> ret = write_sync(ioc, buf, sizeof(buf));
> if (ret < 0) {
> diff --git a/nbd/server.c b/nbd/server.c
> index 5414c49..93c077e 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -1,4 +1,5 @@
> /*
> + * Copyright (C) 2016 Red Hat, Inc.
> * Copyright (C) 2005 Anthony Liguori <anth...@codemonkey.ws>
> *
> * Network Block Device Server Side
> @@ -636,21 +637,23 @@ static ssize_t nbd_receive_request(QIOChannel *ioc,
> struct nbd_request *request)
>
> /* Request
> [ 0 .. 3] magic (NBD_REQUEST_MAGIC)
> - [ 4 .. 7] type (0 == READ, 1 == WRITE)
> + [ 4 .. 5] flags (NBD_CMD_FLAG_FUA, ...)
> + [ 6 .. 7] type (NBD_CMD_READ, ...)
> [ 8 .. 15] handle
> [16 .. 23] from
> [24 .. 27] len
> */
>
> - magic = be32_to_cpup((uint32_t*)buf);
> - request->type = be32_to_cpup((uint32_t*)(buf + 4));
> - request->handle = be64_to_cpup((uint64_t*)(buf + 8));
> - request->from = be64_to_cpup((uint64_t*)(buf + 16));
> - request->len = be32_to_cpup((uint32_t*)(buf + 24));
> + magic = be32_to_cpup((uint32_t *)buf);
> + request->flags = be16_to_cpup((uint16_t *)(buf + 4));
> + request->type = be16_to_cpup((uint16_t *)(buf + 6));
> + request->handle = be64_to_cpup((uint64_t *)(buf + 8));
> + request->from = be64_to_cpup((uint64_t *)(buf + 16));
> + request->len = be32_to_cpup((uint32_t *)(buf + 24));
>
> - TRACE("Got request: { magic = 0x%" PRIx32 ", .type = %" PRIx32
> - ", from = %" PRIu64 " , len = %" PRIu32 " }",
> - magic, request->type, request->from, request->len);
> + TRACE("Got request: { magic = 0x%" PRIx32 ", .flags = %" PRIx16
> + ".type = %" PRIx16 ", from = %" PRIu64 " , len = %" PRIu32 " }",
> + magic, request->flags, request->type, request->from, request->len);
>
> if (magic != NBD_REQUEST_MAGIC) {
> LOG("invalid magic (got 0x%" PRIx32 ")", magic);
> @@ -984,9 +987,8 @@ static ssize_t nbd_co_receive_request(NBDRequest *req,
> struct nbd_request *reque
> goto out;
> }
>
> - if (request->type & ~NBD_CMD_MASK_COMMAND & ~NBD_CMD_FLAG_FUA) {
> - LOG("unsupported flags (got 0x%x)",
> - request->type & ~NBD_CMD_MASK_COMMAND);
> + if (request->flags & ~NBD_CMD_FLAG_FUA) {
> + LOG("unsupported flags (got 0x%x)", request->flags);
> return -EINVAL;
> }
> if ((request->from + request->len) < request->from) {
> @@ -998,7 +1000,7 @@ static ssize_t nbd_co_receive_request(NBDRequest *req,
> struct nbd_request *reque
>
> TRACE("Decoding type");
>
> - command = request->type & NBD_CMD_MASK_COMMAND;
> + command = request->type;
> if (command == NBD_CMD_READ || command == NBD_CMD_WRITE) {
> if (request->len > NBD_MAX_BUFFER_SIZE) {
> LOG("len (%" PRIu32" ) is larger than max len (%u)",
> @@ -1062,7 +1064,7 @@ static void nbd_trip(void *opaque)
> reply.error = -ret;
> goto error_reply;
> }
> - command = request.type & NBD_CMD_MASK_COMMAND;
> + command = request.type;
> if (command != NBD_CMD_DISC && (request.from + request.len) > exp->size) {
> LOG("From: %" PRIu64 ", Len: %" PRIu32", Size: %" PRIu64
> ", Offset: %" PRIu64 "\n",
> @@ -1084,7 +1086,8 @@ static void nbd_trip(void *opaque)
> case NBD_CMD_READ:
> TRACE("Request type is READ");
>
> - if (request.type & NBD_CMD_FLAG_FUA) {
> + /* XXX: NBD Protocol only documents use of FUA with WRITE */
> + if (request.flags & NBD_CMD_FLAG_FUA) {
> ret = blk_co_flush(exp->blk);
> if (ret < 0) {
> LOG("flush failed");
> @@ -1126,7 +1129,7 @@ static void nbd_trip(void *opaque)
> goto error_reply;
> }
>
> - if (request.type & NBD_CMD_FLAG_FUA) {
> + if (request.flags & NBD_CMD_FLAG_FUA) {
> ret = blk_co_flush(exp->blk);
> if (ret < 0) {
> LOG("flush failed");
> --
> 2.5.5
>
>
--
Alex Bligh
