Re: [Qemu-devel] [PATCH 07/18] nbd: Treat flags vs. command type as separate fields

[Alex Bligh]

On 8 Apr 2016, at 23:05, 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>

> ---
> 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 <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 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 <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 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 <address@hidden>
>  *
>  *  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 <address@hidden>
>  *
>  *  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