8:50 p.m.
We've previously documented that nbd_set_strict() can add new bits,
defaulting to on to provide client-side safety, but which can be
overridden when performing specific integration tests against a
server. Prior to the introduction of libnbd support for extended
headers, a user could manually disable STRICT_FLAGS and
STRICT_COMMANDS to test the response of an older server receiving an
unexpected NBD_CMD_FLAG_PAYLOAD_LEN; but for convenience sake, we
prefer our default for that flag to now track extended headers and
ignore what the user passes in, which removes the ability we had for
that integration test. Adding a new strictness knob lets the user be
in charge of that bit's contents once again. The caveat remains that
disabling the strictness bits makes it possible for a client to
violate the NBD protocol which may have undefined results (the
connection may drop, libnbd may hang, ...), so most clients will never
call nbd_set_strict() in the first place.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
v4: new patch
---
generator/API.ml | 32 +++++++++++++++++++++++++-------
lib/rw.c | 46 +++++++++++++++++++++++++++++-----------------
2 files changed, 54 insertions(+), 24 deletions(-)
diff --git a/generator/API.ml b/generator/API.ml
index 2e8a33bd..4d1021b4 100644
--- a/generator/API.ml
+++ b/generator/API.ml
@@ -234,6 +234,7 @@ let strict_flags =
"ZERO_SIZE", 1 lsl 3;
"ALIGN", 1 lsl 4;
"PAYLOAD", 1 lsl 5;
+ "AUTO_FLAG", 1 lsl 6;
]
}
let allow_transport_flags = {
@@ -1138,7 +1139,8 @@ "set_strict_mode", {
Flags that are known by libnbd as associated with a given command
(such as C<LIBNBD_CMD_FLAG_DF> for L<nbd_pread_structured(3)> gated
by L<nbd_can_df(3)>) are controlled by C<LIBNBD_STRICT_COMMANDS>
-instead.
+instead; and C<LIBNBD_CMD_FLAG_PAYLOAD_LEN> is managed automatically
+by libnbd unless C<LIBNBD_STRICT_AUTO_FLAG> is disabled.
Note that the NBD protocol only supports 16 bits of command flags,
even though the libnbd API uses C<uint32_t>; bits outside of the
@@ -1175,6 +1177,19 @@ "set_strict_mode", {
requests larger than 32MiB are liable to cause some servers to
disconnect.
+=item C<LIBNBD_STRICT_AUTO_FLAG> = 0x40
+
+If set, commands that accept the C<LIBNBD_CMD_FLAG_PAYLOAD_LEN>
+flag (such as L<nbd_pwrite(3)> and C<nbd_block_status_filter(3)>)
+ignore the presence or absence of that flag from the caller,
+instead sending the value over the wire that matches the
+server's expectations based on whether extended headers were
+negotiated when the connection was made. If clear, the caller
+takes on the responsibility for whether the payload length
+flag is set or clear during the affected command, which can
+be useful during integration testing but is more likely to
+lead to undefined behavior.
+
=back
For convenience, the constant C<LIBNBD_STRICT_MASK> is available to
@@ -2682,10 +2697,11 @@ "pwrite", {
C<LIBNBD_CMD_FLAG_FUA> meaning that the server should not
return until the data has been committed to permanent storage
(if that is supported - some servers cannot do this, see
-L<nbd_can_fua(3)>). For convenience, libnbd ignores the presence
-or absence of the flag C<LIBNBD_CMD_FLAG_PAYLOAD_LEN> in C<flags>,
-while correctly using the flag over the wire according to whether
-extended headers were negotiated."
+L<nbd_can_fua(3)>). For convenience, unless C<nbd_set_strict_flags(3)>
+was used to disable C<LIBNBD_STRICT_AUTO_FLAG>, libnbd ignores the
+presence or absence of the flag C<LIBNBD_CMD_FLAG_PAYLOAD_LEN>
+in C<flags>, while correctly using the flag over the wire
+according to whether extended headers were negotiated."
^ strict_call_description;
see_also = [Link "can_fua"; Link "is_read_only";
Link "aio_pwrite"; Link "get_block_size";
@@ -3025,8 +3041,10 @@ "block_status_filter", {
All other parameters to this function have the same semantics
as in L<nbd_block_status_64(3)>; except that for convenience,
-the C<flags> parameter may additionally contain or omit
-C<LIBNBD_CMD_FLAG_PAYLOAD_LEN>."
+unless <nbd_set_strict_flags(3)> was used to disable
+C<LIBNBD_STRICT_AUTO_FLAG>, libnbd ignores the presence or
+absence of the flag C<LIBNBD_CMD_FLAG_PAYLOAD_LEN>
+in C<flags>, while correctly using the flag over the wire."
^ strict_call_description;
see_also = [Link "block_status_64";
Link "can_block_status_payload"; Link
"can_meta_context";
diff --git a/lib/rw.c b/lib/rw.c
index 87bcb1b6..2a849d21 100644
--- a/lib/rw.c
+++ b/lib/rw.c
@@ -406,6 +406,15 @@ nbd_unlocked_aio_pwrite (struct nbd_handle *h, const void *buf,
{
struct command_cb cb = { .completion = *completion };
+ if (h->strict & LIBNBD_STRICT_AUTO_FLAG) {
+ /* It is more convenient to manage PAYLOAD_LEN by what was negotiated
+ * than to require the user to have to set it correctly.
+ */
+ if (h->extended_headers)
+ flags |= LIBNBD_CMD_FLAG_PAYLOAD_LEN;
+ else
+ flags &= ~LIBNBD_CMD_FLAG_PAYLOAD_LEN;
+ }
if (h->strict & LIBNBD_STRICT_COMMANDS) {
if (nbd_unlocked_is_read_only (h) == 1) {
set_error (EPERM, "server does not support write operations");
@@ -417,16 +426,12 @@ nbd_unlocked_aio_pwrite (struct nbd_handle *h, const void *buf,
set_error (EINVAL, "server does not support the FUA flag");
return -1;
}
+
+ if (!!(flags & LIBNBD_CMD_FLAG_PAYLOAD_LEN) != h->extended_headers) {
+ set_error (EINVAL, "incorrect setting for PAYLOAD_LEN flag");
+ return -1;
+ }
}
- /* It is more convenient to manage PAYLOAD_LEN by what was negotiated
- * than to require the user to have to set it correctly.
- * TODO: Add new h->strict bit to allow intentional protocol violation
- * for interoperability testing.
- */
- if (h->extended_headers)
- flags |= LIBNBD_CMD_FLAG_PAYLOAD_LEN;
- else
- flags &= ~LIBNBD_CMD_FLAG_PAYLOAD_LEN;
SET_CALLBACK_TO_NULL (*completion);
return nbd_internal_command_common (h, flags, NBD_CMD_WRITE, offset, count,
@@ -615,15 +620,17 @@ nbd_unlocked_aio_block_status_filter (struct nbd_handle *h,
char *name;
size_t i;
- /* Because this affects wire format, it is more convenient to manage
- * PAYLOAD_LEN by what was negotiated than to require the user to
- * have to set it correctly.
- */
- if (!h->extended_headers) {
- set_error (ENOTSUP, "server does not support extended headers");
- return -1;
+ if (h->strict & LIBNBD_STRICT_AUTO_FLAG) {
+ /* Because this affects wire format, it is more convenient to manage
+ * PAYLOAD_LEN by what was negotiated than to require the user to
+ * have to set it correctly.
+ */
+ if (!h->extended_headers) {
+ set_error (ENOTSUP, "server does not support extended headers");
+ return -1;
+ }
+ flags |= LIBNBD_CMD_FLAG_PAYLOAD_LEN;
}
- flags |= LIBNBD_CMD_FLAG_PAYLOAD_LEN;
if (h->strict & LIBNBD_STRICT_COMMANDS) {
if (nbd_unlocked_can_block_status_payload (h) != 1) {
@@ -638,6 +645,11 @@ nbd_unlocked_aio_block_status_filter (struct nbd_handle *h,
"connecting or the server does not support it");
return -1;
}
+
+ if ((flags & LIBNBD_CMD_FLAG_PAYLOAD_LEN) == 0) {
+ set_error (EINVAL, "incorrect setting for PAYLOAD_LEN flag");
+ return -1;
+ }
}
ids = calloc (1, sizeof *ids);
--
2.41.0