Libguestfs - May 2019 - Re: [PATCH] api: Add a special type for the flags argument.

This applies on top of patches 1 & 2 here (instead of patch 3):

https://www.redhat.com/archives/libguestfs/2019-May/msg00206.html
https://www.redhat.com/archives/libguestfs/2019-May/msg00207.html

Rich.

By using a special type we can more naturally express flags in
different programming languages.  For example OCaml will prefer an
optional argument containing a list of flags, defaulting to the empty
list:

  pread [...] ?(flags = [])
---
 generator/generator          | 66 +++++++++++++++++++++++++-----------
 python/t/400-pread.py        |  2 +-
 python/t/410-pwrite.py       |  2 +-
 python/t/460-block-status.py |  4 +--
 python/t/500-aio-pread.py    |  2 +-
 python/t/510-aio-pwrite.py   |  2 +-
 6 files changed, 53 insertions(+), 25 deletions(-)

diff --git a/generator/generator b/generator/generator
index 9f107dc..5d46938 100755
--- a/generator/generator
+++ b/generator/generator
@@ -820,6 +820,7 @@ and arg  | BytesPersistIn of string * string (* same as
above, but buffer persists *)
 | BytesPersistOut of string * string
 | Callback of string * arg list (* callback function returning void *)
+| Flags of string          (* NBD_CMD_FLAG_* flags *)
 | Int of string            (* small int *)
 | Int64 of string          (* 64 bit signed int *)
 | Opaque of string         (* opaque object, void* in C *)
@@ -1225,7 +1226,7 @@ with the server.";
 
   "pread", {
     default_call with
-    args = [ BytesOut ("buf", "count"); UInt64
"offset"; UInt32 "flags" ];
+    args = [ BytesOut ("buf", "count"); UInt64
"offset"; Flags "flags" ];
     ret = RErr;
     shortdesc = "read from the NBD server";
     longdesc = "\
@@ -1241,7 +1242,7 @@ protocol extensions).";
 
   "pwrite", {
     default_call with
-    args = [ BytesIn ("buf", "count"); UInt64
"offset"; UInt32 "flags" ];
+    args = [ BytesIn ("buf", "count"); UInt64
"offset"; Flags "flags" ];
     ret = RErr;
     shortdesc = "write to the NBD server";
     longdesc = "\
@@ -1276,7 +1277,7 @@ C<nbd_aio_disconnect>.";
 
   "flush", {
     default_call with
-    args = [ UInt32 "flags" ]; ret = RErr;
+    args = [ Flags "flags" ]; ret = RErr;
     shortdesc = "flushing pending write requests";
     longdesc = "\
 Issue the flush command to the NBD server.  The function should
@@ -1290,7 +1291,7 @@ protocol extensions).";
 
   "trim", {
     default_call with
-    args = [ UInt64 "count"; UInt64 "offset"; UInt32
"flags" ];
+    args = [ UInt64 "count"; UInt64 "offset"; Flags
"flags" ];
     ret = RErr;
     shortdesc = "send trim to the NBD server";
     longdesc = "\
@@ -1309,7 +1310,7 @@ C<nbd_can_fua>).";
 
   "cache", {
     default_call with
-    args = [ UInt64 "count"; UInt64 "offset"; UInt32
"flags" ];
+    args = [ UInt64 "count"; UInt64 "offset"; Flags
"flags" ];
     ret = RErr;
     shortdesc = "send cache (prefetch) to the NBD server";
     longdesc = "\
@@ -1326,7 +1327,7 @@ protocol extensions).";
 
   "zero", {
     default_call with
-    args = [ UInt64 "count"; UInt64 "offset"; UInt32
"flags" ];
+    args = [ UInt64 "count"; UInt64 "offset"; Flags
"flags" ];
     ret = RErr;
     shortdesc = "send write zeroes to the NBD server";
     longdesc = "\
@@ -1353,7 +1354,7 @@ punching a hole.";
                                   UInt64 "offset";
                                   ArrayAndLen (UInt32 "entries",
                                                "nr_entries")]);
-             UInt32 "flags" ];
+             Flags "flags" ];
     ret = RErr;
     shortdesc = "read the block status of the given range";
     longdesc = "\
@@ -1483,7 +1484,7 @@ on the connection.";
   "aio_pread", {
     default_call with
     args = [ BytesPersistOut ("buf", "count"); UInt64
"offset";
-             UInt32 "flags" ];
+             Flags "flags" ];
     ret = RInt64;
     shortdesc = "read from the NBD server";
     longdesc = "\
@@ -1497,7 +1498,7 @@ parameters behave as documented in
C<nbd_pread>.";
 
   "aio_pwrite", {
     default_call with
-    args = [ BytesPersistIn ("buf", "count"); UInt64
"offset"; UInt32 "flags" ];
+    args = [ BytesPersistIn ("buf", "count"); UInt64
"offset"; Flags "flags" ];
     ret = RInt64;
     shortdesc = "write to the NBD server";
     longdesc = "\
@@ -1511,7 +1512,7 @@ parameters behave as documented in
C<nbd_pwrite>.";
 
   "aio_disconnect", {
     default_call with
-    args = [ UInt32 "flags" ]; ret = RErr;
+    args = [ Flags "flags" ]; ret = RErr;
     shortdesc = "disconnect from the NBD server";
     longdesc = "\
 Issue the disconnect command to the NBD server.  This is
@@ -1526,7 +1527,7 @@ however, C<nbd_shutdown> will call this function if
appropriate.";
 
   "aio_flush", {
     default_call with
-    args = [ UInt32 "flags" ]; ret = RInt64;
+    args = [ Flags "flags" ]; ret = RInt64;
     shortdesc = "send flush command to the NBD server";
     longdesc = "\
 Issue the flush command to the NBD server.  This returns the
@@ -1538,7 +1539,7 @@ in C<nbd_flush>.";
 
   "aio_trim", {
     default_call with
-    args = [ UInt64 "count"; UInt64 "offset"; UInt32
"flags" ];
+    args = [ UInt64 "count"; UInt64 "offset"; Flags
"flags" ];
     ret = RInt64;
     shortdesc = "send trim to the NBD server";
     longdesc = "\
@@ -1551,7 +1552,7 @@ in C<nbd_trim>.";
 
   "aio_cache", {
     default_call with
-    args = [ UInt64 "count"; UInt64 "offset"; UInt32
"flags" ];
+    args = [ UInt64 "count"; UInt64 "offset"; Flags
"flags" ];
     ret = RInt64;
     shortdesc = "send cache (prefetch) to the NBD server";
     longdesc = "\
@@ -1564,7 +1565,7 @@ in C<nbd_cache>.";
 
   "aio_zero", {
     default_call with
-    args = [ UInt64 "count"; UInt64 "offset"; UInt32
"flags" ];
+    args = [ UInt64 "count"; UInt64 "offset"; Flags
"flags" ];
     ret = RInt64;
     shortdesc = "send write zeroes to the NBD server";
     longdesc = "\
@@ -1583,7 +1584,7 @@ in C<nbd_zero>.";
                                   UInt64 "offset";
                                   ArrayAndLen (UInt32 "entries",
                                                "nr_entries")]);
-             UInt32 "flags" ];
+             Flags "flags" ];
     ret = RInt64;
     shortdesc = "send block status to the NBD server";
     longdesc = "\
@@ -2494,6 +2495,21 @@ let generate_lib_states_c ()  
 (* Generate C API. *)
 
+(* Check the API definition. *)
+let () +  (* Flags must only appear once in the final argument position. *)
+  List.iter (
+    fun (name, { args }) ->
+      let args = List.rev args in
+      match args with
+      | [] -> ()
+      | Flags _ :: xs
+      | xs ->
+         if List.exists (function Flags _ -> true | _ -> false) xs then
+           failwithf "%s: Flags must appear in final argument position
only"
+                     name
+  ) handle_calls
+
 let generate_lib_libnbd_syms ()    generate_header HashStyle;
 
@@ -2517,6 +2533,7 @@ let rec name_of_arg = function
 | BytesPersistIn (n, len) -> [n; len]
 | BytesPersistOut (n, len) -> [n; len]
 | Callback (n, _) -> [n]
+| Flags n -> [n]
 | Int n -> [n]
 | Int64 n -> [n]
 | Opaque n -> [n]
@@ -2548,6 +2565,7 @@ let rec print_c_arg_list ?(handle = false) args        |
BytesOut (n, len)
       | BytesPersistOut (n, len) -> pr "void *%s, size_t %s" n len
       | Callback (n, args) -> pr "void (*%s) " n; print_c_arg_list
args
+      | Flags n -> pr "uint32_t %s" n
       | Int n -> pr "int %s" n
       | Int64 n -> pr "int64_t %s" n
       | Opaque n -> pr "void *%s" n
@@ -2950,7 +2968,7 @@ let print_python_binding name { args; ret }           |
UInt64 n -> ()
          (* The following not yet implemented for callbacks XXX *)
          | ArrayAndLen _ | Bool _ | BytesIn _ | BytesOut _
-         | BytesPersistIn _ | BytesPersistOut _ | Callback _
+         | BytesPersistIn _ | BytesPersistOut _ | Callback _ | Flags _
          | Int _ | Int64 _ | Path _ | SockAddrAndLen _ | StringList _
          | UInt _ | UInt32 _ -> pr "  abort ();\n"
        ) args;
@@ -2965,7 +2983,7 @@ let print_python_binding name { args; ret }           |
UInt64 n -> pr " \"K\""
          (* The following not yet implemented for callbacks XXX *)
          | ArrayAndLen _ | Bool _ | BytesIn _ | BytesOut _
-         | BytesPersistIn _ | BytesPersistOut _ | Callback _
+         | BytesPersistIn _ | BytesPersistOut _ | Callback _ | Flags _
          | Int _ | Int64 _ | Path _ | SockAddrAndLen _ | StringList _
          | UInt _ | UInt32 _ -> pr "  abort ();\n"
        ) args;
@@ -2978,7 +2996,7 @@ let print_python_binding name { args; ret }           |
UInt64 n -> pr ", %s" n
          (* The following not yet implemented for callbacks XXX *)
          | ArrayAndLen _ | Bool _ | BytesIn _ | BytesOut _
-         | BytesPersistIn _ | BytesPersistOut _ | Callback _
+         | BytesPersistIn _ | BytesPersistOut _ | Callback _ | Flags _
          | Int _ | Int64 _ | Path _ | SockAddrAndLen _ | StringList _
          | UInt _ | UInt32 _ -> pr "  abort ();\n"
        ) args;
@@ -3009,7 +3027,7 @@ let print_python_binding name { args; ret }           |
Opaque _ -> ()
          (* The following not yet implemented for callbacks XXX *)
          | ArrayAndLen _ | Bool _ | BytesIn _ | BytesOut _
-         | BytesPersistIn _ | BytesPersistOut _ | Callback _
+         | BytesPersistIn _ | BytesPersistOut _ | Callback _ | Flags _
          | Int _ | Int64 _ | Path _ | SockAddrAndLen _ | StringList _
          | UInt _ | UInt32 _ -> ()
        ) args;
@@ -3054,6 +3072,9 @@ let print_python_binding name { args; ret }         pr
"  struct py_aio_buffer *%s_buf;\n" n
     | Callback (n, _) ->
        pr "  struct %s_%s_data callback_data;\n" name n
+    | Flags n ->
+       pr "  uint32_t %s_u32;\n" n;
+       pr "  unsigned int %s = 0; /* really uint32_t */\n" n
     | Int n -> pr "  int %s;\n" n
     | Int64 n ->
        pr "  int64_t %s_i64;\n" n;
@@ -3091,6 +3112,7 @@ let print_python_binding name { args; ret }      |
BytesOut (_, count) -> pr " \"n\""
     | BytesPersistOut (_, count) -> pr " \"O\""
     | Callback (n, _) -> pr " \"O\""
+    | Flags n -> pr " \"|I\""
     | Int n -> pr " \"i\""
     | Int64 n -> pr " \"L\""
     | Opaque _ -> pr " \"O\""
@@ -3114,6 +3136,7 @@ let print_python_binding name { args; ret }      |
BytesPersistOut (n, _) -> pr ", &%s" n
     | BytesOut (_, count) -> pr ", &%s" count
     | Callback _ -> pr ", &callback_data.fn"
+    | Flags n -> pr ", &%s" n
     | Int n -> pr ", &%s" n
     | Int64 n -> pr ", &%s" n
     | Opaque n -> pr ", &%s" n
@@ -3162,6 +3185,7 @@ let print_python_binding name { args; ret }            n;
        pr "    return NULL;\n";
        pr "  }\n"
+    | Flags n -> pr "  %s_u32 = %s;\n" n n
     | Int _ -> ()
     | Int64 n -> pr "  %s_i64 = %s;\n" n n
     | Opaque n ->
@@ -3190,6 +3214,7 @@ let print_python_binding name { args; ret }      |
BytesPersistIn (n, _)
     | BytesPersistOut (n, _) -> pr ", %s_buf->data,
%s_buf->len" n n
     | Callback (n, _) -> pr ", %s_%s_wrapper" name n
+    | Flags n -> pr ", %s_u32" n
     | Int n -> pr ", %s" n
     | Int64 n -> pr ", %s_i64" n
     | Opaque _ -> pr ", &callback_data"
@@ -3223,6 +3248,7 @@ let print_python_binding name { args; ret }      | BytesIn
_
     | BytesPersistIn _ | BytesPersistOut _
     | Callback _
+    | Flags _
     | Int _
     | Int64 _
     | Opaque _
@@ -3264,6 +3290,7 @@ let print_python_binding name { args; ret }      | BytesIn
(n, _) -> pr "  PyBuffer_Release (&%s);\n" n
     | BytesPersistIn _ | BytesOut _ | BytesPersistOut _ -> ()
     | Callback _ -> ()
+    | Flags _ -> ()
     | Int _ -> ()
     | Int64 _ -> ()
     | Opaque _ -> ()
@@ -3359,6 +3386,7 @@ class NBD (object):
                      | BytesPersistOut (n, _) -> n
                      | BytesOut (_, count) -> count
                      | Callback (n, _) -> n
+                     | Flags n -> n ^ "=0"
                      | Int n -> n
                      | Int64 n -> n
                      | Opaque n -> n
diff --git a/python/t/400-pread.py b/python/t/400-pread.py
index da4799b..4d64166 100644
--- a/python/t/400-pread.py
+++ b/python/t/400-pread.py
@@ -20,7 +20,7 @@ import nbd
 h = nbd.NBD ()
 h.connect_command (["nbdkit", "-s",
"--exit-with-parent", "-v",
                     "pattern", "size=512"])
-buf = h.pread (512, 0, 0)
+buf = h.pread (512, 0)
 
 print ("%r" % buf)
 
diff --git a/python/t/410-pwrite.py b/python/t/410-pwrite.py
index 9152ba2..811f233 100644
--- a/python/t/410-pwrite.py
+++ b/python/t/410-pwrite.py
@@ -33,7 +33,7 @@ h = nbd.NBD ()
 h.connect_command (["nbdkit", "-s",
"--exit-with-parent", "-v",
                     "file", datafile])
 h.pwrite (buf1, 0, nbd.CMD_FLAG_FUA)
-buf2 = h.pread (512, 0, 0)
+buf2 = h.pread (512, 0)
 
 assert buf1 == buf2
 
diff --git a/python/t/460-block-status.py b/python/t/460-block-status.py
index f511091..eeaa7b2 100644
--- a/python/t/460-block-status.py
+++ b/python/t/460-block-status.py
@@ -34,14 +34,14 @@ def f (data, metacontext, offset, e):
         return
     entries = e
 
-h.block_status (65536, 0, 42, f, 0)
+h.block_status (65536, 0, 42, f)
 assert entries == [ 8192, 0,
                     8192, 1,
                    16384, 3,
                    16384, 2,
                    16384, 0]
 
-h.block_status (1024, 32256, 42, f, 0)
+h.block_status (1024, 32256, 42, f)
 print ("entries = %r" % entries)
 assert entries == [  512, 3,
                    16384, 2]
diff --git a/python/t/500-aio-pread.py b/python/t/500-aio-pread.py
index 82199ef..85342f5 100644
--- a/python/t/500-aio-pread.py
+++ b/python/t/500-aio-pread.py
@@ -21,7 +21,7 @@ h = nbd.NBD ()
 h.connect_command (["nbdkit", "-s",
"--exit-with-parent", "-v",
                     "pattern", "size=512"])
 buf = nbd.aio_buffer (512)
-id = h.aio_pread (buf, 0, 0)
+id = h.aio_pread (buf, 0)
 while not (h.aio_command_completed (id)):
     h.poll (-1)
 
diff --git a/python/t/510-aio-pwrite.py b/python/t/510-aio-pwrite.py
index b73464b..4770b83 100644
--- a/python/t/510-aio-pwrite.py
+++ b/python/t/510-aio-pwrite.py
@@ -39,7 +39,7 @@ while not (h.aio_command_completed (id)):
     h.poll (-1)
 
 buf2 = nbd.aio_buffer (512)
-id = h.aio_pread (buf2, 0, 0)
+id = h.aio_pread (buf2, 0)
 while not (h.aio_command_completed (id)):
     h.poll (-1)
 
-- 
2.21.0

On 5/28/19 7:57 AM, Richard W.M. Jones wrote:
> By using a special type we can more naturally express flags in
> different programming languages.  For example OCaml will prefer an
> optional argument containing a list of flags, defaulting to the empty
> list:
> 
>   pread [...] ?(flags = [])
> ---
>  generator/generator          | 66 +++++++++++++++++++++++++-----------
>  python/t/400-pread.py        |  2 +-
>  python/t/410-pwrite.py       |  2 +-
>  python/t/460-block-status.py |  4 +--
>  python/t/500-aio-pread.py    |  2 +-
>  python/t/510-aio-pwrite.py   |  2 +-
>  6 files changed, 53 insertions(+), 25 deletions(-)
Definitely nicer than my poor attempt at OCaml hacking :)

> @@ -2494,6 +2495,21 @@ let generate_lib_states_c () >  
>  (* Generate C API. *)
>  
> +(* Check the API definition. *)
> +let () > +  (* Flags must only appear once in the final argument
position. *)
> +  List.iter (
> +    fun (name, { args }) ->
> +      let args = List.rev args in
> +      match args with
> +      | [] -> ()
> +      | Flags _ :: xs
> +      | xs ->
> +         if List.exists (function Flags _ -> true | _ -> false) xs
then
> +           failwithf "%s: Flags must appear in final argument
position only"
> +                     name
> +  ) handle_calls
> +
And this part is nice (even if I'm having to read up on quite a bit of
documentation to understand how it works)

> +++ b/python/t/400-pread.py
> @@ -20,7 +20,7 @@ import nbd
>  h = nbd.NBD ()
>  h.connect_command (["nbdkit", "-s",
"--exit-with-parent", "-v",
>                      "pattern", "size=512"])
> -buf = h.pread (512, 0, 0)
> +buf = h.pread (512, 0)
At any rate, you achieved the same goal I had in mind for omitting a 0
flags argument in Python.
>  
>  print ("%r" % buf)
>  
> diff --git a/python/t/410-pwrite.py b/python/t/410-pwrite.py
> index 9152ba2..811f233 100644
> --- a/python/t/410-pwrite.py
> +++ b/python/t/410-pwrite.py
> @@ -33,7 +33,7 @@ h = nbd.NBD ()
>  h.connect_command (["nbdkit", "-s",
"--exit-with-parent", "-v",
>                      "file", datafile])
>  h.pwrite (buf1, 0, nbd.CMD_FLAG_FUA)
There's still the question of how to pass two flags at once; if I
understand your patch (and after confirming it with nbdsh), we have to
write:

h.zero (64*1024, 0, nbd.CMD_FLAG_FUA | nbd.CMD_FLAG_NO_HOLE)

[side note - requires a server that supports flush; to my surprise,
nbkdit 'null' does but 'memory' did not; I'll probably fix
that]

I don't know if we want to somehow allow:

h.zero (64*1024, 0, nbd.CMD_FLAG_FUA, nbd.CMD_FLAG_NO_HOLE)

instead (two arguments, each with one flag, which the glue code then
combines into a single integer; compared to the existing approach of
letting Python do the bitwise math and pass in a single integer up front).

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