Re: [Libguestfs] [PATCH v4 13/24] nbd/server: Refactor handling of request payload

[Eric Blake]

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 <eblake@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