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

[Vladimir Sementsov-Ogievskiy]

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?

> Signed-off-by: Richard W.M. Jones <rjones@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.

> +            error_prepend(&local_err, "Failed to send reply: ");
> +        } else {
> +            error_free(local_err);
> +            local_err = NULL;
> +        }
>           goto disconnect;
>       }
>   


--
Best regards,
Vladimir