On Tue, Sep 14, 2021 at 05:40:59PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> 13.09.2021 18:19, Richard W.M. Jones wrote:
> >$ rm -f /tmp/sock /tmp/pid
> >$ qemu-img create -f qcow2 /tmp/disk.qcow2 1M
> >$ qemu-nbd -t --format=qcow2 --socket=/tmp/sock --pid-file=/tmp/pid
> >/tmp/disk.qcow2 &
> >$ nbdsh -u 'nbd+unix:///?socket=/tmp/sock' -c 'h.get_size()'
> >qemu-nbd: Disconnect client, due to: Failed to send reply: Unable to write
> >to socket: Broken pipe
> >$ killall qemu-nbd
> >
> >nbdsh is abruptly dropping the NBD connection here which is a valid
> >way to close the connection. It seems unnecessary to print an error
> >in this case so this commit suppresses it.
> >
> >Note that if you call the nbdsh h.shutdown() method then the message
> >was not printed:
> >
> >$ nbdsh -u 'nbd+unix:///?socket=/tmp/sock' -c 'h.get_size()' -c
> >'h.shutdown()'
>
> My personal opinion, is that this warning doesn't hurt in general. I
> think in production tools should gracefully shutdown any connection,
> and abrupt shutdown is a sign of something wrong - i.e., worth
> warning.
>
> Shouldn't nbdsh do graceful shutdown by default?
On the client side the only difference is that nbd_shutdown sends
NBD_CMD_DISC to the server (versus simply closing the socket). On the
server side when the server receives NBD_CMD_DISC it must complete any
in-flight requests, but there's no requirement for the server to
commit anything to disk. IOW you can still lose data even though you
took the time to disconnect.
So I don't think there's any reason for libnbd to always gracefully
shut down (especially in this case where there are no in-flight
requests), and anyway it would break ABI to make that change and slow
down the client in cases when there's nothing to clean up.
> >Signed-off-by: Richard W.M. Jones <rjo...@redhat.com>
> >---
> > nbd/server.c | 7 ++++++-
> > 1 file changed, 6 insertions(+), 1 deletion(-)
> >
> >diff --git a/nbd/server.c b/nbd/server.c
> >index 3927f7789d..e0a43802b2 100644
> >--- a/nbd/server.c
> >+++ b/nbd/server.c
> >@@ -2669,7 +2669,12 @@ static coroutine_fn void nbd_trip(void *opaque)
> > ret = nbd_handle_request(client, &request, req->data, &local_err);
> > }
> > if (ret < 0) {
> >- error_prepend(&local_err, "Failed to send reply: ");
> >+ if (errno != EPIPE) {
>
> Both nbd_handle_request() and nbd_send_generic_reply() declares that
> they return -errno on failure in communication with client. I think,
> you should use ret here: if (ret != -EPIPE). It's safer: who knows,
> does errno really set on all error paths of called functions? If
> not, we may see here errno of some another previous operation.
Should we set errno = 0 earlier in nbd_trip()? I don't really know
how coroutines in qemu interact with thread-local variables though.
Rich.
> >+ error_prepend(&local_err, "Failed to send reply: ");
> >+ } else {
> >+ error_free(local_err);
> >+ local_err = NULL;
> >+ }
> > goto disconnect;
> > }
> >
>
>
> --
> Best regards,
> Vladimir
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines. Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v
