Re: [Qemu-devel] [PATCH v4 RFC 9/8] nbd: Minimal structured read for client

[Eric Blake <eblake@redhat.com>]

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

On 10/17/2017 07:57 AM, Vladimir Sementsov-Ogievskiy wrote:
> Minimal implementation: for structured error only error_report error
> message.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---

and I replied:
> 
> But in the client, I then perform 'w 0 0' (a zero-byte write, which
> should fail because the server is read-only).  I see:
> 
> C: 19481@1508268433.381446:nbd_send_request Sending request to server: {
> .from = 0, .len = 0, .handle = 93997172956880, .flags = 0x1, .type = 1
> (write) }
> S: 19479@1508268433.381516:nbd_receive_request Got request: { magic =
> 0x25609513, .flags = 0x1, .type = 0x1, from = 0, len = 0 }
> S: 19479@1508268433.381527:nbd_co_receive_request_decode_type Decoding
> type: handle = 93997172956880, type = 1 (write)
> S: 19479@1508268433.381540:nbd_co_receive_request_payload_received
> Payload received: handle = 93997172956880, len = 0
> S: 19479@1508268433.381564:nbd_co_send_structured_error Send structured
> error reply: handle = 93997172956880, error = 1 (EPERM), msg = ''
> C: 19481@1508268433.381622:nbd_receive_structured_reply_chunk Got
> structured reply chunk: { flags = 0x1, type = 32769, handle =
> 93997172956880, length = 6 }
> C: wrote 0/0 bytes at offset 0
> C: 0 bytes, 1 ops; 0.0002 sec (0 bytes/sec and 4291.8455 ops/sec)
> 
> Oops - the client claimed success, even though the server replied with
> EPERM.  And the server didn't do a good job of including details on the
> error message.  So there's still some tweaks needed.

Okay, I found that issue:

> +static int nbd_parse_error_payload(NBDStructuredReplyChunk *chunk,
> +                                   uint8_t *payload, int *request_ret,
> +                                   Error **errp)
> +{
> +    uint32_t error;
> +    uint16_t message_size;
> +
> +    assert(chunk->type & (1 << 15));
> +
> +    if (chunk->length < sizeof(error) + sizeof(message_size)) {
> +        error_setg(errp,
> +                   "Protocol error: invalid payload for structured error");
> +        return -EINVAL;
> +    }
> +
> +    error = nbd_errno_to_system_errno(payload_advance32(&payload));
> +    if (error == 0) {
> +        error_setg(errp, "Protocol error: server sent structured error chunk"
> +                         "with error = 0");
> +        return -EINVAL;
> +    }
> +
> +    *request_ret = error;

Here, you set *request_ret to a positive value when the server gives an
error,

> +static coroutine_fn int nbd_co_do_receive_one_chunk(
> +        NBDClientSession *s, uint64_t handle, bool only_structured,
> +        int *request_ret, QEMUIOVector *qiov, void **payload, Error **errp)
>  {

> -    } else {
> -        assert(s->reply.handle == handle);
> -        ret = -nbd_errno_to_system_errno(s->reply.simple.error);
> -        if (qiov && ret == 0) {
> -            if (qio_channel_readv_all(s->ioc, qiov->iov, qiov->niov,
> -                                      NULL) < 0) {
> -                ret = -EIO;
> -                s->quit = true;
> +        error_setg(errp, "Connection closed");
> +        return -EIO;
> +    }
> +
> +    assert(s->reply.handle == handle);
> +
> +    if (nbd_reply_is_simple(&s->reply)) {
> +        if (only_structured) {
> +            error_setg(errp, "Protocol error: simple reply when structured"
> +                             "reply chunk was expected");
> +            return -EINVAL;
> +        }
> +
> +        *request_ret = -nbd_errno_to_system_errno(s->reply.simple.error);

But here, you set it to a negative value,

> +/* nbd_reply_chunk_iter_receive
> + * The pointer stored in @payload requires qemu_vfree() to free it.
> + */
> +static bool nbd_reply_chunk_iter_receive(NBDClientSession *s,
> +                                         NBDReplyChunkIter *iter,
> +                                         uint64_t handle,
> +                                         QEMUIOVector *qiov, NBDReply *reply,
> +                                         void **payload)
> +{
> +    int ret;
> +    NBDReply local_reply;
> +    NBDStructuredReplyChunk *chunk;
> +    Error *local_err = NULL;
> +    if (s->quit) {
> +        error_setg(&local_err, "Connection closed");
> +        nbd_iter_error(iter, true, -EIO, &local_err);
> +        goto break_loop;
> +    }
> +
> +    if (iter->done) {
> +        /* Previous iteration was last. */
> +        goto break_loop;
> +    }
> +
> +    if (reply == NULL) {
> +        reply = &local_reply;
> +    }
> +
> +    ret = nbd_co_receive_one_chunk(s, handle, iter->only_structured,
> +                                   qiov, reply, payload, &local_err);
> +    if (ret < 0) {
> +        /* If it is a fatal error s->quit is set by nbd_co_receive_one_chunk */
> +        nbd_iter_error(iter, s->quit, ret, &local_err);
> +    }

and you only ever set iter.ret to non-zero if the value was negative (so
you were missing all errors sent through a structured reply).

There was a lot of back-and-forth hunting through the code to see where
errors flow.  I wonder if our intermediate processing should try harder
to distinguish between NBD errors sent by the server (but the connection
is still good - aka not a fatal error for the receive loop but does
affect the end client) vs. those detected locally (server sent garbage
or our connection failed, either way, the error is fatal and we won't
ever talk to the server again).

-- 
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 --]