[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 30/44] nbd: Treat flags vs. command type as s
From: |
Alex Bligh |
Subject: |
Re: [Qemu-devel] [PATCH v3 30/44] nbd: Treat flags vs. command type as separate fields |
Date: |
Mon, 25 Apr 2016 10:34:04 +0100 |
On 23 Apr 2016, at 00:40, Eric Blake <address@hidden> 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 <address@hidden>
Reviewed-by: Alex Bligh <address@hidden>
>
> ---
> v3: rebase to other changes earlier in series
> ---
> include/block/nbd.h | 18 ++++++++++++------
> nbd/nbd-internal.h | 4 ++--
> block/nbd-client.c | 9 +++------
> nbd/client.c | 17 ++++++++++-------
> nbd/server.c | 51 ++++++++++++++++++++++++++-------------------------
> 5 files changed, 53 insertions(+), 46 deletions(-)
>
> diff --git a/include/block/nbd.h b/include/block/nbd.h
> index 2c753cc..f4ae86c 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 <address@hidden>
> *
> * 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 035ead4..d0108a1 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 <address@hidden>
> *
> @@ -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 b700100..a9e173a 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -1,4 +1,5 @@
> /*
> + * Copyright (C) 2016 Red Hat, Inc.
> * Copyright (C) 2005 Anthony Liguori <address@hidden>
> *
> * Network Block Device Client Side
> @@ -713,14 +714,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 a20bf8a..1d30b6d 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -1,4 +1,5 @@
> /*
> + * Copyright (C) 2016 Red Hat, Inc.
> * Copyright (C) 2005 Anthony Liguori <address@hidden>
> *
> * Network Block Device Server Side
> @@ -646,21 +647,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);
> @@ -993,7 +996,6 @@ static ssize_t nbd_co_receive_request(NBDRequest *req,
> struct nbd_request *request)
> {
> NBDClient *client = req->client;
> - uint32_t command;
> ssize_t rc;
>
> g_assert(qemu_in_coroutine());
> @@ -1010,8 +1012,7 @@ static ssize_t nbd_co_receive_request(NBDRequest *req,
>
> TRACE("Decoding type");
>
> - command = request->type & NBD_CMD_MASK_COMMAND;
> - if (command == NBD_CMD_DISC) {
> + if (request->type == NBD_CMD_DISC) {
> /* Special case: we're going to disconnect without a reply,
> * whether or not flags, from, or len are bogus */
> TRACE("Request type is DISCONNECT");
> @@ -1028,7 +1029,7 @@ static ssize_t nbd_co_receive_request(NBDRequest *req,
> goto out;
> }
>
> - if (command == NBD_CMD_READ || command == NBD_CMD_WRITE) {
> + if (request->type == NBD_CMD_READ || request->type == NBD_CMD_WRITE) {
> if (request->len > NBD_MAX_BUFFER_SIZE) {
> LOG("len (%" PRIu32" ) is larger than max len (%u)",
> request->len, NBD_MAX_BUFFER_SIZE);
> @@ -1042,7 +1043,7 @@ static ssize_t nbd_co_receive_request(NBDRequest *req,
> goto out;
> }
> }
> - if (command == NBD_CMD_WRITE) {
> + if (request->type == NBD_CMD_WRITE) {
> TRACE("Reading %" PRIu32 " byte(s)", request->len);
>
> if (read_sync(client->ioc, req->data, request->len) != request->len) {
> @@ -1061,10 +1062,10 @@ static ssize_t nbd_co_receive_request(NBDRequest *req,
> rc = -EINVAL;
> 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);
> - return -EINVAL;
> + if (request->flags & ~NBD_CMD_FLAG_FUA) {
> + LOG("unsupported flags (got 0x%x)", request->flags);
> + rc = -EINVAL;
> + goto out;
> }
>
> rc = 0;
> @@ -1084,7 +1085,6 @@ static void nbd_trip(void *opaque)
> struct nbd_request request;
> struct nbd_reply reply;
> ssize_t ret;
> - uint32_t command;
> int flags;
>
> TRACE("Reading request.");
> @@ -1108,7 +1108,6 @@ static void nbd_trip(void *opaque)
> reply.error = -ret;
> goto error_reply;
> }
> - command = request.type & NBD_CMD_MASK_COMMAND;
>
> if (client->closing) {
> /*
> @@ -1118,11 +1117,12 @@ static void nbd_trip(void *opaque)
> goto done;
> }
>
> - switch (command) {
> + switch (request.type) {
> 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");
> @@ -1155,7 +1155,7 @@ static void nbd_trip(void *opaque)
> TRACE("Writing to device");
>
> flags = 0;
> - if (request.type & NBD_CMD_FLAG_FUA) {
> + if (request.flags & NBD_CMD_FLAG_FUA) {
> flags |= BDRV_REQ_FUA;
> }
> ret = blk_pwrite(exp->blk, request.from + exp->dev_offset,
> @@ -1208,8 +1208,9 @@ static void nbd_trip(void *opaque)
> * NBD_CMD_READ, since we choose not to send bogus filler
> * data; likewise after NBD_CMD_WRITE if we did not read the
> * payload. */
> - if (nbd_co_send_reply(req, &reply, 0) < 0 || command == NBD_CMD_READ
> ||
> - (command == NBD_CMD_WRITE && !req->complete)) {
> + if (nbd_co_send_reply(req, &reply, 0) < 0 ||
> + request.type == NBD_CMD_READ ||
> + (request.type == NBD_CMD_WRITE && !req->complete)) {
> goto out;
> }
> break;
> --
> 2.5.5
>
>
--
Alex Bligh
- [Qemu-devel] [PATCH v3 17/44] nbd: Switch to byte-based block access, (continued)
- [Qemu-devel] [PATCH v3 17/44] nbd: Switch to byte-based block access, Eric Blake, 2016/04/22
- [Qemu-devel] [PATCH v3 11/44] nand: Switch to byte-based block access, Eric Blake, 2016/04/22
- [Qemu-devel] [PATCH v3 20/44] block: Switch blk_read_unthrottled() to byte interface, Eric Blake, 2016/04/22
- [Qemu-devel] [PATCH v3 22/44] block: Kill blk_write(), blk_read(), Eric Blake, 2016/04/22
- [Qemu-devel] [PATCH v3 21/44] block: Switch blk_write_zeroes() to byte interface, Eric Blake, 2016/04/22
- [Qemu-devel] [PATCH v3 28/44] nbd: Detect servers that send unexpected error values, Eric Blake, 2016/04/22
- [Qemu-devel] [PATCH v3 05/44] nbd: Group all Linux-specific ioctl code in one place, Eric Blake, 2016/04/22
- [Qemu-devel] [PATCH v3 33/44] nbd: Let client skip portions of server reply, Eric Blake, 2016/04/22
- [Qemu-devel] [PATCH v3 30/44] nbd: Treat flags vs. command type as separate fields, Eric Blake, 2016/04/22
- Re: [Qemu-devel] [PATCH v3 30/44] nbd: Treat flags vs. command type as separate fields,
Alex Bligh <=
- [Qemu-devel] [PATCH v3 38/44] block: Add blk_get_opt_transfer_length(), Eric Blake, 2016/04/22
- [Qemu-devel] [PATCH v3 41/44] nbd: Implement NBD_CMD_WRITE_ZEROES on server, Eric Blake, 2016/04/22
- [Qemu-devel] [PATCH v3 42/44] nbd: Implement NBD_CMD_WRITE_ZEROES on client, Eric Blake, 2016/04/22
- [Qemu-devel] [PATCH v3 37/44] nbd: Create struct for tracking export info, Eric Blake, 2016/04/22
- [Qemu-devel] [PATCH v3 43/44] nbd: Implement NBD_OPT_BLOCK_SIZE on server, Eric Blake, 2016/04/22
- [Qemu-devel] [PATCH v3 19/44] qemu-io: Switch to byte-based block access, Eric Blake, 2016/04/22