11:48 p.m.
If a plugin's .open or .get_size or .can_write fails, right now that
is fatal to the connection. When nbdkit was first implemented, this
made sense (there was no way to report errors to oldstyle or
NBD_OPT_EXPORT_NAME). But now that newstyle is around, it's rather
abrupt to hang up on the client, and better is to return an error to
NBD_OPT_GO, and let the client choose what to do (most clients will
probably hang up, whether gracefully with NBD_OPT_ABORT or abruptly,
rather than try other NBD_OPT_*, but _we_ shouldn't be the ones
forcing their hand).
For an example of what this improves, if you run:
$ nbdkit -fv sh - <<\EOF
case $1 in get_size) echo oops >&2; exit 1;; *) exit 2;; esac
EOF
Pre-patch, qemu complains about the abrupt server death:
$ qemu-nbd --list
qemu-nbd: Failed to read option reply: Unexpected end-of-file before all bytes were read
Post-patch, qemu gets the desired error message, and exits gracefully:
$ qemu-nbd --list
qemu-nbd: Requested export not available
Note that this does not fix the pre-existing problem that we can end
up calling .finalize even when .prepare was skipped or failed (that
latent problem was first exposed for the rare client that calls
NBD_OPT_INFO before NBD_OPT_GO, see commit a6b88b19); a later patch
will have to add better bookkeeping for that, and for better handling
of reopen requests from the retry filter.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
server/protocol-handshake-newstyle.c | 32 +++++++++++++++++++++-------
server/protocol-handshake-oldstyle.c | 3 +++
2 files changed, 27 insertions(+), 8 deletions(-)
diff --git a/server/protocol-handshake-newstyle.c b/server/protocol-handshake-newstyle.c
index 2480d7a3..878fe53f 100644
--- a/server/protocol-handshake-newstyle.c
+++ b/server/protocol-handshake-newstyle.c
@@ -198,7 +198,7 @@ conn_recv_full (struct connection *conn, void *buf, size_t len,
/* Sub-function of negotiate_handshake_newstyle_options below. It
* must be called on all non-error paths out of the options for-loop
- * in that function.
+ * in that function, and must not cause any wire traffic.
*/
static int
finish_newstyle_options (struct connection *conn, uint64_t *exportsize)
@@ -322,7 +322,9 @@ negotiate_handshake_newstyle_options (struct connection *conn)
if (check_export_name (conn, option, data, optlen, optlen, true) == -1)
return -1;
- /* We have to finish the handshake by sending handshake_finish. */
+ /* We have to finish the handshake by sending handshake_finish.
+ * On failure, we have to disconnect.
+ */
if (finish_newstyle_options (conn, &exportsize) == -1)
return -1;
@@ -460,16 +462,30 @@ negotiate_handshake_newstyle_options (struct connection *conn)
* or else we drop the support for that context.
*/
if (check_export_name (conn, option, &data[4], exportnamelen,
- optlen - 6, true) == -1)
- return -1;
+ optlen - 6, true) == -1) {
+ if (send_newstyle_option_reply (conn, option, NBD_REP_ERR_INVALID)
+ == -1)
+ return -1;
+ continue;
+ }
/* The spec is confusing, but it is required that we send back
* NBD_INFO_EXPORT, even if the client did not request it!
* qemu client in particular does not request this, but will
- * fail if we don't send it.
+ * fail if we don't send it. Note that if .open fails, but we
+ * succeed at .close, then we merely return an error to the
+ * client and let them try another NBD_OPT, rather than
+ * disconnecting.
*/
- if (finish_newstyle_options (conn, &exportsize) == -1)
- return -1;
+ if (finish_newstyle_options (conn, &exportsize) == -1) {
+ if (backend->finalize (backend, conn) == -1)
+ return -1;
+ backend_close (backend, conn);
+ if (send_newstyle_option_reply (conn, option,
+ NBD_REP_ERR_UNKNOWN) == -1)
+ return -1;
+ continue;
+ }
if (send_newstyle_option_reply_info_export (conn, option,
NBD_REP_INFO,
@@ -497,7 +513,7 @@ negotiate_handshake_newstyle_options (struct connection *conn)
}
/* Unlike NBD_OPT_EXPORT_NAME, NBD_OPT_GO sends back an ACK
- * or ERROR packet.
+ * or ERROR packet. If this was NBD_OPT_LIST, call .close.
*/
if (send_newstyle_option_reply (conn, option, NBD_REP_ACK) == -1)
return -1;
diff --git a/server/protocol-handshake-oldstyle.c b/server/protocol-handshake-oldstyle.c
index 4efc668b..45a1a486 100644
--- a/server/protocol-handshake-oldstyle.c
+++ b/server/protocol-handshake-oldstyle.c
@@ -52,6 +52,9 @@ protocol_handshake_oldstyle (struct connection *conn)
assert (tls != 2); /* Already filtered in main */
+ /* With oldstyle, our only option if .open or friends fail is to
+ * disconnect, as we cannot report the problem to the client.
+ */
if (protocol_common_open (conn, &exportsize, &eflags) == -1)
return -1;
--
2.21.0