Re: [Qemu-devel] [PATCH v5 11/11] nbd: Minimal structured read for client

[Eric Blake <eblake@redhat.com>]

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

On 10/19/2017 05:26 PM, Eric Blake wrote:
> From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> Minimal implementation: for structured error only error_report error
> message.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> 

> +static int nbd_parse_offset_hole_payload(NBDStructuredReplyChunk *chunk,
> +                                         uint8_t *payload, QEMUIOVector *qiov,
> +                                         Error **errp)
> +{
> +    uint64_t offset;
> +    uint32_t hole_size;
> +
> +    if (chunk->length != sizeof(offset) + sizeof(hole_size)) {
> +        error_setg(errp, "Protocol error: invalid payload for "
> +                         "NBD_REPLY_TYPE_OFFSET_HOLE");
> +        return -EINVAL;
> +    }
> +
> +    offset = payload_advance64(&payload);
> +    hole_size = payload_advance32(&payload);
> +
> +    if (offset > qiov->size - hole_size) {
> +        error_setg(errp, "Protocol error: server sent chunk exceeding requested"
> +                         " region");
> +        return -EINVAL;
> +    }
> +
> +    qemu_iovec_memset(qiov, offset, 0, hole_size);

Ouch. We have a discrepancy here, that needs clarification in the NBD
spec (cc'd).  In your server implementation, you are returning the
offset as sent by the client (that is, all offsets are absolute to the
size of the export).  But in this client implementation, you are
computing where to decode the zeroes by treating offset as though it
were relative to the request, rather than absolute to the export.

Or, in numbers, if I request a read of 2k starting at an offset of 4k,
the server implementation returns an offset of 4k, and the client
rejects the message because 4k is larger than the 2k request.

I think that absolute numbers are better to work with than
request-relative, but don't see anything in the proposed spec that
states one way or the other, so this is your chance to agree with my
preference or else argue why request-relative offsets are nicer, before
wordsmithing a change to the spec to make it obvious which semantics are
intended.  Then I can change either the server to match (if we want
request-relative) or the client to remember the original offset it sent
in order to turn absolute answers from the server back into
request-relative offsets for where to place the zeroes (if we go with
absolute offsets).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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