Re: [Libguestfs] [libnbd PATCH v2 1/5] api: Add xxx_MASK constant for each Flags type

Richard W.M. Jones
2020-09-17T13:23:30Z

On Fri, Sep 11, 2020 at 04:49:52PM -0500, Eric Blake
wrote:
> Document and make it easier for applications to determine which
> bitmask values were supported at compile time.  Also, generating
> bitmask values in hex tends to be more legible than in decimal, as it
> makes it obvious that the values are intended for bitwise operations.
> And it doesn't hurt that this reduces churn in some of the tests if
> new bits are added.

Yes this is a sensible bit of tidy-up.

ACK.

Rich.

>  docs/libnbd.pod                                     |  7 +++++++
>  generator/API.ml                                    | 13 +++++++++++--
>  generator/C.ml                                      | 13 ++++++++-----
>  generator/GoLang.ml                                 |  8 ++++++--
>  generator/OCaml.ml                                  |  9 +++++++++
>  generator/Python.ml                                 |  5 ++++-
>  lib/handle.c                                        |  5 ++---
>  python/t/110-defaults.py                            |  3 +--
>  python/t/120-set-non-defaults.py                    |  5 ++---
>  ocaml/tests/test_110_defaults.ml                    |  3 +--
>  ocaml/tests/test_120_set_non_defaults.ml            |  3 +--
>  tests/errors.c                                      |  9 +++------
>  .../libnbd/libnbd_110_defaults_test.go              |  2 +-
>  .../libnbd/libnbd_120_set_non_defaults_test.go      |  4 ++--
>  14 files changed, 58 insertions(+), 31 deletions(-)
>
> diff --git a/docs/libnbd.pod b/docs/libnbd.pod
> index f2ba3bb..d8dffea 100644
> --- a/docs/libnbd.pod
> +++ b/docs/libnbd.pod
> @@ -520,6 +520,13 @@ To get the size of the export in bytes, use
L<nbd_get_size(3)>:
>  You can read and write data from the NBD server using
L<nbd_pread(3)>
>  and L<nbd_pwrite(3)> or their asynchronous equivalents.
>
> +All data commands support a C<flags> argument (mandatory in C, but
> +optional in languages where it can default to 0).  For convenience,
> +the constant C<LIBNBD_CMD_FLAG_MASK> is defined with the set of
flags
> +currently recognized by libnbd, where future NBD protocol extensions
> +may result in additional flags being supported; but in general,
> +specific data commands only accept a subset of known flags.
> +
>  Some servers also support:
>
>  =over 4
> diff --git a/generator/API.ml b/generator/API.ml
> index cc1a7ba..42eeac0 100644
> --- a/generator/API.ml
> +++ b/generator/API.ml
> @@ -679,6 +679,8 @@ fewer all-zero padding bytes over the connection.
>
>  =back
>
> +For convenience, the constant C<LIBNBD_HANDSHAKE_FLAG_MASK> is
> +available to describe all flags supported by this build of libnbd.
>  Future NBD extensions may add further flags, which in turn may
>  be enabled by default in newer libnbd.  As such, when attempting
>  to disable only one specific bit, it is wiser to first call
> @@ -895,7 +897,11 @@ ORed together:
>
>  =item C<LIBNBD_ALLOW_TRANSPORT_VSOCK>
>
> -=back";
> +=back
> +
> +For convenience, the constant C<LIBNBD_ALLOW_TRANSPORT_MASK> is
> +available to describe all transports recognized by this build of
> +libnbd.  A future version of the library may add new flags.";
>      see_also = [Link "connect_uri"; Link
"set_uri_allow_tls"];
>    };
>
> @@ -1629,7 +1635,10 @@ issuing those commands before informing the server
of the intent
>  to disconnect.
>
>  =back
> -";
> +
> +For convenience, the constant C<LIBNBD_SHUTDOWN_MASK> is available
> +to describe all shutdown flags recognized by this build of libnbd.
> +A future version of the library may add new flags.";
>      see_also = [Link "close"; Link "aio_disconnect"];
>      example = Some "examples/reads-and-writes.c";
>    };
> diff --git a/generator/C.ml b/generator/C.ml
> index c9f0ff4..4d4958d 100644
> --- a/generator/C.ml
> +++ b/generator/C.ml
> @@ -349,11 +349,15 @@ let generate_include_libnbd_h () >    ) all_enums;
>    List.iter (
>      fun { flag_prefix; flags } ->
> +      let mask = ref 0 in
>        List.iter (
>          fun (flag, i) ->
>            let flag = sprintf "LIBNBD_%s_%s" flag_prefix flag in
> -          pr "#define %-40s %d\n" flag i
> +          pr "#define %-40s 0x%02x\n" flag i;
> +          mask := !mask lor i
>        ) flags;
> +      let flag = sprintf "LIBNBD_%s_MASK" flag_prefix in
> +      pr "#define %-40s 0x%02x\n" flag !mask;
>        pr "\n"
>    ) all_flags;
>    List.iter (
> @@ -490,13 +494,12 @@ let generate_lib_api_c () >      );
>
>      (* Check parameters are valid. *)
> -    let print_flags_check n { flag_prefix; flags } > +    let
print_flags_check n { flag_prefix } >        let value = match errcode with
>          | Some value -> value
>          | None -> assert false in
> -      let mask = List.fold_left (lor) 0 (List.map snd flags) in
> -      pr "  if (unlikely ((%s & ~%d) != 0)) {\n" n mask;
> -      pr "    set_error (EINVAL, \"%%s: invalid value for flag:
%%d\",\n";
> +      pr "  if (unlikely ((%s & ~LIBNBD_%s_MASK) != 0)) {\n"
n flag_prefix;
> +      pr "    set_error (EINVAL, \"%%s: invalid value for flag:
0x%%x\",\n";
>        pr "               \"%s\", %s);\n" n n;
>        pr "    ret = %s;\n" value;
>        pr "    goto out;\n";
> diff --git a/generator/GoLang.ml b/generator/GoLang.ml
> index 359c7d0..f07b074 100644
> --- a/generator/GoLang.ml
> +++ b/generator/GoLang.ml
> @@ -447,12 +447,16 @@ import (
>  ";
>    List.iter (
>      fun { flag_prefix; flags } ->
> -      pr "type %s uint32\n" (camel_case flag_prefix);
> +      let flag_type = camel_case flag_prefix in
> +      let mask = ref 0 in
> +      pr "type %s uint32\n" flag_type;
>        pr "const (\n";
>        List.iter (
>          fun (flag, v) ->
> -          pr "    %s_%s = %s(%d)\n" flag_prefix flag (camel_case
flag_prefix) v
> +          pr "    %s_%s = %s(0x%02x)\n" flag_prefix flag
flag_type v;
> +          mask := !mask lor v
>        ) flags;
> +      pr "    %s_MASK = %s(0x%02x)\n" flag_prefix flag_type
!mask;
>        pr ")\n";
>        pr "\n"
>    ) all_flags;
> diff --git a/generator/OCaml.ml b/generator/OCaml.ml
> index 4bcd450..43b3679 100644
> --- a/generator/OCaml.ml
> +++ b/generator/OCaml.ml
> @@ -164,6 +164,8 @@ type cookie = int64
>            pr "  | %s\n" flag
>        ) flags;
>        pr "  | UNKNOWN of int\n";
> +      pr "\n";
> +      pr "  val mask : t list\n";
>        pr "end\n";
>        pr "\n"
>    ) all_flags;
> @@ -269,6 +271,13 @@ let () >            pr "  | %s\n" flag
>        ) flags;
>        pr "  | UNKNOWN of int\n";
> +      pr "\n";
> +      pr "  let mask = [\n";
> +      List.iter (
> +        fun (flag, _) ->
> +          pr "    %s;\n" flag
> +      ) flags;
> +      pr "  ]\n";
>        pr "end\n";
>        pr "\n"
>    ) all_flags;
> diff --git a/generator/Python.ml b/generator/Python.ml
> index 71368c6..fd09eae 100644
> --- a/generator/Python.ml
> +++ b/generator/Python.ml
> @@ -696,11 +696,14 @@ Error.__str__ = _str
>    ) all_enums;
>    List.iter (
>      fun { flag_prefix; flags } ->
> +      let mask = ref 0 in
>        List.iter (
>          fun (flag, i) ->
>            let flag = sprintf "%s_%s" flag_prefix flag in
> -          pr "%s = %d\n" flag i
> +          pr "%s = 0x%02x\n" flag i;
> +          mask := !mask lor i
>        ) flags;
> +      pr "%s_MASK = 0x%02x\n" flag_prefix !mask;
>        pr "\n"
>    ) all_flags;
>    List.iter (fun (n, i) -> pr "%s = %d\n" n i) constants;
> diff --git a/lib/handle.c b/lib/handle.c
> index 7987d59..4d26842 100644
> --- a/lib/handle.c
> +++ b/lib/handle.c
> @@ -65,12 +65,11 @@ nbd_create (void)
>    h->tls_verify_peer = true;
>    h->request_sr = true;
>
> -  h->uri_allow_transports = (uint32_t) -1;
> +  h->uri_allow_transports = LIBNBD_ALLOW_TRANSPORT_MASK;
>    h->uri_allow_tls = LIBNBD_TLS_ALLOW;
>    h->uri_allow_local_file = false;
>
> -  h->gflags = (LIBNBD_HANDSHAKE_FLAG_FIXED_NEWSTYLE |
> -               LIBNBD_HANDSHAKE_FLAG_NO_ZEROES);
> +  h->gflags = LIBNBD_HANDSHAKE_FLAG_MASK;
>
>    s = getenv ("LIBNBD_DEBUG");
>    h->debug = s && strcmp (s, "1") == 0;
> diff --git a/python/t/110-defaults.py b/python/t/110-defaults.py
> index 47c1d02..fb961cf 100644
> --- a/python/t/110-defaults.py
> +++ b/python/t/110-defaults.py
> @@ -22,6 +22,5 @@ assert h.get_export_name() == ""
>  assert h.get_full_info() is False
>  assert h.get_tls() == nbd.TLS_DISABLE
>  assert h.get_request_structured_replies() is True
> -assert h.get_handshake_flags() == (nbd.HANDSHAKE_FLAG_FIXED_NEWSTYLE |
> -                                   nbd.HANDSHAKE_FLAG_NO_ZEROES)
> +assert h.get_handshake_flags() == nbd.HANDSHAKE_FLAG_MASK
>  assert h.get_opt_mode() is False
> diff --git a/python/t/120-set-non-defaults.py
b/python/t/120-set-non-defaults.py
> index 3963021..3da0c23 100644
> --- a/python/t/120-set-non-defaults.py
> +++ b/python/t/120-set-non-defaults.py
> @@ -34,11 +34,10 @@ if h.supports_tls():
>  h.set_request_structured_replies(False)
>  assert h.get_request_structured_replies() is False
>  try:
> -    h.set_handshake_flags(nbd.HANDSHAKE_FLAG_NO_ZEROES << 1)
> +    h.set_handshake_flags(nbd.HANDSHAKE_FLAG_MASK + 1)
>      assert False
>  except nbd.Error:
> -    assert h.get_handshake_flags() == (nbd.HANDSHAKE_FLAG_NO_ZEROES |
> -                                       nbd.HANDSHAKE_FLAG_FIXED_NEWSTYLE)
> +    assert h.get_handshake_flags() == nbd.HANDSHAKE_FLAG_MASK
>  h.set_handshake_flags(0)
>  assert h.get_handshake_flags() == 0
>  h.set_opt_mode(True)
> diff --git a/ocaml/tests/test_110_defaults.ml
b/ocaml/tests/test_110_defaults.ml
> index 54f2cbc..f5886fc 100644
> --- a/ocaml/tests/test_110_defaults.ml
> +++ b/ocaml/tests/test_110_defaults.ml
> @@ -28,8 +28,7 @@ let () >    let sr =
NBD.get_request_structured_replies nbd in
>    assert (sr = true);
>    let flags = NBD.get_handshake_flags nbd in
> -  assert (flags = [ NBD.HANDSHAKE_FLAG.FIXED_NEWSTYLE;
> -                    NBD.HANDSHAKE_FLAG.NO_ZEROES ]);
> +  assert (flags = NBD.HANDSHAKE_FLAG.mask);
>    let opt = NBD.get_opt_mode nbd in
>    assert (opt = false)
>
> diff --git a/ocaml/tests/test_120_set_non_defaults.ml
b/ocaml/tests/test_120_set_non_defaults.ml
> index 79fe184..b660e5d 100644
> --- a/ocaml/tests/test_120_set_non_defaults.ml
> +++ b/ocaml/tests/test_120_set_non_defaults.ml
> @@ -46,8 +46,7 @@ let () >    with
>      NBD.Error _ -> ();
>    let flags = NBD.get_handshake_flags nbd in
> -  assert (flags = [ NBD.HANDSHAKE_FLAG.FIXED_NEWSTYLE;
> -                    NBD.HANDSHAKE_FLAG.NO_ZEROES ]);
> +  assert (flags = NBD.HANDSHAKE_FLAG.mask);
>    NBD.set_handshake_flags nbd [];
>    let flags = NBD.get_handshake_flags nbd in
>    assert (flags = []);
> diff --git a/tests/errors.c b/tests/errors.c
> index 906c7da..8166429 100644
> --- a/tests/errors.c
> +++ b/tests/errors.c
> @@ -86,7 +86,6 @@ main (int argc, char *argv[])
>     */
>    const char *cmd[] = { "nbdkit", "-s",
"--exit-with-parent", "sh",
>                          script, NULL };
> -  int i;
>
>    progname = argv[0];
>
> @@ -147,15 +146,13 @@ main (int argc, char *argv[])
>    }
>
>    /* Attempt to set a bitmask with an unknown bit. */
> -  i = LIBNBD_HANDSHAKE_FLAG_NO_ZEROES << 1;
> -  if (nbd_set_handshake_flags (nbd, i) != -1) {
> +  if (nbd_set_handshake_flags (nbd, LIBNBD_HANDSHAKE_FLAG_MASK + 1) != -1)
{
>      fprintf (stderr, "%s: test failed: "
>               "nbd_set_handshake_flags did not reject invalid
bitmask\n",
>               argv[0]);
>      exit (EXIT_FAILURE);
>    }
> -  i = LIBNBD_HANDSHAKE_FLAG_FIXED_NEWSTYLE |
LIBNBD_HANDSHAKE_FLAG_NO_ZEROES;
> -  if (nbd_get_handshake_flags (nbd) != i) {
> +  if (nbd_get_handshake_flags (nbd) != LIBNBD_HANDSHAKE_FLAG_MASK) {
>      fprintf (stderr, "%s: test failed: "
>               "nbd_get_handshake_flags not left at default
value\n",
>               argv[0]);
> @@ -247,7 +244,7 @@ main (int argc, char *argv[])
>    check (EINVAL, "nbd_pread: ");
>
>    /* Use unknown command flags */
> -  if (nbd_pread (nbd, buf, 1, 0, -1) != -1) {
> +  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]);
> diff --git a/golang/src/libguestfs.org/libnbd/libnbd_110_defaults_test.go
b/golang/src/libguestfs.org/libnbd/libnbd_110_defaults_test.go
> index 9af0ed8..9e782ac 100644
> --- a/golang/src/libguestfs.org/libnbd/libnbd_110_defaults_test.go
> +++ b/golang/src/libguestfs.org/libnbd/libnbd_110_defaults_test.go
> @@ -63,7 +63,7 @@ func Test110Defaults(t *testing.T) {
>  	if err != nil {
>  		t.Fatalf("could not get handshake flags: %s", err)
>  	}
> -	if flags != HANDSHAKE_FLAG_FIXED_NEWSTYLE | HANDSHAKE_FLAG_NO_ZEROES {
> +	if flags != HANDSHAKE_FLAG_MASK {
>  		t.Fatalf("unexpected handshake flags")
>  	}
>
> diff --git
a/golang/src/libguestfs.org/libnbd/libnbd_120_set_non_defaults_test.go
b/golang/src/libguestfs.org/libnbd/libnbd_120_set_non_defaults_test.go
> index 5d8cce7..b0b06bd 100644
> --- a/golang/src/libguestfs.org/libnbd/libnbd_120_set_non_defaults_test.go
> +++ b/golang/src/libguestfs.org/libnbd/libnbd_120_set_non_defaults_test.go
> @@ -93,7 +93,7 @@ func Test120SetNonDefaults(t *testing.T) {
>  		t.Fatalf("unexpected structured replies state")
>  	}
>
> -	err = h.SetHandshakeFlags(HANDSHAKE_FLAG_NO_ZEROES << 1)
> +	err = h.SetHandshakeFlags(HANDSHAKE_FLAG_MASK + 1)
>  	if err == nil {
>  		t.Fatalf("expect failure for out-of-range flags")
>  	}
> @@ -101,7 +101,7 @@ func Test120SetNonDefaults(t *testing.T) {
>  	if err != nil {
>  		t.Fatalf("could not get handshake flags: %s", err)
>  	}
> -	if flags != HANDSHAKE_FLAG_FIXED_NEWSTYLE | HANDSHAKE_FLAG_NO_ZEROES {
> +	if flags != HANDSHAKE_FLAG_MASK {
>  		t.Fatalf("unexpected handshake flags")
>  	}
>
> --
> 2.28.0
>
> _______________________________________________
> Libguestfs mailing list
> Libguestfs@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
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.