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

>> >>> + >>> + /* 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.