2:59 p.m.
nbdkit prior to commit c70616f87c was prone to deadlock on a
bi-directional connection: it would end up stuck in a read() after
getting NBD_CMD_DISC to make sure that the client wasn't erroneously
sending any more requests, at the same time that the client could get
stuck in a read() waiting for EOF from the server responding to
NBD_CMD_DISC. While nbdkit as a server has been fixed, it is possible
that there are other servers out there with the same issue. What's
more, nbdkit 430f814102 demonstrated that clients can be robust to
such servers, by utilizing shutdown() to give the server an early EOF
even while the bi-directional socket remains open. So, libnbd needs
to use the same solution as what nbdkit had done as a client.
We need to wire up a couple of new states in order to call the
just-added .shut_writes hook as needed (in practice, a client won't be
calling nbd_aio_disconnect while there are still pending earlier
requests, and thus we are unlikely to hit the case where writes
performed during gnutls_bye() actually block; but the logic to support
this is easy to copy from what we already have in place for
NBD_CMD_WRITE).
---
lib/internal.h | 1 +
generator/state_machine.ml | 19 +++++++++++++++++--
generator/states-issue-command.c | 20 ++++++++++++++++++--
3 files changed, 36 insertions(+), 4 deletions(-)
diff --git a/lib/internal.h b/lib/internal.h
index 0d02687..c99c3e7 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -192,6 +192,7 @@ struct nbd_handle {
*/
struct nbd_request request;
bool in_write_payload;
+ bool in_write_shutdown;
/* When connecting, this stores the socket address. */
struct sockaddr_storage connaddr;
diff --git a/generator/state_machine.ml b/generator/state_machine.ml
index 6c7f8bd..bd65ffb 100644
--- a/generator/state_machine.ml
+++ b/generator/state_machine.ml
@@ -602,14 +602,29 @@ and issue_command_state_machine = [
NotifyRead, "PAUSE_WRITE_PAYLOAD" ];
};
-State {
+ State {
default_state with
name = "PAUSE_WRITE_PAYLOAD";
comment = "Interrupt write payload to receive an earlier command's
reply";
external_events = [];
};
-State {
+ State {
+ default_state with
+ name = "SEND_WRITE_SHUTDOWN";
+ comment = "Sending write shutdown notification to the remote server";
+ external_events = [ NotifyWrite, "";
+ NotifyRead, "PAUSE_WRITE_SHUTDOWN" ];
+ };
+
+ State {
+ default_state with
+ name = "PAUSE_WRITE_SHUTDOWN";
+ comment = "Interrupt write shutdown to receive an earlier command's
reply";
+ external_events = [];
+ };
+
+ State {
default_state with
name = "FINISH";
comment = "Finish issuing a command";
diff --git a/generator/states-issue-command.c b/generator/states-issue-command.c
index bd9946d..a810114 100644
--- a/generator/states-issue-command.c
+++ b/generator/states-issue-command.c
@@ -1,5 +1,5 @@
/* nbd client library in userspace: state machine
- * Copyright (C) 2013-2019 Red Hat Inc.
+ * Copyright (C) 2013-2020 Red Hat Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -31,7 +31,9 @@ STATE_MACHINE {
* writes yet; we want to advance back to the correct state but
* without trying a send_from_wbuf that will likely return 1.
*/
- if (h->wlen) {
+ if (h->in_write_shutdown)
+ SET_NEXT_STATE_AND_BLOCK (%SEND_WRITE_SHUTDOWN);
+ else if (h->wlen) {
if (h->in_write_payload)
SET_NEXT_STATE_AND_BLOCK (%SEND_WRITE_PAYLOAD);
else
@@ -79,6 +81,10 @@ STATE_MACHINE {
h->wflags = MSG_MORE;
SET_NEXT_STATE (%SEND_WRITE_PAYLOAD);
}
+ else if (cmd->type == NBD_CMD_DISC) {
+ h->in_write_shutdown = true;
+ SET_NEXT_STATE (%SEND_WRITE_SHUTDOWN);
+ }
else
SET_NEXT_STATE (%FINISH);
return 0;
@@ -97,6 +103,16 @@ STATE_MACHINE {
SET_NEXT_STATE (%^REPLY.START);
return 0;
+ ISSUE_COMMAND.SEND_WRITE_SHUTDOWN:
+ if (h->sock->ops->shut_writes (h, h->sock))
+ SET_NEXT_STATE (%FINISH);
+ return 0;
+
+ ISSUE_COMMAND.PAUSE_WRITE_SHUTDOWN:
+ assert (h->in_write_shutdown);
+ SET_NEXT_STATE (%^REPLY.START);
+ return 0;
+
ISSUE_COMMAND.FINISH:
struct command *cmd;
--
2.26.0.rc2