Re: [PATCH v2] nbd/server: Suppress Broken pipe errors on abrupt disconnection

[Eric Blake <eblake@redhat.com>]

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?

nbdsh exposes the ability to do graceful shutdown, but does not force
it (it is up to the client software using nbdsh whether it calls the
right APIs for a graceful shutdown).

We might consider a new API (which we'd then expose via a new
command-line option to nbdsh) that requests that libnbd try harder to
send NBD_OPT_ABORT or NBD_CMD_DISC prior to closing, but it would
still be something that end users would have to opt into using, and
not something we can turn on by default.

> > +++ 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.

Correct - 'errno' is indeterminate at this point; the correct check is
if (-ret != EPIPE).  I can make that tweak while taking this patch, if
we decide it is worthwhile.


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