4:04 p.m.
Another patch in the series of being more tolerant to various server
bugs. Although a server is non-compliant for using a simple reply to
NBD_CMD_READ once structured commands are negotiated, it is just as
easy to read the packet and continue on after guaranteeing we report
an error.
In the process, I noticed that in the corner case of a server that
starts with a structured packet and follows with a simple reply, we
can no longer assume that the simple reply is the only receipt of
data, so we have to be more careful with assertions and increments to
data_seen.
As in earlier patches, the following temporary one-line tweak to
nbdkit will produce a server that demonstrates the scenario:
| diff --git i/server/protocol.c w/server/protocol.c
| index 2ac77055..ac359ca1 100644
| --- i/server/protocol.c
| +++ w/server/protocol.c
| @@ -738,7 +738,7 @@ protocol_recv_request_send_reply (void)
| * us from sending human-readable error messages to the client, so
| * we should reconsider this in future.
| */
| - if (conn->structured_replies &&
| + if (conn->structured_replies && !error &&
| (cmd == NBD_CMD_READ || cmd == NBD_CMD_BLOCK_STATUS)) {
| if (!error) {
| if (cmd == NBD_CMD_READ)
The following command line:
$ ./run /path/to/nbdkit -U - memory 1M --filter=error error-pread-rate=100% \
--run 'nbdsh -u "$uri" -c "\
try:
h.pread(1,0)
except nbd.Error as ex:
print(ex.string)
h.flush()
print(h.get_size())
"'
demonstrates the following issue pre-patch:
nbdkit: memory.0: error: injecting EIO error into pread
nbd_pread: server sent unexpected simple reply for read
nbdsh: command line script failed: nbd_flush: invalid state: DEAD: the handle must be
connected with the server: Invalid argument
and the nicer flow post-patch:
nbdkit: memory.0: error: injecting EIO error into pread
nbd_pread: read: command failed: Protocol error
1048576
where we report EPROTO rather than the server's EIO.
---
generator/states-reply-simple.c | 21 +++++++++++++--------
1 file changed, 13 insertions(+), 8 deletions(-)
diff --git a/generator/states-reply-simple.c b/generator/states-reply-simple.c
index 24f7efa..ac58823 100644
--- a/generator/states-reply-simple.c
+++ b/generator/states-reply-simple.c
@@ -49,17 +49,23 @@ STATE_MACHINE {
return 0;
}
+ /* Although a server with structured replies negotiated is in error
+ * for using a simple reply to NBD_CMD_READ, we can cope with the
+ * packet, but diagnose it by failing the read with EPROTO.
+ */
if (cmd->type == NBD_CMD_READ && h->structured_replies) {
- set_error (0, "server sent unexpected simple reply for read");
- SET_NEXT_STATE(%.DEAD);
- return 0;
+ debug (h, "server sent unexpected simple reply for read");
+ if (cmd->error == 0)
+ cmd->error = EPROTO;
}
- cmd->error = nbd_internal_errno_of_nbd_error (error);
- if (cmd->error == 0 && cmd->type == NBD_CMD_READ) {
+ error = nbd_internal_errno_of_nbd_error (error);
+ if (cmd->error == 0)
+ cmd->error = error;
+ if (error == 0 && cmd->type == NBD_CMD_READ) {
h->rbuf = cmd->data;
h->rlen = cmd->count;
- cmd->data_seen = cmd->count;
+ cmd->data_seen += cmd->count;
SET_NEXT_STATE (%RECV_READ_PAYLOAD);
}
else {
@@ -80,9 +86,8 @@ STATE_MACHINE {
/* guaranteed by START */
assert (cmd);
if (CALLBACK_IS_NOT_NULL (cmd->cb.fn.chunk)) {
- int error = 0;
+ int error = cmd->error;
- assert (cmd->error == 0);
if (CALL_CALLBACK (cmd->cb.fn.chunk,
cmd->data, cmd->count,
cmd->offset, LIBNBD_READ_DATA,
--
2.37.1