[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Libguestfs] [libnbd PATCH v4 4/6] states: Prepare to send 64-bit re
From: |
Laszlo Ersek |
Subject: |
Re: [Libguestfs] [libnbd PATCH v4 4/6] states: Prepare to send 64-bit requests |
Date: |
Mon, 24 Jul 2023 11:55:46 +0200 |
On 7/21/23 18:08, Eric Blake wrote:
> Support sending 64-bit requests if extended headers were negotiated.
> This includes setting NBD_CMD_FLAG_PAYLOAD_LEN any time we send an
> extended NBD_CMD_WRITE; this is such a fundamental part of the
> protocol that for now it is easier to silently ignore whatever value
> the user passes in for that bit in the flags parameter of nbd_pwrite
> regardless of the current settings in set_strict_mode, rather than
> trying to force the user to pass in the correct value to match whether
> extended mode is negotiated. However, when we later add APIs to give
> the user more control for interoperability testing, it may be worth
> adding a new set_strict_mode control knob to explicitly enable the
> client to intentionally violate the protocol (the testsuite added in
> this patch would then be updated to match).
>
> At this point, h->extended_headers is permanently false (we can't
> enable it until all other aspects of the protocol have likewise been
> converted).
>
> Note that struct command.data_seen remains typed as uint32_t despite
> being an accumulator for count values that are now stored in 64 bits;
> this is okay, because we drop the connection if count exceeds
> MAX_REQUEST_SIZE on any one transaction, such that data_seen's use as
> a bounded accumulator can never exceed 2*MAX_REQUEST_SIZE.
>
> Support for using FLAG_PAYLOAD_LEN with NBD_CMD_BLOCK_STATUS is less
> fundamental, and deserves to be in its own patch.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
> ---
>
> v4: fix Makefile indentation [Laszlo], prefer progname over argv[0] in
> test [Laszlo], enhance commit message about command.data_seen
> ---
> lib/internal.h | 10 ++-
> generator/API.ml | 20 +++---
> generator/states-issue-command.c | 29 ++++++---
> generator/states-reply-chunk.c | 2 +-
> lib/rw.c | 17 +++--
> tests/Makefile.am | 4 ++
> tests/pwrite-extended.c | 108 +++++++++++++++++++++++++++++++
> .gitignore | 1 +
> 8 files changed, 165 insertions(+), 26 deletions(-)
> create mode 100644 tests/pwrite-extended.c
>
> diff --git a/lib/internal.h b/lib/internal.h
> index c7793f1f..2e4f987c 100644
> --- a/lib/internal.h
> +++ b/lib/internal.h
> @@ -110,6 +110,9 @@ struct nbd_handle {
> char *tls_username; /* Username, NULL = use current username */
> char *tls_psk_file; /* PSK filename, NULL = no PSK */
>
> + /* Extended headers. */
> + bool extended_headers; /* If we negotiated NBD_OPT_EXTENDED_HEADERS
> */
> +
> /* Desired metadata contexts. */
> bool request_sr;
> bool request_meta;
> @@ -273,7 +276,10 @@ struct nbd_handle {
> /* Issuing a command must use a buffer separate from sbuf, for the
> * case when we interrupt a request to service a reply.
> */
> - struct nbd_request request;
> + union {
> + struct nbd_request compact;
> + struct nbd_request_ext extended;
> + } req;
> bool in_write_payload;
> bool in_write_shutdown;
>
> @@ -378,7 +384,7 @@ struct command {
> uint16_t type;
> uint64_t cookie;
> uint64_t offset;
> - uint32_t count;
> + uint64_t count;
> void *data; /* Buffer for read/write */
> struct command_cb cb;
> bool initialized; /* For read, true if getting a hole may skip memset */
> diff --git a/generator/API.ml b/generator/API.ml
> index 5fcb0e1f..02c1260d 100644
> --- a/generator/API.ml
> +++ b/generator/API.ml
> @@ -198,11 +198,12 @@ let cmd_flags =
> flag_prefix = "CMD_FLAG";
> guard = Some "((h->strict & LIBNBD_STRICT_FLAGS) || flags > UINT16_MAX)";
> flags = [
> - "FUA", 1 lsl 0;
> - "NO_HOLE", 1 lsl 1;
> - "DF", 1 lsl 2;
> - "REQ_ONE", 1 lsl 3;
> - "FAST_ZERO", 1 lsl 4;
> + "FUA", 1 lsl 0;
> + "NO_HOLE", 1 lsl 1;
> + "DF", 1 lsl 2;
> + "REQ_ONE", 1 lsl 3;
> + "FAST_ZERO", 1 lsl 4;
> + "PAYLOAD_LEN", 1 lsl 5;
> ]
> }
> let handshake_flags = {
> @@ -2507,7 +2508,7 @@ "pread_structured", {
> "pwrite", {
> default_call with
> args = [ BytesIn ("buf", "count"); UInt64 "offset" ];
> - optargs = [ OFlags ("flags", cmd_flags, Some ["FUA"]) ];
> + optargs = [ OFlags ("flags", cmd_flags, Some ["FUA"; "PAYLOAD_LEN"]) ];
> ret = RErr;
> permitted_states = [ Connected ];
> shortdesc = "write to the NBD server";
> @@ -2530,7 +2531,10 @@ "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)>)."
> +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."
> ^ strict_call_description;
> see_also = [Link "can_fua"; Link "is_read_only";
> Link "aio_pwrite"; Link "get_block_size";
> @@ -3220,7 +3224,7 @@ "aio_pwrite", {
> default_call with
> args = [ BytesPersistIn ("buf", "count"); UInt64 "offset" ];
> optargs = [ OClosure completion_closure;
> - OFlags ("flags", cmd_flags, Some ["FUA"]) ];
> + OFlags ("flags", cmd_flags, Some ["FUA"; "PAYLOAD_LEN"]) ];
> ret = RCookie;
> permitted_states = [ Connected ];
> shortdesc = "write to the NBD server";
> diff --git a/generator/states-issue-command.c
> b/generator/states-issue-command.c
> index 30721946..59b05835 100644
> --- a/generator/states-issue-command.c
> +++ b/generator/states-issue-command.c
> @@ -41,15 +41,24 @@ ISSUE_COMMAND.START:
> return 0;
> }
>
> - h->request.magic = htobe32 (NBD_REQUEST_MAGIC);
> - h->request.flags = htobe16 (cmd->flags);
> - h->request.type = htobe16 (cmd->type);
> - h->request.cookie = htobe64 (cmd->cookie);
> - h->request.offset = htobe64 (cmd->offset);
> - h->request.count = htobe32 (cmd->count);
> + /* These fields are coincident between req.compact and req.extended */
> + h->req.compact.flags = htobe16 (cmd->flags);
> + h->req.compact.type = htobe16 (cmd->type);
> + h->req.compact.cookie = htobe64 (cmd->cookie);
> + h->req.compact.offset = htobe64 (cmd->offset);
> + if (h->extended_headers) {
> + h->req.extended.magic = htobe32 (NBD_EXTENDED_REQUEST_MAGIC);
> + h->req.extended.count = htobe64 (cmd->count);
> + h->wlen = sizeof (h->req.extended);
> + }
> + else {
> + assert (cmd->count <= UINT32_MAX);
> + h->req.compact.magic = htobe32 (NBD_REQUEST_MAGIC);
> + h->req.compact.count = htobe32 (cmd->count);
> + h->wlen = sizeof (h->req.compact);
> + }
> h->chunks_sent++;
> - h->wbuf = &h->request;
> - h->wlen = sizeof (h->request);
> + h->wbuf = &h->req;
> if (cmd->type == NBD_CMD_WRITE || cmd->next)
> h->wflags = MSG_MORE;
> SET_NEXT_STATE (%SEND_REQUEST);
> @@ -74,7 +83,7 @@ ISSUE_COMMAND.PREPARE_WRITE_PAYLOAD:
>
> assert (h->cmds_to_issue != NULL);
> cmd = h->cmds_to_issue;
> - assert (cmd->cookie == be64toh (h->request.cookie));
> + assert (cmd->cookie == be64toh (h->req.compact.cookie));
> if (cmd->type == NBD_CMD_WRITE) {
> h->wbuf = cmd->data;
> h->wlen = cmd->count;
> @@ -120,7 +129,7 @@ ISSUE_COMMAND.FINISH:
> assert (!h->wlen);
> assert (h->cmds_to_issue != NULL);
> cmd = h->cmds_to_issue;
> - assert (cmd->cookie == be64toh (h->request.cookie));
> + assert (cmd->cookie == be64toh (h->req.compact.cookie));
> h->cmds_to_issue = cmd->next;
> if (h->cmds_to_issue_tail == cmd)
> h->cmds_to_issue_tail = NULL;
> diff --git a/generator/states-reply-chunk.c b/generator/states-reply-chunk.c
> index 0d76c159..1758ac34 100644
> --- a/generator/states-reply-chunk.c
> +++ b/generator/states-reply-chunk.c
> @@ -34,7 +34,7 @@ structured_reply_in_bounds (uint64_t offset, uint32_t
> length,
> offset + length > cmd->offset + cmd->count) {
> set_error (0, "range of structured reply is out of bounds, "
> "offset=%" PRIu64 ", cmd->offset=%" PRIu64 ", "
> - "length=%" PRIu32 ", cmd->count=%" PRIu32 ": "
> + "length=%" PRIu32 ", cmd->count=%" PRIu64 ": "
> "this is likely to be a bug in the NBD server",
> offset, cmd->offset, length, cmd->count);
> return false;
> diff --git a/lib/rw.c b/lib/rw.c
> index 3dc3499e..8b2bd4cc 100644
> --- a/lib/rw.c
> +++ b/lib/rw.c
> @@ -223,13 +223,11 @@ nbd_internal_command_common (struct nbd_handle *h,
> }
> break;
>
> - /* Other commands are currently limited by the 32 bit field in the
> - * command structure on the wire, but in future we hope to support
> - * 64 bit values here with a change to the NBD protocol which is
> - * being discussed upstream.
> + /* Other commands are limited by the 32 bit field in the command
> + * structure on the wire, unless extended headers were negotiated.
> */
> default:
> - if (count > UINT32_MAX) {
> + if (!h->extended_headers && count > UINT32_MAX) {
> set_error (ERANGE, "request too large: maximum request size is %"
> PRIu32,
> UINT32_MAX);
> goto err;
> @@ -358,6 +356,15 @@ nbd_unlocked_aio_pwrite (struct nbd_handle *h, const
> void *buf,
> 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,
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index 6eddcb7a..52fadd9c 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -233,6 +233,7 @@ check_PROGRAMS += \
> closure-lifetimes \
> pread-initialize \
> socket-activation-name \
> + pwrite-extended \
> $(NULL)
>
> TESTS += \
> @@ -655,6 +656,9 @@ socket_activation_name_SOURCES = \
> requires.h
> socket_activation_name_LDADD = $(top_builddir)/lib/libnbd.la
>
> +pwrite_extended_SOURCES = pwrite-extended.c
> +pwrite_extended_LDADD = $(top_builddir)/lib/libnbd.la
> +
> #----------------------------------------------------------------------
> # Testing TLS support.
>
> diff --git a/tests/pwrite-extended.c b/tests/pwrite-extended.c
> new file mode 100644
> index 00000000..c6f4dd29
> --- /dev/null
> +++ b/tests/pwrite-extended.c
> @@ -0,0 +1,108 @@
> +/* NBD client library in userspace
> + * Copyright Red Hat
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301
> USA
> + */
> +
> +/* Check behavior of pwrite with PAYLOAD_LEN flag for extended headers. */
> +
> +#include <config.h>
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <errno.h>
> +#include <unistd.h>
> +#include <sys/stat.h>
> +
> +#include <libnbd.h>
> +
> +static char *progname;
> +static char buf[512];
> +
> +static void
> +check (int experr, const char *prefix)
> +{
> + const char *msg = nbd_get_error ();
> + int errnum = nbd_get_errno ();
> +
> + fprintf (stderr, "error: \"%s\"\n", msg);
> + fprintf (stderr, "errno: %d (%s)\n", errnum, strerror (errnum));
> + if (strncmp (msg, prefix, strlen (prefix)) != 0) {
> + fprintf (stderr, "%s: test failed: missing context prefix: %s\n",
> + progname, msg);
> + exit (EXIT_FAILURE);
> + }
> + if (errnum != experr) {
> + fprintf (stderr, "%s: test failed: "
> + "expected errno = %d (%s), but got %d\n",
> + progname, experr, strerror (experr), errnum);
> + exit (EXIT_FAILURE);
> + }
> +}
> +
> +int
> +main (int argc, char *argv[])
> +{
> + struct nbd_handle *nbd;
> + const char *cmd[] = {
> + "nbdkit", "-s", "-v", "--exit-with-parent", "memory", "1048576", NULL
> + };
> + uint32_t strict;
> +
> + progname = argv[0];
> +
> + nbd = nbd_create ();
> + if (nbd == NULL) {
> + fprintf (stderr, "%s: %s\n", progname, nbd_get_error ());
> + exit (EXIT_FAILURE);
> + }
> +
> + /* Connect to the server. */
> + if (nbd_connect_command (nbd, (char **)cmd) == -1) {
> + fprintf (stderr, "%s: %s\n", progname, nbd_get_error ());
> + exit (EXIT_FAILURE);
> + }
> +
> + /* FIXME: future API addition to test if server negotiated extended mode.
> + * Until then, strict flags must ignore the PAYLOAD_LEN flag for pwrite,
> + * even though it is rejected for other commands.
> + */
> + strict = nbd_get_strict_mode (nbd);
> + if (!(strict & LIBNBD_STRICT_FLAGS)) {
> + fprintf (stderr, "%s: test failed: "
> + "nbd_get_strict_mode did not have expected flag set\n",
> + progname);
> + exit (EXIT_FAILURE);
> + }
> + if (nbd_aio_pread (nbd, buf, 512, 0, NBD_NULL_COMPLETION,
> + LIBNBD_CMD_FLAG_PAYLOAD_LEN) != -1) {
> + fprintf (stderr, "%s: test failed: "
> + "nbd_aio_pread did not fail with unexpected flag\n",
> + progname);
> + exit (EXIT_FAILURE);
> + }
> + check (EINVAL, "nbd_aio_pread: ");
> +
> + if (nbd_aio_pwrite (nbd, buf, 512, 0, NBD_NULL_COMPLETION,
> + LIBNBD_CMD_FLAG_PAYLOAD_LEN) == -1 ||
> + nbd_aio_pwrite (nbd, buf, 512, 0, NBD_NULL_COMPLETION, 0) == -1) {
> + fprintf (stderr, "%s: %s\n", progname, nbd_get_error ());
> + exit (EXIT_FAILURE);
> + }
> +
> + nbd_close (nbd);
> + exit (EXIT_SUCCESS);
> +}
> diff --git a/.gitignore b/.gitignore
> index b43e83c0..e5304f16 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -252,6 +252,7 @@ Makefile.in
> /tests/pki/
> /tests/pread-initialize
> /tests/private-data
> +/tests/pwrite-extended
> /tests/read-only-flag
> /tests/read-write-flag
> /tests/server-death
Acked-by: Laszlo Ersek <lersek@redhat.com>
- [libnbd PATCH v4 0/6] NBD 64-bit extensions (libnbd portions, prep work), Eric Blake, 2023/07/21
- [libnbd PATCH v4 2/6] block_status: Refactor array storage, Eric Blake, 2023/07/21
- [libnbd PATCH v4 1/6] internal: Track chunk payload length left, Eric Blake, 2023/07/21
- [libnbd PATCH v4 3/6] protocol: Add definitions for extended headers, Eric Blake, 2023/07/21
- [libnbd PATCH v4 4/6] states: Prepare to send 64-bit requests, Eric Blake, 2023/07/21
- Re: [Libguestfs] [libnbd PATCH v4 4/6] states: Prepare to send 64-bit requests,
Laszlo Ersek <=
- [libnbd PATCH v4 5/6] states: Prepare to receive 64-bit replies, Eric Blake, 2023/07/21
- [libnbd PATCH v4 6/6] states: Break deadlock if server goofs on extended replies, Eric Blake, 2023/07/21