On 5/30/23 20:18, Eric Blake wrote:
> On Tue, May 30, 2023 at 04:06:32PM +0200, Laszlo Ersek wrote:
>> On 5/25/23 15:00, 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).
>>>
>>> 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 <ebl...@redhat.com>
>>> ---
>
>>> @@ -364,7 +370,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 */
>>
>> (1) Are there places in the code where we currently assign this "count"
>> field back to a uint32_t object, and assume truncation impossible?
>
> Grepping for '->count' in lib/ and generator/ shows we need to check
> at least:
>
> generator/states-reply-simple.c: h->rlen = cmd->count;
> generator/states-reply-simple.c: cmd->data_seen += cmd->count;
>
> which are adjustments to size_t and uint32_t variables respectively,
> in response to a server's reply to an NBD_CMD_READ command. But since
> we never send a server a read request larger than 64M, truncation and
> overflow are not possible in those lines of code (at most one simple
> reply is possible, and code in states-reply-structured.c ensures that
> cmd->data_seen is a saturating variable that never exceeds
> 2*MAX_REQUEST_SIZE).
>
> There is also pre-series:
>
> generator/states-issue-command.c: h->request.count = htobe32 (cmd->count);
>
> which is specifically updated in this patch to cover extended headers.
>
>>> +++ 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.handle = 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.handle = htobe64 (cmd->cookie);
>>> + h->req.compact.offset = htobe64 (cmd->offset);
>>
>> What's more, this is a "by the book" common initial sequence! :)
>
>>
>>> + 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);
>>> + }
>
> Indeed, and shows why my efforts to get a sane layout early in the
> series matter, even if it will cause me a bit more rebase churn here
> based on my response to your comments earlier in the series.
>
>>> @@ -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;
>>
>> Nice -- I wanted to ask for:
>>
>> flags &= ~(uint32_t)LIBNBD_CMD_FLAG_PAYLOAD_LEN;
>>
>> due to LIBNBD_CMD_FLAG_PAYLOAD_LEN having type "int".
>>
>> However: in patch#3, what has type "int" is:
>>
>> +#define NBD_CMD_FLAG_PAYLOAD_LEN (1<<5)
>>
>> and here we have LIBNBD_CMD_FLAG_PAYLOAD_LEN instead -- and the latter
>> has type unsigned int already, from your recent commit 69eecae2c03a
>> ("api: Generate flag values as unsigned", 2022-11-11).
>
> Still, worth a (separate) cleanup patch to nbd-protocol.h to prefer
> unsigned constants for the flag values where they are not generated.
>
>>
>> And I think we're fine assuming that uint32_t is unsigned int.
>
> Not true of all generic C platforms, but certainly true for the
> POSIX-like platforms we target (anyone that defines uint32_t as
> 'unsigned long' on a platform with 32-bit longs is unusual, but even
> then we should still be okay).
>
>>
>>>
>>> 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 3a93251e..8b839bf5 100644
>>> --- a/tests/Makefile.am
>>> +++ b/tests/Makefile.am
>>> @@ -232,6 +232,7 @@ check_PROGRAMS += \
>>> closure-lifetimes \
>>> pread-initialize \
>>> socket-activation-name \
>>> + pwrite-extended \
>>> $(NULL)
>>>
>>> TESTS += \
>>
>> (2) Incorrect indentation: two spaces rather than one tab.
>
> Arrgh. ./.editorconfig is supposed to do this correctly, but
> obviously its interaction with emacs is a bit botched when it comes to
> Makefile syntax. Will clean up.
>
>>> +++ b/tests/pwrite-extended.c
>
>>> +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\n", nbd_get_error ());
>>
>> (3) Minor inconsistency with check(): we're not printing "progname" here.
>>
>>> + exit (EXIT_FAILURE);
>>> + }
>>> +
>>> + /* Connect to the server. */
>>> + if (nbd_connect_command (nbd, (char **)cmd) == -1) {
>>> + fprintf (stderr, "%s: %s\n", argv[0], nbd_get_error ());
>>> + exit (EXIT_FAILURE);
>>> + }
>>
>> (4) Another kind of inconsistency: we could use "progname" here, in
>> place of argv[0].
>>
>> (This applies to all other fprintf()s below.)
>
> Probably copy-and-paste from other similar tests, but I don't mind
> cleaning those up.
>
>>
>>> +
>>> + /* 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",
>>> + argv[0]);
>>> + exit (EXIT_FAILURE);
>>> + }
>>
>> Not sure if I understand this check. Per
>> <https://libguestfs.org/nbd_set_strict_mode.3.html>, I take it that
>> LIBNBD_STRICT_FLAGS should be "on" by default. Are you enforcing that?
>> And if so: is it your intent that, *even with* LIBNBD_STRICT_FLAGS, an
>> invalid PAYLOAD_LEN is not rejected (as seen by the libnbd client app),
>> but fixed up silently?
>
> Rather, until we can tell if the server negotiated extended mode, we
> are ASSUMING that the server did NOT negotiate it, and therefore we
> are in violation of the spec if we send the flag over the wire
> anyways.
OK.
> We can flag all other API where it is inappropriate to ever
> use...
>
>>
>>> + 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",
>>> + argv[0]);
>>> + exit (EXIT_FAILURE);
>>> + }
>>> + check (EINVAL, "nbd_aio_pread: ");
>>
>> Ah, got it now. We do want APIs other than pwrite to fail.
>
> ...but because we don't want to require clients to correctly decide
> when to pass or omit the flag to their API calls (by us masking out
> the user's choice and then hardcoding our actual wire behavior based
> on negotiated mode), passing the flag to pread works even when it
> would be technically wrong over the wire.
I don't understand.
What you describe here (= us fixing up the flag for the caller) applies
to *pwrite*, not *pread*. Furthermore, the above check tests pread's
behavior, and it expects pread to *fail*.
In effect, my understanding of the test code is this:
- assume extended headers have not been negotiated
- require that the NBD connection be created such that it enforces flag
validity on the client side (i.e., "strict mode" including "strict flags")
- test that pread fails with PAYLOAD_LEN -- pread should fail
*regardless* of extended headers having been negotiated, because (a) if
extended headers are not in use, then the flag is altogether invalid,
(b) even with extended headers, a read request does not accept the flag.
Because we don't add "PAYLOAD_LEN" as a valid flag to pread in the
generator code, the check for (b) is always active.
- test that pwrite succeeds with PAYLOAD_LEN -- pwrite should succeed
*regardless* of extended headers having been negotiated, because we set
PAYLOAD_LEN internally, dependent on the extended headers; i.e.,
ignoring the user's argument.
That is, I think I did manage to explain the test to myself, but your
most recent answer confuses me again! :)
> The FIXME does get modified
> again later in the series, when I do add in support for detecting when
> the server supports extended headers.
Right, I assume FIXME in the test code might be addressed together with
the TODO in nbd_unlocked_aio_pwrite(). Once we know whether the server
negotiated extended headers, *and* if the user asks for strictness
regarding the PAYLOAD_LEN flag, we can enforce PAYLOAD_LEN's equivalence
with extended headers in pwrite calls.
Laszlo
>
>>
>>> +
>>> + if (nbd_aio_pwrite (nbd, buf, 512, 0, NBD_NULL_COMPLETION,
>>> + LIBNBD_CMD_FLAG_PAYLOAD_LEN) == -1) {
>>> + fprintf (stderr, "%s: %s\n", argv[0], nbd_get_error ());
>>> + exit (EXIT_FAILURE);
>>> + }
>>> +
>>> + if (nbd_aio_pwrite (nbd, buf, 512, 0, NBD_NULL_COMPLETION, 0) == -1) {
>>> + fprintf (stderr, "%s: %s\n", argv[0], nbd_get_error ());
>>> + exit (EXIT_FAILURE);
>>> + }
>>
>> You could contract these into:
>>
>> 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", argv[0], nbd_get_error ());
>> exit (EXIT_FAILURE);
>> }
>
> Sure.
>
>>
>>> +
>>> + nbd_close (nbd);
>>> + exit (EXIT_SUCCESS);
>>> +}
>>
>> In general, I think it's good practice to reach nbd_close() whenever
>> nbd_create() succeeds (that is, on error paths as well, after
>> nbd_create() succeeds). For example, if we connected to the server with
>> systemd socket activation in this test, and (say) one of the pwrites
>> failed, then we'd leak the unix domain socket in the filesystem (from
>> the bind() in "generator/states-connect-socket-activation.c").
>> "sact_sockpath" is unlinked in nbd_close().
>>
>> (As written, this test should not be affected, because, according to
>> unix(7), unix domain sockets created with socketpair(2) are unnamed.)
>
> Pre-existing in other tests, but a good observation for a followup patch.
>
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
