Eric Blake
2023-Jun-08 13:56 UTC
[Libguestfs] [PATCH v4 13/24] nbd/server: Refactor handling of request payload
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), in part
because req->complete is set later on some paths, but all errors
should still be handled gracefully.
Signed-off-by: Eric Blake <eblake at redhat.com>
---
v4: less indentation on several 'if's [Vladimir]
---
nbd/server.c | 76 ++++++++++++++++++++++++++++++------------------
nbd/trace-events | 1 +
2 files changed, 49 insertions(+), 28 deletions(-)
diff --git a/nbd/server.c b/nbd/server.c
index 4ac05d0cd7b..d7dc29f0445 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -2329,6 +2329,8 @@ static int coroutine_fn
nbd_co_receive_request(NBDRequestData *req, NBDRequest *
Error **errp)
{
NBDClient *client = req->client;
+ bool extended_with_payload;
+ unsigned payload_len = 0;
int valid_flags;
int ret;
@@ -2342,48 +2344,63 @@ static int coroutine_fn
nbd_co_receive_request(NBDRequestData *req, NBDRequest *
trace_nbd_co_receive_request_decode_type(request->cookie,
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;
}
- if (request->type == NBD_CMD_READ || request->type == NBD_CMD_WRITE
||
- request->type == NBD_CMD_CACHE)
- {
- 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;
+ /* Payload and buffer handling. */
+ extended_with_payload = client->mode >= NBD_MODE_EXTENDED &&
+ (request->flags & NBD_CMD_FLAG_PAYLOAD_LEN);
+ if ((request->type == NBD_CMD_READ || request->type == NBD_CMD_WRITE
||
+ request->type == NBD_CMD_CACHE || extended_with_payload) &&
+ 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_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->mode >= NBD_MODE_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_CACHE) {
- req->data = blk_try_blockalign(client->exp->common.blk,
- request->len);
- if (req->data == NULL) {
- error_setg(errp, "No memory");
- return -ENOMEM;
- }
+ 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) {
+ error_setg(errp, "No memory");
+ return -ENOMEM;
}
}
- 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->cookie,
- request->len);
+ payload_len);
}
+ req->complete = true;
/* Sanity checks. */
if (client->exp->nbdflags & NBD_FLAG_READ_ONLY &&
@@ -2413,7 +2430,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 && client->mode >=
NBD_MODE_STRUCTURED) {
+ if (request->type == NBD_CMD_WRITE && client->mode >=
NBD_MODE_EXTENDED) {
+ valid_flags |= NBD_CMD_FLAG_PAYLOAD_LEN;
+ } else if (request->type == NBD_CMD_READ &&
+ client->mode >= NBD_MODE_STRUCTURED) {
valid_flags |= NBD_CMD_FLAG_DF;
} else if (request->type == NBD_CMD_WRITE_ZEROES) {
valid_flags |= NBD_CMD_FLAG_NO_HOLE | NBD_CMD_FLAG_FAST_ZERO;
diff --git a/nbd/trace-events b/nbd/trace-events
index 3338da2be2a..6a34d7f027a 100644
--- a/nbd/trace-events
+++ b/nbd/trace-events
@@ -71,6 +71,7 @@ nbd_co_send_extents(uint64_t cookie, unsigned int extents,
uint32_t id, uint64_t
nbd_co_send_chunk_error(uint64_t cookie, int err, const char *errname, const
char *msg) "Send structured error reply: cookie = %" PRIu64 ",
error = %d (%s), msg = '%s'"
nbd_co_receive_request_decode_type(uint64_t cookie, uint16_t type, const char
*name) "Decoding type: cookie = %" PRIu64 ", type = %"
PRIu16 " (%s)"
nbd_co_receive_request_payload_received(uint64_t cookie, uint64_t len)
"Payload received: cookie = %" 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"
--
2.40.1
Eric Blake
2023-Jun-08 18:29 UTC
[Libguestfs] [PATCH v4 13/24] nbd/server: Refactor handling of request payload
On Thu, Jun 08, 2023 at 08:56:42AM -0500, 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 theTechnically, the NBD spec does not (yet) have a documented cap of a 63-bit size limit; although I should probably propose a patch upstream that does that (I had one in my draft work, but it hasn't yet made it upstream)> 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), in part > because req->complete is set later on some paths, but all errors > should still be handled gracefully. > > Signed-off-by: Eric Blake <eblake at redhat.com> > --- > > v4: less indentation on several 'if's [Vladimir] > --- > nbd/server.c | 76 ++++++++++++++++++++++++++++++------------------ > nbd/trace-events | 1 + > 2 files changed, 49 insertions(+), 28 deletions(-) > > diff --git a/nbd/server.c b/nbd/server.c > index 4ac05d0cd7b..d7dc29f0445 100644 > --- a/nbd/server.c > +++ b/nbd/server.c > @@ -2329,6 +2329,8 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, NBDRequest * > Error **errp) > { > NBDClient *client = req->client; > + bool extended_with_payload; > + unsigned payload_len = 0; > int valid_flags; > int ret; > > @@ -2342,48 +2344,63 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, NBDRequest * > trace_nbd_co_receive_request_decode_type(request->cookie, 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; > } > > - if (request->type == NBD_CMD_READ || request->type == NBD_CMD_WRITE || > - request->type == NBD_CMD_CACHE) > - { > - 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; > + /* Payload and buffer handling. */ > + extended_with_payload = client->mode >= NBD_MODE_EXTENDED && > + (request->flags & NBD_CMD_FLAG_PAYLOAD_LEN); > + if ((request->type == NBD_CMD_READ || request->type == NBD_CMD_WRITE || > + request->type == NBD_CMD_CACHE || extended_with_payload) && > + request->len > NBD_MAX_BUFFER_SIZE) {I'm still debating about rewriting this series of slightly-different 'if' into a single switch (request->type) block; the benefit is that all actions for one command will be localized instead of split over multiple lines of if, the drawback is that some code will now have to be duplicated across commands (such as the buffer allocation code for CMD_READ and CMD_WRITE). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org