[Libguestfs] [nbdkit PATCH 2/2] nbd: Don't read after client sends NBD_CMD_DISC

[Richard W.M. Jones]

On Wed, Apr 18, 2018 at 09:51:08PM -0500, Eric Blake wrote:
> According to the NBD spec, the server should close the connection
> rather than replying to NBD_CMD_DISC (after flushing any pending
> replies to earlier commands), and a compliant client should not
> send any data after that command.  However, when nbdkit is
> running multithreaded, we already have multiple threads competing
> for a read lock, and each of those threads would try to read()
> from the socket, which will never make progress unless the client
> hangs up so that the read fails with EOF.
> 
> But if the client waits to close its end until after seeing EOF
> from the server (as was the case with the nbd plugin as the client
> until the previous patch), then both sides are deadlocked on a
> read.  We already short-circuit a read attempt when the socket
> is closed; we should likewise avoid a read attempt after the
> client has requested a disconnect, so that we aren't at the
> mercy of the client properly using shutdown().
> 
> Note that this patch in isolation without the previous patch to
> the nbd plugin is sufficient to make the testsuite deadlock go
> away.
> 
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---
>  src/connections.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/src/connections.c b/src/connections.c
> index 46f2cd4..6c1f8cd 100644
> --- a/src/connections.c
> +++ b/src/connections.c
> @@ -1056,8 +1056,9 @@ recv_request_send_reply (struct connection *conn)
>    /* Read the request packet. */
>    {
>      ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&conn->read_lock);
> -    if (get_status (conn) < 0)
> -      return -1;
> +    r = get_status (conn);
> +    if (r <= 0)
> +      return r;
>      r = conn->recv (conn, &request, sizeof request);
>      if (r == -1) {
>        nbdkit_error ("read request: %m");

These look fine thanks, so:

ACK series.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top