Re: [Qemu-devel] [PATCH 2/6 for-3.1] nbd: stop waiting for a NBD response with NBD_CMD_DISC

[Eric Blake]

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.

Does that depend on whether the server tried to gracefully close the TLS 
connection (and flushed enough packets to do so)?  At any rate, while 
it's nice to wait for the server to cleanly end TLS, we can't control 
the behavior of all servers, and I agree with your analysis that having 
the server silently deal with an abrupt shutdown is nicer than polluting 
stderr, as it's not actually a data loss situation for either the client 
or the server.

> 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

s/supress/suppress/

> message. This fixes a regression introduced in
>
>    commit be41c100c0d67f6072ddd4910c4b6f7d239f025f
>    Author: Vladimir Sementsov-Ogievskiy <address@hidden>
>    Date:   Fri May 26 14:09:13 2017 +0300
>
>      nbd/client.c: use errp instead of LOG
>
>      Move to modern errp scheme from just LOGging errors.
>
>      Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>      Message-Id: <address@hidden>
>      Signed-off-by: Paolo Bonzini <address@hidden>
>
> Signed-off-by: Daniel P. Berrangé <address@hidden>
> ---
>   block/nbd-client.c | 1 +
>   1 file changed, 1 insertion(+)

Reviewed-by: Eric Blake <address@hidden>

Agree that this is 3.1 material; will queue.

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