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: 2322 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>
> ---
> 

> +static int nbd_co_request(BlockDriverState *bs, NBDRequest *request,
> +                          QEMUIOVector *write_qiov)
>  {

> -    return nbd_co_receive_reply(client, request->handle,
> -                                request->type == NBD_CMD_READ ? qiov : NULL);
> +    ret = nbd_co_receive_return_code(client, request->handle, &local_err);
> +    if (ret < 0) {
> +        error_report_err(local_err);
> +    }

I think this new error_report_err() is a regression in behavior.
Running the old server:

$ qemu-nbd -x foo -f qcow2 --trace='nbd_*' file -r

and an old client:

$ qemu-io -f raw nbd://localhost:10809/foo
qemu-io> w 0 0
write failed: Operation not permitted
qemu-io> q

but with the new client (once I fix the bug about being able to ignore
the NBD_REP_ERR_UNSUP with non-zero length in the earlier patch):

$ ./qemu-io -f raw nbd://localhost:10809/foo
qemu-io> w 0 0
Request failed: Operation not permitted
write failed: Operation not permitted
qemu-io>

and worse, new server with new client:

$ ./qemu-io -f raw nbd://localhost:10809/foo
qemu-io> w 0 0
: Operation not permitted
write failed: Operation not permitted
qemu-io>

we don't even manage to post a sane message.

Reporting fatal errors where we lose connection with the server (or
forcefully give up on the server because it violated protocol) may be
okay, but reporting common errors where the server reported a problem
but we are still connected is too verbose.

I know I asked about errp plumbing on v3, but now I'm thinking that it
was a premature request; we either plumb in errp handling without any
new features, or we do the new features in isolation and only later see
if adding errp plumbing makes sense.  Yes, that means undoing some of
the changes you made between v3 and v4, so sorry for the churn it has
caused.

I hope to post a v5 soon with the tweaks I've made after playing with
this version.

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