Re: [Qemu-block] [Qemu-devel] [PATCH v3 30/44] nbd: Treat flags vs. command type as separate fields

[Alex Bligh]

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