Re: [Qemu-devel] [PATCH v4 04/11] nbd: Improve server handling of bogus commands

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2189 bytes --]

On 06/13/2016 06:19 AM, Paolo Bonzini wrote:

>> +    /* Sanity checks, part 2. */
>> +    if (request->from + request->len > client->exp->size) {
>> +        LOG("operation past EOF; From: %" PRIu64 ", Len: %" PRIu32
>> +            ", Size: %" PRIu64, request->from, request->len,
>> +            (uint64_t)client->exp->size);
>> +        rc = -EINVAL;
> 
> For writes, this should be ENOSPC according to the spec.

Good call.


>> +        if (nbd_co_send_reply(req, &reply, 0) < 0 || command == NBD_CMD_READ ||
>> +            (command == NBD_CMD_WRITE && !req->complete)) {
> 
> It's simpler to always set req->complete.  Putting everything together:
> 
> diff --git a/nbd/server.c b/nbd/server.c
> index 4743316..73505dc 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -1017,6 +1017,11 @@ static ssize_t nbd_co_receive_request(NBDRequest *req,
>      TRACE("Decoding type");
>  
>      command = request->type & NBD_CMD_MASK_COMMAND;
> +    if (command != NBD_CMD_WRITE) {
> +        /* No payload, we are ready to read the next request.  */
> +        req->complete = true;
> +    }
> +

Nice.

> @@ -1213,12 +1218,9 @@ static void nbd_trip(void *opaque)
>          LOG("invalid request type (%" PRIu32 ") received", request.type);
>          reply.error = EINVAL;
>      error_reply:
> -        /* We must disconnect after replying with an error to
> -         * 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)) {
> +        /* We must disconnect after NBD_CMD_WRITE if we did not
> +         * read the payload. */
> +        if (nbd_co_send_reply(req, &reply, 0) < 0 || !req->complete)) {

I'm not sure I agree with your change on NBD_CMD_READ, but we can hash
that out with upstream NBD list on the correct protocol, and make any
further changes as a followup.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]