diff mbox series

[v3,06/14] nbd/server: Refactor handling of request payload

Message ID 20230515195343.1915857-7-eblake@redhat.com
State New
Headers show
Series qemu patches for 64-bit NBD extensions | expand

Commit Message

Eric Blake May 15, 2023, 7:53 p.m. UTC
Upcoming additions to support NBD 64-bit effect lengths allow for the
possibility to distinguish between payload length (capped at 32M) and
effect length (up to 63 bits).  Without that extension, only the
NBD_CMD_WRITE request has a payload; but with the extension, it makes
sense to allow at least NBD_CMD_BLOCK_STATUS to have both a payload
and effect length (where the payload is a limited-size struct that in
turns gives the real effect length as well as a subset of known ids
for which status is requested).  Other future NBD commands may also
have a request payload, so the 64-bit extension introduces a new
NBD_CMD_FLAG_PAYLOAD_LEN that distinguishes between whether the header
length is a payload length or an effect length, rather than
hard-coding the decision based on the command.  Note that we do not
support the payload version of BLOCK_STATUS yet.

For this patch, no semantic change is intended for a compliant client.
For a non-compliant client, it is possible that the error behavior
changes (a different message, a change on whether the connection is
killed or remains alive for the next command, or so forth), but all
errors should still be handled gracefully.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 nbd/server.c     | 55 +++++++++++++++++++++++++++++++++---------------
 nbd/trace-events |  1 +
 2 files changed, 39 insertions(+), 17 deletions(-)

Comments

Vladimir Sementsov-Ogievskiy May 31, 2023, 8:04 a.m. UTC | #1
On 15.05.23 22:53, Eric Blake wrote:
> Upcoming additions to support NBD 64-bit effect lengths allow for the
> possibility to distinguish between payload length (capped at 32M) and
> effect length (up to 63 bits).  Without that extension, only the
> NBD_CMD_WRITE request has a payload; but with the extension, it makes
> sense to allow at least NBD_CMD_BLOCK_STATUS to have both a payload
> and effect length (where the payload is a limited-size struct that in
> turns gives the real effect length as well as a subset of known ids
> for which status is requested).  Other future NBD commands may also
> have a request payload, so the 64-bit extension introduces a new
> NBD_CMD_FLAG_PAYLOAD_LEN that distinguishes between whether the header
> length is a payload length or an effect length, rather than
> hard-coding the decision based on the command.  Note that we do not
> support the payload version of BLOCK_STATUS yet.
> 
> For this patch, no semantic change is intended for a compliant client.
> For a non-compliant client, it is possible that the error behavior
> changes (a different message, a change on whether the connection is
> killed or remains alive for the next command, or so forth), but all
> errors should still be handled gracefully.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>   nbd/server.c     | 55 +++++++++++++++++++++++++++++++++---------------
>   nbd/trace-events |  1 +
>   2 files changed, 39 insertions(+), 17 deletions(-)
> 
> diff --git a/nbd/server.c b/nbd/server.c
> index cf38a104d9a..5812a773ace 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -2316,6 +2316,8 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, NBDRequest *
>                                                  Error **errp)
>   {
>       NBDClient *client = req->client;
> +    bool extended_with_payload;
> +    int payload_len = 0;
>       int valid_flags;
>       int ret;
> 
> @@ -2329,27 +2331,41 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, NBDRequest *
>       trace_nbd_co_receive_request_decode_type(request->handle, request->type,
>                                                nbd_cmd_lookup(request->type));
> 
> -    if (request->type != NBD_CMD_WRITE) {
> -        /* No payload, we are ready to read the next request.  */
> -        req->complete = true;
> -    }
> -
>       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 */
> +        req->complete = true;
>           return -EIO;
>       }
> 
> +    /* Payload and buffer handling. */
> +    extended_with_payload = client->header_style >= NBD_HEADER_EXTENDED &&
> +        (request->flags & NBD_CMD_FLAG_PAYLOAD_LEN);
>       if (request->type == NBD_CMD_READ || request->type == NBD_CMD_WRITE ||
> -        request->type == NBD_CMD_CACHE)
> -    {
> +        request->type == NBD_CMD_CACHE || extended_with_payload) {
>           if (request->len > NBD_MAX_BUFFER_SIZE) {
>               error_setg(errp, "len (%" PRIu64" ) is larger than max len (%u)",
>                          request->len, NBD_MAX_BUFFER_SIZE);

hmm pre-patch, here req->complete is set to true, except for WRITE request

>               return -EINVAL;
>           }
> 

the whole big if with sub-ifs becomes really hard to read. At least, I think, no reason to keep the following two ifs inside the bigger if, as small ifs just check the same conditions.

> -        if (request->type != NBD_CMD_CACHE) {
> +        if (request->type == NBD_CMD_WRITE || extended_with_payload) {
> +            payload_len = request->len;
> +            if (request->type != NBD_CMD_WRITE) {
> +                /*
> +                 * For now, we don't support payloads on other
> +                 * commands; but we can keep the connection alive.
> +                 */
> +                request->len = 0;
> +            } else if (client->header_style >= NBD_HEADER_EXTENDED &&
> +                       !extended_with_payload) {
> +                /* The client is noncompliant. Trace it, but proceed. */
> +                trace_nbd_co_receive_ext_payload_compliance(request->from,
> +                                                            request->len);
> +            }
> +        }
> +
> +        if (request->type == NBD_CMD_WRITE || request->type == NBD_CMD_READ) {
>               req->data = blk_try_blockalign(client->exp->common.blk,
>                                              request->len);
>               if (req->data == NULL) {
> @@ -2359,18 +2375,20 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, NBDRequest *
>           }
>       }
> 
> -    if (request->type == NBD_CMD_WRITE) {
> -        assert(request->len <= NBD_MAX_BUFFER_SIZE);
> -        if (nbd_read(client->ioc, req->data, request->len, "CMD_WRITE data",
> -                     errp) < 0)
> -        {
> +    if (payload_len) {
> +        if (req->data) {
> +            ret = nbd_read(client->ioc, req->data, payload_len,
> +                           "CMD_WRITE data", errp);
> +        } else {
> +            ret = nbd_drop(client->ioc, payload_len, errp);
> +        }
> +        if (ret < 0) {
>               return -EIO;
>           }
> -        req->complete = true;
> -
>           trace_nbd_co_receive_request_payload_received(request->handle,
> -                                                      request->len);
> +                                                      payload_len);
>       }
> +    req->complete = true;
> 
>       /* Sanity checks. */
>       if (client->exp->nbdflags & NBD_FLAG_READ_ONLY &&
> @@ -2400,7 +2418,10 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, NBDRequest *
>                                                 client->check_align);
>       }
>       valid_flags = NBD_CMD_FLAG_FUA;
> -    if (request->type == NBD_CMD_READ &&
> +    if (request->type == NBD_CMD_WRITE &&
> +        client->header_style >= NBD_HEADER_EXTENDED) {
> +        valid_flags |= NBD_CMD_FLAG_PAYLOAD_LEN;
> +    } else if (request->type == NBD_CMD_READ &&
>           client->header_style >= NBD_HEADER_STRUCTURED) {
>           valid_flags |= NBD_CMD_FLAG_DF;
>       } else if (request->type == NBD_CMD_WRITE_ZEROES) {
> diff --git a/nbd/trace-events b/nbd/trace-events
> index e2c1d68688d..adf5666e207 100644
> --- a/nbd/trace-events
> +++ b/nbd/trace-events
> @@ -71,6 +71,7 @@ nbd_co_send_extents(uint64_t handle, unsigned int extents, uint32_t id, uint64_t
>   nbd_co_send_structured_error(uint64_t handle, int err, const char *errname, const char *msg) "Send structured error reply: handle = %" PRIu64 ", error = %d (%s), msg = '%s'"
>   nbd_co_receive_request_decode_type(uint64_t handle, uint16_t type, const char *name) "Decoding type: handle = %" PRIu64 ", type = %" PRIu16 " (%s)"
>   nbd_co_receive_request_payload_received(uint64_t handle, uint64_t len) "Payload received: handle = %" PRIu64 ", len = %" PRIu64
> +nbd_co_receive_ext_payload_compliance(uint64_t from, uint64_t len) "client sent non-compliant write without payload flag: from=0x%" PRIx64 ", len=0x%" PRIx64
>   nbd_co_receive_align_compliance(const char *op, uint64_t from, uint64_t len, uint32_t align) "client sent non-compliant unaligned %s request: from=0x%" PRIx64 ", len=0x%" PRIx64 ", align=0x%" PRIx32
>   nbd_trip(void) "Reading request"
>
diff mbox series

Patch

diff --git a/nbd/server.c b/nbd/server.c
index cf38a104d9a..5812a773ace 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -2316,6 +2316,8 @@  static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, NBDRequest *
                                                Error **errp)
 {
     NBDClient *client = req->client;
+    bool extended_with_payload;
+    int payload_len = 0;
     int valid_flags;
     int ret;

@@ -2329,27 +2331,41 @@  static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, NBDRequest *
     trace_nbd_co_receive_request_decode_type(request->handle, request->type,
                                              nbd_cmd_lookup(request->type));

-    if (request->type != NBD_CMD_WRITE) {
-        /* No payload, we are ready to read the next request.  */
-        req->complete = true;
-    }
-
     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 */
+        req->complete = true;
         return -EIO;
     }

+    /* Payload and buffer handling. */
+    extended_with_payload = client->header_style >= NBD_HEADER_EXTENDED &&
+        (request->flags & NBD_CMD_FLAG_PAYLOAD_LEN);
     if (request->type == NBD_CMD_READ || request->type == NBD_CMD_WRITE ||
-        request->type == NBD_CMD_CACHE)
-    {
+        request->type == NBD_CMD_CACHE || extended_with_payload) {
         if (request->len > NBD_MAX_BUFFER_SIZE) {
             error_setg(errp, "len (%" PRIu64" ) is larger than max len (%u)",
                        request->len, NBD_MAX_BUFFER_SIZE);
             return -EINVAL;
         }

-        if (request->type != NBD_CMD_CACHE) {
+        if (request->type == NBD_CMD_WRITE || extended_with_payload) {
+            payload_len = request->len;
+            if (request->type != NBD_CMD_WRITE) {
+                /*
+                 * For now, we don't support payloads on other
+                 * commands; but we can keep the connection alive.
+                 */
+                request->len = 0;
+            } else if (client->header_style >= NBD_HEADER_EXTENDED &&
+                       !extended_with_payload) {
+                /* The client is noncompliant. Trace it, but proceed. */
+                trace_nbd_co_receive_ext_payload_compliance(request->from,
+                                                            request->len);
+            }
+        }
+
+        if (request->type == NBD_CMD_WRITE || request->type == NBD_CMD_READ) {
             req->data = blk_try_blockalign(client->exp->common.blk,
                                            request->len);
             if (req->data == NULL) {
@@ -2359,18 +2375,20 @@  static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, NBDRequest *
         }
     }

-    if (request->type == NBD_CMD_WRITE) {
-        assert(request->len <= NBD_MAX_BUFFER_SIZE);
-        if (nbd_read(client->ioc, req->data, request->len, "CMD_WRITE data",
-                     errp) < 0)
-        {
+    if (payload_len) {
+        if (req->data) {
+            ret = nbd_read(client->ioc, req->data, payload_len,
+                           "CMD_WRITE data", errp);
+        } else {
+            ret = nbd_drop(client->ioc, payload_len, errp);
+        }
+        if (ret < 0) {
             return -EIO;
         }
-        req->complete = true;
-
         trace_nbd_co_receive_request_payload_received(request->handle,
-                                                      request->len);
+                                                      payload_len);
     }
+    req->complete = true;

     /* Sanity checks. */
     if (client->exp->nbdflags & NBD_FLAG_READ_ONLY &&
@@ -2400,7 +2418,10 @@  static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, NBDRequest *
                                               client->check_align);
     }
     valid_flags = NBD_CMD_FLAG_FUA;
-    if (request->type == NBD_CMD_READ &&
+    if (request->type == NBD_CMD_WRITE &&
+        client->header_style >= NBD_HEADER_EXTENDED) {
+        valid_flags |= NBD_CMD_FLAG_PAYLOAD_LEN;
+    } else if (request->type == NBD_CMD_READ &&
         client->header_style >= NBD_HEADER_STRUCTURED) {
         valid_flags |= NBD_CMD_FLAG_DF;
     } else if (request->type == NBD_CMD_WRITE_ZEROES) {
diff --git a/nbd/trace-events b/nbd/trace-events
index e2c1d68688d..adf5666e207 100644
--- a/nbd/trace-events
+++ b/nbd/trace-events
@@ -71,6 +71,7 @@  nbd_co_send_extents(uint64_t handle, unsigned int extents, uint32_t id, uint64_t
 nbd_co_send_structured_error(uint64_t handle, int err, const char *errname, const char *msg) "Send structured error reply: handle = %" PRIu64 ", error = %d (%s), msg = '%s'"
 nbd_co_receive_request_decode_type(uint64_t handle, uint16_t type, const char *name) "Decoding type: handle = %" PRIu64 ", type = %" PRIu16 " (%s)"
 nbd_co_receive_request_payload_received(uint64_t handle, uint64_t len) "Payload received: handle = %" PRIu64 ", len = %" PRIu64
+nbd_co_receive_ext_payload_compliance(uint64_t from, uint64_t len) "client sent non-compliant write without payload flag: from=0x%" PRIx64 ", len=0x%" PRIx64
 nbd_co_receive_align_compliance(const char *op, uint64_t from, uint64_t len, uint32_t align) "client sent non-compliant unaligned %s request: from=0x%" PRIx64 ", len=0x%" PRIx64 ", align=0x%" PRIx32
 nbd_trip(void) "Reading request"