On Sat, Nov 17, 2018 at 08:19:10PM -0600, Eric Blake wrote:
> On 11/16/18 9:53 AM, Daniel P. Berrangé wrote:
> > When sending a NBD_CMD_DISC message there is no reply expected,
> > however, the nbd_read_eof() coroutine is still waiting for a reply.
> > In a plain NBD connection this doesn't matter as it will just get an
> > EOF, however, on a TLS connection it will get an interrupted TLS data
> > packet. The nbd_read_eof() will then print an error message on the
> > console due to missing reply to NBD_CMD_DISC.
> >
> > This can be seen with qemu-img
> >
> > $ qemu-img info \
> > --object tls-creds-x509,dir=tlsdir,id=tls0,endpoint=client \
> > --image-opts driver=nbd,host=127.0.0.1,port=9000,tls-creds=tls0
> > qemu-img: Cannot read from TLS channel: Input/output error
> > image: nbd://127.0.0.1:9000
> > file format: nbd
> > virtual size: 10M (10485760 bytes)
> > disk size: unavailable
> >
> > Simply setting the 'quit' flag after sending NBD_CMD_DISC is enough to
> > get the coroutine to stop waiting for a reply and thus supress the error
> > message.
>
> Actually, it's not quite enough - once you actually start performing I/O,
> enough coroutines are kicked off that the error still happens:
Hmm, does the client not send requests synchronously ? I expected that
any other I/O operations would have already received their reply by the
time we sent the DISC command.
>
> $ qemu-io -c 'r 1m 1m' -c 'w -P 0x22 1m 1m' --image-opts \
> --object tls-creds-x509,dir=scratch/tls/client1,endpoint=client,id=tls0\
> driver=nbd,host=localhost,port=10809,tls-creds=tls0
> read 1048576/1048576 bytes at offset 1048576
> 1 MiB, 1 ops; 0.0430 sec (23.204 MiB/sec and 23.2040 ops/sec)
> wrote 1048576/1048576 bytes at offset 1048576
> 1 MiB, 1 ops; 0.0152 sec (65.479 MiB/sec and 65.4793 ops/sec)
> Cannot read from TLS channel: Input/output error
>
> Squashing this in on top of your patch helps, though:
>
> diff --git i/block/nbd-client.c w/block/nbd-client.c
> index 5f63e4b8f15..e7916c78996 100644
> --- i/block/nbd-client.c
> +++ w/block/nbd-client.c
> @@ -79,7 +79,14 @@ static coroutine_fn void nbd_read_reply_entry(void
> *opaque)
> assert(s->reply.handle == 0);
> ret = nbd_receive_reply(s->ioc, &s->reply, &local_err);
> if (local_err) {
> - error_report_err(local_err);
> + /* If we are already quitting, either another error has
> + * already been reported, or we requested NBD_CMD_DISC and
> + * don't need to report anything further. */
> + if (!s->quit) {
> + error_report_err(local_err);
> + } else {
> + error_free(local_err);
> + }
> }
> if (ret <= 0) {
> break;
>
> But I want to do more testing to make sure I'm not missing out on reporting
> an actual error if I add that.
Yes, I'd be a little concerned about missing reporting of an error from
a command other than NBD_CMD_DISC if we did this.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
