8:02 a.m.
On 6/20/19 12:49 AM, Richard W.M. Jones wrote:
On Mon, Jun 17, 2019 at 07:07:53PM -0500, Eric Blake wrote:
> The NBD spec requires that a server doing structured reads must not
> succeed unless it covers the entire buffer with reply chunks. In the
> general case, this requires a lot of bookkeeping to check whether
> offsets were non-overlapping and sufficient, and we'd rather defer
> such checking to an optional callback function. But a callback
> function will only be reached per chunk, while we still want to fail
> the overall read if the callback function was never called because the
> server erroneously replied with NBD_REPLY_TYPE_NONE with no other
> chunks instead of an expected NBD_REPLY_TYPE_ERROR*. For this specific
> error case, the bookkeeping is much simpler - we merely track if we've
> seen at least one data chunk.
I guess the implicit assumption here is that count > 0, which IIRC the
NBD protocol demands. It seems like we don't actually check this in
nbd_internal_command_common so would it be worth adding a check there?
The protocol says a 0-length read request is unspecified behavior. I
don't know if nbd_internal_command_common should reject it - a
particular server may have reliable behavior, and thus libnbd should not
prevent a client from doing it. On the other hand, the spec says a
0-length structured read chunk is broken. In qemu-nbd, a 0-length read
is not forbidden, but it results in a mere NBD_REPLY_TYPE_DONE rather
than an NBD_REPLY_TYPE_OFFSET_DATA. Which means this patch should
probably special case a 0 length request to NOT be an error even when
data_seen is false.
We may someday want to add a strict mode (where we do as much as we can
to prevent clients from sending 0-length or out-of-bounds requests, and
do additional sanity checking that server replies are compliant), vs. a
loose mode (where clients can send malformed requests, in order to test
server responses). This is yet one more place to consider gating under
such an option setting.
In any case this patch on its own looks good, so ACK.
Here's the tweak I'm adding before pushing:
diff --git i/generator/states-reply-structured.c
w/generator/states-reply-structured.c
index 6740400..9bde46f 100644
--- i/generator/states-reply-structured.c
+++ w/generator/states-reply-structured.c
@@ -95,6 +95,10 @@
return 0;
}
else if (type == NBD_REPLY_TYPE_OFFSET_DATA) {
+ /* The spec states that 0-length requests are unspecified, but
+ * 0-length replies are broken. Still, it's easy enough to support
+ * them as an extension, so we use < instead of <=.
+ */
if (length < sizeof h->sbuf.sr.payload.offset_data) {
SET_NEXT_STATE (%.DEAD);
set_error (0, "too short length in NBD_REPLY_TYPE_OFFSET_DATA");
@@ -110,9 +110,9 @@
return 0;
}
else if (type == NBD_REPLY_TYPE_OFFSET_HOLE) {
- if (length != 12) {
+ if (length != sizeof h->sbuf.sr.payload.offset_hole) {
SET_NEXT_STATE (%.DEAD);
- set_error (0, "invalid length in NBD_REPLY_TYPE_NONE");
+ set_error (0, "invalid length in NBD_REPLY_TYPE_OFFSET_HOLE");
return -1;
}
h->rbuf = &h->sbuf.sr.payload.offset_hole;
@@ -356,6 +360,10 @@
return -1;
}
+ /* The spec states that 0-length requests are unspecified, but
+ * 0-length replies are broken. Still, it's easy enough to support
+ * them as an extension, and this works even when length == 0.
+ */
memset (cmd->data + offset, 0, length);
SET_NEXT_STATE(%FINISH);
diff --git i/lib/aio.c w/lib/aio.c
index 7fb0fdf..2dcce68 100644
--- i/lib/aio.c
+++ w/lib/aio.c
@@ -73,7 +73,10 @@ nbd_unlocked_aio_command_completed (struct nbd_handle *h,
type = cmd->type;
error = cmd->error;
- if (type == NBD_CMD_READ && !cmd->data_seen && !error)
+ /* The spec states that a 0-length read request is unspecified; but
+ * it is easy enough to treat it as successful as an extension.
+ */
+ if (type == NBD_CMD_READ && !cmd->data_seen && cmd->count
&& !error)
error = EIO;
/* Retire it from the list and free it. */
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org