[Libguestfs] [libnbd PATCH v3 04/22] states: Prepare to send 64-bit requests

[Eric Blake]

On Wed, May 31, 2023 at 01:59:12PM +0200, Laszlo Ersek wrote:
> >>
> >>> +
> >>> +  /* 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*.

I'll have to blame late-night email responses for this one.  Yes,
*pread* rejects the flag from the library client (never valid to use;
if we bypass safety checks we can send it to the server which will
also flag it); *pwrite" ignores the flag from the library client
(whether or not we use it, and whether or not our use matches what is
negotiated, what we send to the server uses what is negotiated).

> 
> 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! :)

Yeah, your explanation is correct, and I should quit writing emails at
my bedtime.

> 
> > 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.

I did NOT add a strictness flag in v3; but may do so in v4.  You are
right that it would require a user to opt-in to that strictness flag
before we enforce PAYLOAD_LEN to match the use of extended headers.
Unlike other strictness flags that are on by default, no sane user
will opt in to this one except for unit tests of low-level protocol
behaviors where it can be handy for forcing libnbd to send known-bad
packets to see whether a server trying to implement extended headers
has done so gracefully, hence, adding the strictness flag is lower
priority.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org