On 08.06.23 21:29, Eric Blake wrote:
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 the
Technically, 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 <ebl...@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
I vote for switch!) Really, seems it would be a lot simpler to read.
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).
--
Best regards,
Vladimir
