[Libguestfs] [libnbd PATCH v2 4/5] api: Add STRICT_FLAGS to set_strict_mode
Richard W.M. Jones
rjones at redhat.com
Thu Sep 17 13:32:19 UTC 2020
On Fri, Sep 11, 2020 at 04:49:55PM -0500, Eric Blake wrote:
> The next strict knob: allow the user to pass unknown flags across the
> wire (this is different than passing a known flag at the wrong time).
>
> It is interesting to note that NBD only permits 16 bits of flags, but
> we have a signature that takes uint32_t; if we wanted, we could pack
> libnbd-specific flags in the upper bits that the NBD protocol would
> never see.
This is fine ACK.
About the "guard" change: I probably would have put that change in a
separate commit. Also you could consider having a default_flags
parameter to avoid having to set guard = None everywhere, although you
would still have to make a one-off change to every *_flags. It would
look like this:
let default_flags = { flag_prefix = ""; guard = None; flags = [] }
...
let handshake_flags = {
default_flags with
flag_prefix = "HANDSHAKE_FLAG";
flags = [ ... ]
}
(We already do this with "default_call"). Neither of these is a big deal
though.
Rich.
> generator/API.ml | 46 ++++++++++++++++++++++++++++++++++++++--------
> generator/API.mli | 1 +
> generator/C.ml | 7 +++++--
> lib/disconnect.c | 2 +-
> lib/rw.c | 6 +++---
> tests/errors.c | 22 ++++++++++++++++++++--
> 6 files changed, 68 insertions(+), 16 deletions(-)
>
> diff --git a/generator/API.ml b/generator/API.ml
> index aa970e6..4cd425b 100644
> --- a/generator/API.ml
> +++ b/generator/API.ml
> @@ -87,6 +87,7 @@ and enum = {
> }
> and flags = {
> flag_prefix : string;
> + guard : string option;
> flags : (string * int) list
> }
> and permitted_state =
> @@ -165,6 +166,7 @@ let all_enums = [ tls_enum; block_size_enum ]
> (* Flags. See also Constants below. *)
> 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;
> @@ -175,6 +177,7 @@ let cmd_flags = {
> }
> let handshake_flags = {
> flag_prefix = "HANDSHAKE_FLAG";
> + guard = None;
> flags = [
> "FIXED_NEWSTYLE", 1 lsl 0;
> "NO_ZEROES", 1 lsl 1;
> @@ -182,12 +185,15 @@ let handshake_flags = {
> }
> let strict_flags = {
> flag_prefix = "STRICT";
> + guard = None;
> flags = [
> "COMMANDS", 1 lsl 0;
> + "FLAGS", 1 lsl 1;
> ]
> }
> let allow_transport_flags = {
> flag_prefix = "ALLOW_TRANSPORT";
> + guard = None;
> flags = [
> "TCP", 1 lsl 0;
> "UNIX", 1 lsl 1;
> @@ -196,6 +202,7 @@ let allow_transport_flags = {
> }
> let shutdown_flags = {
> flag_prefix = "SHUTDOWN";
> + guard = None;
> flags = [
> "IMMEDIATE", 1 lsl 1;
> ]
> @@ -749,6 +756,22 @@ a read-only server, or attempting to use C<LIBNBD_CMD_FLAG_FUA> when
> L<nbd_can_fua(3)> returned false). If clear, this flag relies on the
> server to reject unexpected commands.
>
> +=item C<LIBNBD_STRICT_FLAGS> = 2
> +
> +If set, this flag rejects client requests that attempt to set a
> +command flag not recognized by libnbd (those outside of
> +C<LIBNBD_CMD_FLAG_MASK>). If clear, this flag passes on unknown
> +flags to the server. One possible reason to relax this strictness
> +knob is to send C<LIBNBD_CMD_FLAG_FUA> on a read command (libnbd
> +normally prevents that, but the NBD protocol allows it to succeed);
> +however, it is also possible that sending an unknown flag may
> +cause the server to change its reply in a manner that confuses
> +libnbd, perhaps causing deadlock or ending the connection.
> +
> +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
> +range permitted by the protocol are always a client-side error.
> +
> =back
>
> For convenience, the constant C<LIBNBD_STRICT_MASK> is available to
> @@ -1568,9 +1591,10 @@ required into the server's replies, or if you want to use
> C<LIBNBD_CMD_FLAG_DF>.
>
> The C<flags> parameter must be C<0> for now (it exists for future NBD
> -protocol extensions).";
> +protocol extensions)."
> +^ strict_call_description;
> see_also = [Link "aio_pread"; Link "pread_structured";
> - Link "get_block_size"];
> + Link "get_block_size"; Link "set_strict_mode"];
> example = Some "examples/fetch-first-sector.c";
> };
>
> @@ -1646,9 +1670,11 @@ The C<flags> parameter may be C<0> for no flags, or may contain
> C<LIBNBD_CMD_FLAG_DF> meaning that the server should not reply with
> more than one fragment (if that is supported - some servers cannot do
> this, see L<nbd_can_df(3)>). Libnbd does not validate that the server
> -actually obeys the flag.";
> +actually obeys the flag."
> +^ strict_call_description;
> see_also = [Link "can_df"; Link "pread";
> - Link "aio_pread_structured"; Link "get_block_size"];
> + Link "aio_pread_structured"; Link "get_block_size";
> + Link "set_strict_mode"];
> };
>
> "pwrite", {
> @@ -2123,10 +2149,12 @@ Or supply the optional C<completion_callback> which will be invoked
> as described in L<libnbd(3)/Completion callbacks>.
>
> Note that you must ensure C<buf> is valid until the command has
> -completed. Other parameters behave as documented in L<nbd_pread(3)>.";
> +completed. Other parameters behave as documented in L<nbd_pread(3)>."
> +^ strict_call_description;
> example = Some "examples/aio-connect-read.c";
> see_also = [SectionLink "Issuing asynchronous commands";
> - Link "aio_pread_structured"; Link "pread"];
> + Link "aio_pread_structured"; Link "pread";
> + Link "set_strict_mode"];
> };
>
> "aio_pread_structured", {
> @@ -2145,9 +2173,11 @@ To check if the command completed, call L<nbd_aio_command_completed(3)>.
> Or supply the optional C<completion_callback> which will be invoked
> as described in L<libnbd(3)/Completion callbacks>.
>
> -Other parameters behave as documented in L<nbd_pread_structured(3)>.";
> +Other parameters behave as documented in L<nbd_pread_structured(3)>."
> +^ strict_call_description;
> see_also = [SectionLink "Issuing asynchronous commands";
> - Link "aio_pread"; Link "pread_structured"];
> + Link "aio_pread"; Link "pread_structured";
> + Link "set_strict_mode"];
> };
>
> "aio_pwrite", {
> diff --git a/generator/API.mli b/generator/API.mli
> index db978ca..41e09f5 100644
> --- a/generator/API.mli
> +++ b/generator/API.mli
> @@ -100,6 +100,7 @@ and enum = {
> }
> and flags = {
> flag_prefix : string; (** prefix of each flag name *)
> + guard : string option; (** additional gating for checking valid flags *)
> flags : (string * int) list (** flag names and their values in C *)
> }
> and permitted_state =
> diff --git a/generator/C.ml b/generator/C.ml
> index 86d9c5c..5f68b14 100644
> --- a/generator/C.ml
> +++ b/generator/C.ml
> @@ -494,7 +494,7 @@ let generate_lib_api_c () =
> );
>
> (* Check parameters are valid. *)
> - let print_flags_check n { flag_prefix; flags } subset =
> + let print_flags_check n { flag_prefix; flags; guard } subset =
> let value = match errcode with
> | Some value -> value
> | None -> assert false in
> @@ -508,7 +508,10 @@ let generate_lib_api_c () =
> ) flags;
> sprintf "0x%x" !v
> | None -> "LIBNBD_" ^ flag_prefix ^ "_MASK" in
> - pr " if (unlikely ((%s & ~%s) != 0)) {\n" n mask;
> + let guard = match guard with
> + | Some value -> " && " ^ value
> + | None -> "" in
> + pr " if (unlikely ((%s & ~%s) != 0)%s) {\n" n mask guard;
> pr " set_error (EINVAL, \"%%s: invalid value for flag: 0x%%x\",\n";
> pr " \"%s\", %s);\n" n n;
> pr " ret = %s;\n" value;
> diff --git a/lib/disconnect.c b/lib/disconnect.c
> index 9de1e34..f99fbd0 100644
> --- a/lib/disconnect.c
> +++ b/lib/disconnect.c
> @@ -64,7 +64,7 @@ nbd_unlocked_aio_disconnect (struct nbd_handle *h, uint32_t flags)
> {
> int64_t id;
>
> - id = nbd_internal_command_common (h, 0, NBD_CMD_DISC, 0, 0, NULL, NULL);
> + id = nbd_internal_command_common (h, flags, NBD_CMD_DISC, 0, 0, NULL, NULL);
> if (id == -1)
> return -1;
> h->disconnect_request = true;
> diff --git a/lib/rw.c b/lib/rw.c
> index f49fe25..e3e80ad 100644
> --- a/lib/rw.c
> +++ b/lib/rw.c
> @@ -281,7 +281,7 @@ nbd_unlocked_aio_pread (struct nbd_handle *h, void *buf,
> struct command_cb cb = { .completion = *completion };
>
> SET_CALLBACK_TO_NULL (*completion);
> - return nbd_internal_command_common (h, 0, NBD_CMD_READ, offset, count,
> + return nbd_internal_command_common (h, flags, NBD_CMD_READ, offset, count,
> buf, &cb);
> }
>
> @@ -350,7 +350,7 @@ nbd_unlocked_aio_flush (struct nbd_handle *h,
> }
>
> SET_CALLBACK_TO_NULL (*completion);
> - return nbd_internal_command_common (h, 0, NBD_CMD_FLUSH, 0, 0,
> + return nbd_internal_command_common (h, flags, NBD_CMD_FLUSH, 0, 0,
> NULL, &cb);
> }
>
> @@ -409,7 +409,7 @@ nbd_unlocked_aio_cache (struct nbd_handle *h,
> }
>
> SET_CALLBACK_TO_NULL (*completion);
> - return nbd_internal_command_common (h, 0, NBD_CMD_CACHE, offset, count,
> + return nbd_internal_command_common (h, flags, NBD_CMD_CACHE, offset, count,
> NULL, &cb);
> }
>
> diff --git a/tests/errors.c b/tests/errors.c
> index 0c4151a..0cfcac5 100644
> --- a/tests/errors.c
> +++ b/tests/errors.c
> @@ -84,7 +84,7 @@ main (int argc, char *argv[])
> * which delays responding to writes until a witness file no longer
> * exists.
> */
> - const char *cmd[] = { "nbdkit", "-s", "--exit-with-parent", "sh",
> + const char *cmd[] = { "nbdkit", "-s", "-v", "--exit-with-parent", "sh",
> script, NULL };
> uint32_t strict;
>
> @@ -244,7 +244,25 @@ main (int argc, char *argv[])
> }
> check (EINVAL, "nbd_pread: ");
>
> - /* Use unknown command flags */
> + /* Use unknown command flags, client-side */
> + strict = nbd_get_strict_mode (nbd) | LIBNBD_STRICT_FLAGS;
> + if (nbd_set_strict_mode (nbd, strict) == -1) {
> + fprintf (stderr, "%s\n", nbd_get_error ());
> + exit (EXIT_FAILURE);
> + }
> + if (nbd_pread (nbd, buf, 1, 0, LIBNBD_CMD_FLAG_MASK + 1) != -1) {
> + fprintf (stderr, "%s: test failed: "
> + "nbd_pread did not fail with bogus flags\n",
> + argv[0]);
> + exit (EXIT_FAILURE);
> + }
> + check (EINVAL, "nbd_pread: ");
> + /* Use unknown command flags, server-side */
> + strict &= ~(LIBNBD_STRICT_FLAGS | LIBNBD_STRICT_COMMANDS);
> + if (nbd_set_strict_mode (nbd, strict) == -1) {
> + fprintf (stderr, "%s\n", nbd_get_error ());
> + exit (EXIT_FAILURE);
> + }
> if (nbd_pread (nbd, buf, 1, 0, LIBNBD_CMD_FLAG_MASK + 1) != -1) {
> fprintf (stderr, "%s: test failed: "
> "nbd_pread did not fail with bogus flags\n",
> --
> 2.28.0
>
> _______________________________________________
> Libguestfs mailing list
> Libguestfs at redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW
More information about the Libguestfs
mailing list