Libguestfs - May 2019 - Re: [libnbd PATCH 2/4] api: Rearrange flags argument to block_status

RFC because this is an API break, but we haven't declared stable API
yet. If we like it, I'm working on using libnbd to implement the
nbdkit-nbd plugin; knowing whether it is API version 0.1 or 0.2 will
be useful. I also dabbled with allowing optional parameters in python,
although my OCaml is weak enough that there may be cleaner ways to
approach that.

Eric Blake (4):
  api: Add flags parameter to pread, trim, cache, disconnect
  api: Rearrange flags argument to block_status
  python: Let flags parameter be optional
  api: Add DF flag support for pread

 docs/libnbd.pod                      |   6 +-
 examples/batched-read-write.c        |   4 +-
 examples/simple-fetch-first-sector.c |   2 +-
 examples/simple-reads-and-writes.c   |   2 +-
 examples/threaded-reads-and-writes.c |   2 +-
 generator/generator                  | 160 ++++++++++++++++++++-------
 interop/dirty-bitmap.c               |   6 +-
 interop/interop.c                    |   2 +-
 lib/disconnect.c                     |  10 +-
 lib/flags.c                          |  11 ++
 lib/rw.c                             |  51 ++++++---
 python/t/460-block-status.py         |   8 +-
 tests/aio-parallel-load.c            |   2 +-
 tests/aio-parallel.c                 |   2 +-
 tests/connect-tls.c                  |   2 +-
 tests/errors.c                       |   2 +-
 tests/meta-base-allocation.c         |  10 +-
 tests/oldstyle.c                     |   2 +-
 tests/synch-parallel.c               |   2 +-
 19 files changed, 203 insertions(+), 83 deletions(-)

-- 
2.20.1

This is an API/ABI break, but we have not yet declared stable API.

The NBD spec allows clients to send FLAG_FUA on any command, even
though it recommends that FUA only be sent for write-like commands;
however, without strong reason, I don't see the point in letting
libnbd allow clients to send FLAG_FUA on non-write commands. The NBD
spec also adds the DF flag for reads from a server that supports
structured replies (which an upcoming commit will address), and may
add future flags.

Thinking towards the future, we normally want to refuse a flag that we
know the spec does not permit (for example, we already reject
nbd_pwrite(...,LIBNBD_CMD_FLAG_FUA) if the server did not advertise
FUA support), and the NBD spec tends to not allow new flags without
some form of handshaking to agree that both sides understand the new
flag. But we may also want to add a future API
nbd_set_experimental(nbd, true) as a way to allow a client to pass in
any flags for rapid prototyping against a server, without having to
wait for a new libnbd release that understands the new flag.

As such, all of our transmission-phase APIs should have a flags
parameter, even if callers must always set it to 0 for now.  Note that
in the case of disconnect, nbd_aio_disconnect gets the flag, but
nbd_shutdown remains a general purpose wrapper that works even when
not in transmission phase (that is, there is a reason why we did not
add a mere nbd_disconnect sync wrapper), so it does not need to get a
flag (if you need fine-grained control over shutdown, aio is
sufficient).

We may also want to consider teaching the generator about optional
parameters, and have flags default to 0 if not present in languages
that support that (python being the obvious candidate); sadly, C
varargs are insufficient for our purposes, as we'd have to have some
way to know whether or not to expect to parse a flags argument from a
va_list argument.
---
 docs/libnbd.pod                      |  6 +--
 examples/batched-read-write.c        |  4 +-
 examples/simple-fetch-first-sector.c |  2 +-
 examples/simple-reads-and-writes.c   |  2 +-
 examples/threaded-reads-and-writes.c |  2 +-
 generator/generator                  | 70 +++++++++++++++++++---------
 interop/interop.c                    |  2 +-
 lib/disconnect.c                     | 10 +++-
 lib/rw.c                             | 33 +++++++++----
 python/t/400-pread.py                |  2 +-
 python/t/410-pwrite.py               |  2 +-
 python/t/500-aio-pread.py            |  2 +-
 python/t/510-aio-pwrite.py           |  2 +-
 tests/aio-parallel-load.c            |  2 +-
 tests/aio-parallel.c                 |  2 +-
 tests/connect-tls.c                  |  2 +-
 tests/errors.c                       |  2 +-
 tests/oldstyle.c                     |  2 +-
 tests/synch-parallel.c               |  2 +-
 19 files changed, 99 insertions(+), 52 deletions(-)

diff --git a/docs/libnbd.pod b/docs/libnbd.pod
index b5708f4..1aba966 100644
--- a/docs/libnbd.pod
+++ b/docs/libnbd.pod
@@ -11,7 +11,7 @@ libnbd - network block device (NBD) client library in
userspace
  
  if ((nbd = nbd_create ()) == NULL ||
      nbd_connect_tcp (nbd, "server.example.com", "nbd") ==
-1 ||
-     nbd_pread (nbd, buf, sizeof buf, 0) == -1)
+     nbd_pread (nbd, buf, sizeof buf, 0, 0) == -1)
    fprintf (stderr, "%s\n", nbd_get_error ());
    exit (EXIT_FAILURE);
  }
@@ -91,7 +91,7 @@ Read the first sector (512 bytes) from the NBD export:

  char buf[512];
  
- if (nbd_pread (nbd, buf, sizeof buf, 0) == -1) {
+ if (nbd_pread (nbd, buf, sizeof buf, 0, 0) == -1) {
    fprintf (stderr, "%s\n", nbd_get_error ());
    exit (EXIT_FAILURE);
  }
@@ -156,7 +156,7 @@ has completed:
  int64_t id;
  char buf[512];
  
- id = nbd_aio_pread (nbd, buf, sizeof buf, offset)
+ id = nbd_aio_pread (nbd, buf, sizeof buf, offset, 0)
  if (id == -1) {
    fprintf (stderr, "%s\n", nbd_get_error ());
    exit (EXIT_FAILURE);
diff --git a/examples/batched-read-write.c b/examples/batched-read-write.c
index 300ca02..a59f5a0 100644
--- a/examples/batched-read-write.c
+++ b/examples/batched-read-write.c
@@ -54,7 +54,7 @@ try_deadlock (void *arg)

   /* Issue commands. */
   in_flight = 0;
-  handles[0] = nbd_aio_pread (nbd, in, packetsize, 0);
+  handles[0] = nbd_aio_pread (nbd, in, packetsize, 0, 0);
   if (handles[0] == -1) {
     fprintf (stderr, "%s\n", nbd_get_error ());
     goto error;
@@ -183,7 +183,7 @@ main (int argc, char *argv[])
   }

   /* Attempt to be non-destructive, by writing what file already contains */
-  if (nbd_pread (nbd, out, packetsize, packetsize) == -1) {
+  if (nbd_pread (nbd, out, packetsize, packetsize, 0) == -1) {
     fprintf (stderr, "sync read failed: %s\n", nbd_get_error ());
     exit (EXIT_FAILURE);
   }
diff --git a/examples/simple-fetch-first-sector.c
b/examples/simple-fetch-first-sector.c
index 7791d26..1778626 100644
--- a/examples/simple-fetch-first-sector.c
+++ b/examples/simple-fetch-first-sector.c
@@ -38,7 +38,7 @@ main (int argc, char *argv[])
     exit (EXIT_FAILURE);
   }

-  if (nbd_pread (nbd, buf, sizeof buf, 0) == -1) {
+  if (nbd_pread (nbd, buf, sizeof buf, 0, 0) == -1) {
     fprintf (stderr, "%s\n", nbd_get_error ());
     exit (EXIT_FAILURE);
   }
diff --git a/examples/simple-reads-and-writes.c
b/examples/simple-reads-and-writes.c
index fa8f48d..5ed5bb7 100644
--- a/examples/simple-reads-and-writes.c
+++ b/examples/simple-reads-and-writes.c
@@ -71,7 +71,7 @@ main (int argc, char *argv[])
   /* 1000 reads and writes. */
   for (i = 0; i < 1000; ++i) {
     offset = rand () % (exportsize - sizeof buf);
-    if (nbd_pread (nbd, buf, sizeof buf, offset) == -1) {
+    if (nbd_pread (nbd, buf, sizeof buf, offset, 0) == -1) {
       fprintf (stderr, "%s\n", nbd_get_error ());
       exit (EXIT_FAILURE);
     }
diff --git a/examples/threaded-reads-and-writes.c
b/examples/threaded-reads-and-writes.c
index 8e42d3a..b912e3b 100644
--- a/examples/threaded-reads-and-writes.c
+++ b/examples/threaded-reads-and-writes.c
@@ -257,7 +257,7 @@ start_thread (void *arg)
       if (cmd == 0)
         handle = nbd_aio_pwrite (nbd, buf, sizeof buf, offset, 0);
       else
-        handle = nbd_aio_pread (nbd, buf, sizeof buf, offset);
+        handle = nbd_aio_pread (nbd, buf, sizeof buf, offset, 0);
       if (handle == -1) {
         fprintf (stderr, "%s\n", nbd_get_error ());
         goto error;
diff --git a/generator/generator b/generator/generator
index 0a3db5e..9af5377 100755
--- a/generator/generator
+++ b/generator/generator
@@ -1219,7 +1219,7 @@ with the server.";

   "pread", {
     default_call with
-    args = [ BytesOut ("buf", "count"); UInt64
"offset" ];
+    args = [ BytesOut ("buf", "count"); UInt64
"offset"; UInt32 "flags" ];
     ret = RErr;
     shortdesc = "read from the NBD server";
     longdesc = "\
@@ -1227,7 +1227,10 @@ Issue a read command to the NBD server for the range
starting
 at C<offset> and ending at C<offset> + C<count> - 1.  NBD
 can only read all or nothing using this call.  The call
 returns when the data has been read fully into C<buf> or there is an
-error.";
+error.
+
+The C<flags> parameter must be C<0> for now (it exists for future
NBD
+protocol extensions).";
   };

   "pwrite", {
@@ -1257,18 +1260,26 @@ C<nbd_can_fua>).";
 Issue the disconnect command to the NBD server.  This is
 a nice way to tell the server we are going away, but from the
 client's point of view has no advantage over abruptly closing
-the connection (see C<nbd_close>).";
+the connection (see C<nbd_close>).
+
+This function works whether or not the handle is ready for
+transmission commands, and as such does not take a C<flags>
+parameter. If more fine-grained control is needed, see
+C<nbd_aio_disconnect>.";
   };

   "flush", {
     default_call with
-    args = []; ret = RErr;
+    args = [ UInt32 "flags" ]; ret = RErr;
     shortdesc = "flushing pending write requests";
     longdesc = "\
 Issue the flush command to the NBD server.  The function should
 return when all write commands which have completed have been
 committed to permanent storage on the server.  Note this will
-return an error if C<nbd_can_flush> is false.";
+return an error if C<nbd_can_flush> is false.
+
+The C<flags> parameter must be C<0> for now (it exists for future
NBD
+protocol extensions).";
   };

   "trim", {
@@ -1292,7 +1303,7 @@ C<nbd_can_fua>).";

   "cache", {
     default_call with
-    args = [ UInt64 "count"; UInt64 "offset" ];
+    args = [ UInt64 "count"; UInt64 "offset"; UInt32
"flags" ];
     ret = RErr;
     shortdesc = "send cache (prefetch) to the NBD server";
     longdesc = "\
@@ -1301,7 +1312,10 @@ if supported by the server causes data to be prefetched
 into faster storage by the server, speeding up a subsequent
 C<nbd_pread> call.  The server can also silently ignore
 this command.  Note this will return an error if
-C<nbd_can_cache> is false.";
+C<nbd_can_cache> is false.
+
+The C<flags> parameter must be C<0> for now (it exists for future
NBD
+protocol extensions).";
   };

   "zero", {
@@ -1461,7 +1475,8 @@ on the connection.";

   "aio_pread", {
     default_call with
-    args = [ BytesPersistOut ("buf", "count"); UInt64
"offset" ];
+    args = [ BytesPersistOut ("buf", "count"); UInt64
"offset";
+             UInt32 "flags" ];
     ret = RInt64;
     shortdesc = "read from the NBD server";
     longdesc = "\
@@ -1469,7 +1484,8 @@ Issue a read command to the NBD server.  This returns the
 unique positive 64 bit handle for this command, or C<-1> on
 error.  To check if the command completed, call
 C<nbd_aio_command_completed>.  Note that you must ensure
-C<buf> is valid until the command has completed.";
+C<buf> is valid until the command has completed.  Other
+parameters behave as documented in C<nbd_pread>.";
   };

   "aio_pwrite", {
@@ -1482,29 +1498,35 @@ Issue a write command to the NBD server.  This returns
the
 unique positive 64 bit handle for this command, or C<-1> on
 error.  To check if the command completed, call
 C<nbd_aio_command_completed>.  Note that you must ensure
-C<buf> is valid until the command has completed.";
+C<buf> is valid until the command has completed.  Other
+parameters behave as documented in C<nbd_pwrite>.";
   };

   "aio_disconnect", {
     default_call with
-    args = []; ret = RErr;
+    args = [ UInt32 "flags" ]; ret = RErr;
     shortdesc = "disconnect from the NBD server";
     longdesc = "\
 Issue the disconnect command to the NBD server.  This is
 not a normal command because NBD servers are not obliged
 to send a reply.  Instead you should wait for
-C<nbd_aio_is_closed> to become true on the connection.";
+C<nbd_aio_is_closed> to become true on the connection.
+
+The C<flags> parameter must be C<0> for now (it exists for future
NBD
+protocol extensions).  There is no direct synchronous counterpart;
+however, C<nbd_shutdown> will call this function if appropriate.";
   };

   "aio_flush", {
     default_call with
-    args = []; ret = RInt64;
+    args = [ UInt32 "flags" ]; ret = RInt64;
     shortdesc = "send flush command to the NBD server";
     longdesc = "\
 Issue the flush command to the NBD server.  This returns the
 unique positive 64 bit handle for this command, or C<-1> on
 error.  To check if the command completed, call
-C<nbd_aio_command_completed>.";
+C<nbd_aio_command_completed>.  Parameters behave as documented
+in C<nbd_flush>.";
   };

   "aio_trim", {
@@ -1516,19 +1538,21 @@ C<nbd_aio_command_completed>.";
 Issue a trim command to the NBD server.  This returns the
 unique positive 64 bit handle for this command, or C<-1> on
 error.  To check if the command completed, call
-C<nbd_aio_command_completed>.";
+C<nbd_aio_command_completed>.  Parameters behave as documented
+in C<nbd_trim>.";
   };

   "aio_cache", {
     default_call with
-    args = [ UInt64 "count"; UInt64 "offset" ];
+    args = [ UInt64 "count"; UInt64 "offset"; UInt32
"flags" ];
     ret = RInt64;
     shortdesc = "send cache (prefetch) to the NBD server";
     longdesc = "\
 Issue the cache (prefetch) command to the NBD server.  This
 returns the unique positive 64 bit handle for this command, or
 C<-1> on error.  To check if the command completed, call
-C<nbd_aio_command_completed>.";
+C<nbd_aio_command_completed>.  Parameters behave as documented
+in C<nbd_cache>.";
   };

   "aio_zero", {
@@ -1540,7 +1564,8 @@ C<nbd_aio_command_completed>.";
 Issue a write zeroes command to the NBD server.  This returns the
 unique positive 64 bit handle for this command, or C<-1> on
 error.  To check if the command completed, call
-C<nbd_aio_command_completed>.";
+C<nbd_aio_command_completed>.  Parameters behave as documented
+in C<nbd_zero>.";
   };

   "aio_block_status", {
@@ -1557,7 +1582,8 @@ C<nbd_aio_command_completed>.";
 Send the block status command to the NBD server.  This returns the
 unique positive 64 bit handle for this command, or C<-1> on
 error.  To check if the command completed, call
-C<nbd_aio_command_completed>.";
+C<nbd_aio_command_completed>.  Parameters behave as documented
+in C<nbd_block_status>.";
   };

   "aio_get_fd", {
@@ -2697,7 +2723,7 @@ libnbd-api - libnbd C API
  
  if ((nbd = nbd_create ()) == NULL ||
      nbd_connect_tcp (nbd, \"server.example.com\", \"nbd\")
== -1 ||
-     nbd_pread (nbd, buf, sizeof buf, 0) == -1)
+     nbd_pread (nbd, buf, sizeof buf, 0, 0) == -1)
    fprintf (stderr, \"%%s\\n\", nbd_get_error ());
    exit (EXIT_FAILURE);
  }
@@ -3276,7 +3302,7 @@ Python bindings for libnbd
 import nbd
 h = nbd.NBD ()
 h.connect_tcp (\"localhost\", \"nbd\")
-buf = h.pread (512, 0)
+buf = h.pread (512, 0, 0)

 Read the libnbd(3) man page to find out how to use the API.
 '''
@@ -3377,7 +3403,7 @@ is an open NBD handle called ‘h’.

 h.connect_tcp (\"remote\", \"10809\")   # Connect to a
remote server.
 h.get_size ()                       # Get size of the remote disk.
-buf = h.pread (512, 0)              # Read the first sector.
+buf = h.pread (512, 0, 0)           # Read the first sector.
 exit() or Ctrl-D                    # Quit the shell
 help (nbd)                          # Display documentation
 '''
diff --git a/interop/interop.c b/interop/interop.c
index aed8581..24f79cc 100644
--- a/interop/interop.c
+++ b/interop/interop.c
@@ -138,7 +138,7 @@ main (int argc, char *argv[])
     goto out;
   }

-  if (nbd_pread (nbd, buf, sizeof buf, 0) == -1) {
+  if (nbd_pread (nbd, buf, sizeof buf, 0, 0) == -1) {
     fprintf (stderr, "%s\n", nbd_get_error ());
     goto out;
   }
diff --git a/lib/disconnect.c b/lib/disconnect.c
index 23e95dd..6695e59 100644
--- a/lib/disconnect.c
+++ b/lib/disconnect.c
@@ -22,6 +22,7 @@
 #include <stdlib.h>
 #include <stdbool.h>
 #include <errno.h>
+#include <inttypes.h>

 #include "internal.h"

@@ -31,7 +32,7 @@ nbd_unlocked_shutdown (struct nbd_handle *h)

   if (nbd_unlocked_aio_is_ready (h) ||
       nbd_unlocked_aio_is_processing (h)) {
-    if (nbd_unlocked_aio_disconnect (h) == -1)
+    if (nbd_unlocked_aio_disconnect (h, 0) == -1)
       return -1;
   }

@@ -45,10 +46,15 @@ nbd_unlocked_shutdown (struct nbd_handle *h)
 }

 int
-nbd_unlocked_aio_disconnect (struct nbd_handle *h)
+nbd_unlocked_aio_disconnect (struct nbd_handle *h, uint32_t flags)
 {
   int64_t id;

+  if (flags != 0) {
+    set_error (EINVAL, "invalid flag: %" PRIu32, flags);
+    return -1;
+  }
+
   id = nbd_internal_command_common (h, 0, NBD_CMD_DISC, 0, 0, NULL, NULL);
   if (id == -1)
     return -1;
diff --git a/lib/rw.c b/lib/rw.c
index 0183fc1..bd453d1 100644
--- a/lib/rw.c
+++ b/lib/rw.c
@@ -44,11 +44,11 @@ wait_for_command (struct nbd_handle *h, int64_t handle)
 /* Issue a read command and wait for the reply. */
 int
 nbd_unlocked_pread (struct nbd_handle *h, void *buf,
-                    size_t count, uint64_t offset)
+                    size_t count, uint64_t offset, uint32_t flags)
 {
   int64_t ch;

-  ch = nbd_unlocked_aio_pread (h, buf, count, offset);
+  ch = nbd_unlocked_aio_pread (h, buf, count, offset, flags);
   if (ch == -1)
     return -1;

@@ -71,11 +71,11 @@ nbd_unlocked_pwrite (struct nbd_handle *h, const void *buf,

 /* Issue a flush command and wait for the reply. */
 int
-nbd_unlocked_flush (struct nbd_handle *h)
+nbd_unlocked_flush (struct nbd_handle *h, uint32_t flags)
 {
   int64_t ch;

-  ch = nbd_unlocked_aio_flush (h);
+  ch = nbd_unlocked_aio_flush (h, flags);
   if (ch == -1)
     return -1;

@@ -99,11 +99,11 @@ nbd_unlocked_trim (struct nbd_handle *h,
 /* Issue a cache command and wait for the reply. */
 int
 nbd_unlocked_cache (struct nbd_handle *h,
-                    uint64_t count, uint64_t offset)
+                    uint64_t count, uint64_t offset, uint32_t flags)
 {
   int64_t ch;

-  ch = nbd_unlocked_aio_cache (h, count, offset);
+  ch = nbd_unlocked_aio_cache (h, count, offset, flags);
   if (ch == -1)
     return -1;

@@ -233,8 +233,13 @@ nbd_internal_command_common (struct nbd_handle *h,

 int64_t
 nbd_unlocked_aio_pread (struct nbd_handle *h, void *buf,
-                        size_t count, uint64_t offset)
+                        size_t count, uint64_t offset, uint32_t flags)
 {
+  if (flags != 0) {
+    set_error (EINVAL, "invalid flag: %" PRIu32, flags);
+    return -1;
+  }
+
   return nbd_internal_command_common (h, 0, NBD_CMD_READ, offset, count,
                                       buf, NULL);
 }
@@ -265,13 +270,18 @@ nbd_unlocked_aio_pwrite (struct nbd_handle *h, const void
*buf,
 }

 int64_t
-nbd_unlocked_aio_flush (struct nbd_handle *h)
+nbd_unlocked_aio_flush (struct nbd_handle *h, uint32_t flags)
 {
   if (nbd_unlocked_can_flush (h) != 1) {
     set_error (EINVAL, "server does not support flush operations");
     return -1;
   }

+  if (flags != 0) {
+    set_error (EINVAL, "invalid flag: %" PRIu32, flags);
+    return -1;
+  }
+
   return nbd_internal_command_common (h, 0, NBD_CMD_FLUSH, 0, 0,
                                       NULL, NULL);
 }
@@ -308,7 +318,7 @@ nbd_unlocked_aio_trim (struct nbd_handle *h,

 int64_t
 nbd_unlocked_aio_cache (struct nbd_handle *h,
-                        uint64_t count, uint64_t offset)
+                        uint64_t count, uint64_t offset, uint32_t flags)
 {
   /* Actually according to the NBD protocol document, servers do exist
    * that support NBD_CMD_CACHE but don't advertise the
@@ -319,6 +329,11 @@ nbd_unlocked_aio_cache (struct nbd_handle *h,
     return -1;
   }

+  if (flags != 0) {
+    set_error (EINVAL, "invalid flag: %" PRIu32, flags);
+    return -1;
+  }
+
   return nbd_internal_command_common (h, 0, NBD_CMD_CACHE, offset, count,
                                       NULL, NULL);
 }
diff --git a/python/t/400-pread.py b/python/t/400-pread.py
index 4d64166..da4799b 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)
+buf = h.pread (512, 0, 0)

 print ("%r" % buf)

diff --git a/python/t/410-pwrite.py b/python/t/410-pwrite.py
index 811f233..9152ba2 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)
+buf2 = h.pread (512, 0, 0)

 assert buf1 == buf2

diff --git a/python/t/500-aio-pread.py b/python/t/500-aio-pread.py
index 85342f5..82199ef 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)
+id = h.aio_pread (buf, 0, 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 4770b83..b73464b 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)
+id = h.aio_pread (buf2, 0, 0)
 while not (h.aio_command_completed (id)):
     h.poll (-1)

diff --git a/tests/aio-parallel-load.c b/tests/aio-parallel-load.c
index d1422f3..20fa358 100644
--- a/tests/aio-parallel-load.c
+++ b/tests/aio-parallel-load.c
@@ -257,7 +257,7 @@ start_thread (void *arg)
         status->bytes_sent += sizeof buf;
       }
       else {
-        handle = nbd_aio_pread (nbd, buf, sizeof buf, offset);
+        handle = nbd_aio_pread (nbd, buf, sizeof buf, offset, 0);
         status->bytes_received += sizeof buf;
       }
       if (handle == -1) {
diff --git a/tests/aio-parallel.c b/tests/aio-parallel.c
index 70687f1..7c62ba3 100644
--- a/tests/aio-parallel.c
+++ b/tests/aio-parallel.c
@@ -296,7 +296,7 @@ start_thread (void *arg)
         memcpy (&ramdisk[offset], buf, BUFFERSIZE);
       }
       else {
-        handle = nbd_aio_pread (nbd, buf, BUFFERSIZE, offset);
+        handle = nbd_aio_pread (nbd, buf, BUFFERSIZE, offset, 0);
         status->bytes_received += BUFFERSIZE;
       }
       if (handle == -1) {
diff --git a/tests/connect-tls.c b/tests/connect-tls.c
index 0b13231..b49a29f 100644
--- a/tests/connect-tls.c
+++ b/tests/connect-tls.c
@@ -83,7 +83,7 @@ main (int argc, char *argv[])
     exit (EXIT_FAILURE);
   }

-  if (nbd_pread (nbd, buf, sizeof buf, 0) == -1) {
+  if (nbd_pread (nbd, buf, sizeof buf, 0, 0) == -1) {
     fprintf (stderr, "%s\n", nbd_get_error ());
     exit (EXIT_FAILURE);
   }
diff --git a/tests/errors.c b/tests/errors.c
index 25b82bb..99d5820 100644
--- a/tests/errors.c
+++ b/tests/errors.c
@@ -41,7 +41,7 @@ main (int argc, char *argv[])
   }

   /* Issue a connected command when not connected. */
-  if (nbd_pread (nbd, NULL, 0, 0) != -1) {
+  if (nbd_pread (nbd, NULL, 0, 0, 0) != -1) {
     fprintf (stderr, "%s: test failed: "
              "nbd_pread did not fail on non-connected handle\n",
              argv[0]);
diff --git a/tests/oldstyle.c b/tests/oldstyle.c
index 821ab98..a0b594c 100644
--- a/tests/oldstyle.c
+++ b/tests/oldstyle.c
@@ -66,7 +66,7 @@ main (int argc, char *argv[])
     exit (EXIT_FAILURE);
   }

-  if (nbd_pread (nbd, rbuf, sizeof rbuf, 0) == -1) {
+  if (nbd_pread (nbd, rbuf, sizeof rbuf, 0, 0) == -1) {
     fprintf (stderr, "%s\n", nbd_get_error ());
     exit (EXIT_FAILURE);
   }
diff --git a/tests/synch-parallel.c b/tests/synch-parallel.c
index 181e369..387a96f 100644
--- a/tests/synch-parallel.c
+++ b/tests/synch-parallel.c
@@ -224,7 +224,7 @@ start_thread (void *arg)
       memcpy (&ramdisk[offset], buf, sizeof buf);
     }
     else {
-      if (nbd_pread (nbd, buf, sizeof buf, offset) == -1) {
+      if (nbd_pread (nbd, buf, sizeof buf, offset, 0) == -1) {
         fprintf (stderr, "%s\n", nbd_get_error ());
         goto error;
       }
-- 
2.20.1

This is an API/ABI break, but we have not yet declared stable API.

In order to use optional arguments in languages that support that, any
optional arguments must be last. Let's be consistent in this by
rearranging the arguments to block_status so that flags is always
last.
---
 generator/generator          | 10 ++++++----
 interop/dirty-bitmap.c       |  6 +++---
 lib/rw.c                     | 12 +++++-------
 python/t/460-block-status.py |  8 ++++++--
 tests/meta-base-allocation.c | 10 ++++------
 5 files changed, 24 insertions(+), 22 deletions(-)

diff --git a/generator/generator b/generator/generator
index 9af5377..8e81d8f 100755
--- a/generator/generator
+++ b/generator/generator
@@ -1341,12 +1341,13 @@ punching a hole.";

   "block_status", {
     default_call with
-    args = [ UInt64 "count"; UInt64 "offset"; UInt32
"flags";
+    args = [ UInt64 "count"; UInt64 "offset";
              Opaque "data";
              Callback ("extent", [Opaque "data"; String
"metacontext";
                                   UInt64 "offset";
                                   ArrayAndLen (UInt32 "entries",
-                                               "nr_entries")]) ];
+                                               "nr_entries")]);
+             UInt32 "flags" ];
     ret = RErr;
     shortdesc = "read the block status of the given range";
     longdesc = "\
@@ -1570,12 +1571,13 @@ in C<nbd_zero>.";

   "aio_block_status", {
     default_call with
-    args = [ UInt64 "count"; UInt64 "offset"; UInt32
"flags";
+    args = [ UInt64 "count"; UInt64 "offset";
              Opaque "data";
              Callback ("extent", [Opaque "data"; String
"metacontext";
                                   UInt64 "offset";
                                   ArrayAndLen (UInt32 "entries",
-                                               "nr_entries")]) ];
+                                               "nr_entries")]);
+             UInt32 "flags" ];
     ret = RInt64;
     shortdesc = "send block status to the NBD server";
     longdesc = "\
diff --git a/interop/dirty-bitmap.c b/interop/dirty-bitmap.c
index 0a808b2..e096868 100644
--- a/interop/dirty-bitmap.c
+++ b/interop/dirty-bitmap.c
@@ -106,13 +106,13 @@ main (int argc, char *argv[])
     exit (EXIT_FAILURE);
   }

-  if (nbd_block_status (nbd, exportsize, 0, 0, NULL, cb) == -1) {
+  if (nbd_block_status (nbd, exportsize, 0, NULL, cb, 0) == -1) {
     fprintf (stderr, "%s\n", nbd_get_error ());
     exit (EXIT_FAILURE);
   }
   assert (calls == 0x11);
-  if (nbd_block_status (nbd, exportsize, 0, LIBNBD_CMD_FLAG_REQ_ONE,
-                        &exportsize, cb) == -1) {
+  if (nbd_block_status (nbd, exportsize, 0, &exportsize, cb,
+                        LIBNBD_CMD_FLAG_REQ_ONE) == -1) {
     fprintf (stderr, "%s\n", nbd_get_error ());
     exit (EXIT_FAILURE);
   }
diff --git a/lib/rw.c b/lib/rw.c
index bd453d1..feaf468 100644
--- a/lib/rw.c
+++ b/lib/rw.c
@@ -127,13 +127,12 @@ nbd_unlocked_zero (struct nbd_handle *h,
 /* Issue a block status command and wait for the reply. */
 int
 nbd_unlocked_block_status (struct nbd_handle *h,
-                           uint64_t count, uint64_t offset, uint32_t flags,
-                           void *data,
-                           extent_fn extent)
+                           uint64_t count, uint64_t offset,
+                           void *data, extent_fn extent, uint32_t flags)
 {
   int64_t ch;

-  ch = nbd_unlocked_aio_block_status (h, count, offset, flags, data, extent);
+  ch = nbd_unlocked_aio_block_status (h, count, offset, data, extent, flags);
   if (ch == -1)
     return -1;

@@ -371,9 +370,8 @@ nbd_unlocked_aio_zero (struct nbd_handle *h,
 int64_t
 nbd_unlocked_aio_block_status (struct nbd_handle *h,
                                uint64_t count, uint64_t offset,
-                               uint32_t flags,
-                               void *data,
-                               extent_fn extent)
+                               void *data, extent_fn extent,
+                               uint32_t flags)
 {
   if (!h->structured_replies) {
     set_error (ENOTSUP, "server does not support structured
replies");
diff --git a/python/t/460-block-status.py b/python/t/460-block-status.py
index c141a79..815caf0 100644
--- a/python/t/460-block-status.py
+++ b/python/t/460-block-status.py
@@ -34,14 +34,18 @@ def f (data, metacontext, offset, e):
         return
     entries = e

-h.block_status (65536, 0, 0, 42, f)
+h.block_status (65536, 0, 42, f, 0)
 assert entries == [ 8192, 0,
                     8192, 1,
                    16384, 3,
                    16384, 2,
                    16384, 0]

-h.block_status (1024, 32256, 0, 42, f)
+h.block_status (1024, 32256, 42, f, 0)
 print ("entries = %r" % entries)
 assert entries == [  512, 3,
                    16384, 2]
+
+h.block_status (1024, 32256, 42, f, nbd.CMD_FLAG_REQ_ONE)
+print ("entries = %r" % entries)
+assert entries == [  512, 3]
diff --git a/tests/meta-base-allocation.c b/tests/meta-base-allocation.c
index 5c37e45..ce344d7 100644
--- a/tests/meta-base-allocation.c
+++ b/tests/meta-base-allocation.c
@@ -98,22 +98,20 @@ main (int argc, char *argv[])

   /* Read the block status. */
   id = 1;
-  if (nbd_block_status (nbd, 65536, 0, 0, &id,
-                        check_extent) == -1) {
+  if (nbd_block_status (nbd, 65536, 0, &id, check_extent, 0) == -1) {
     fprintf (stderr, "%s\n", nbd_get_error ());
     exit (EXIT_FAILURE);
   }

   id = 2;
-  if (nbd_block_status (nbd, 1024, 32768-512, 0, &id,
-                        check_extent) == -1) {
+  if (nbd_block_status (nbd, 1024, 32768-512, &id, check_extent, 0) == -1)
{
     fprintf (stderr, "%s\n", nbd_get_error ());
     exit (EXIT_FAILURE);
   }

   id = 3;
-  if (nbd_block_status (nbd, 1024, 32768-512, LIBNBD_CMD_FLAG_REQ_ONE, &id,
-                        check_extent) == -1) {
+  if (nbd_block_status (nbd, 1024, 32768-512, &id, check_extent,
+                        LIBNBD_CMD_FLAG_REQ_ONE) == -1) {
     fprintf (stderr, "%s\n", nbd_get_error ());
     exit (EXIT_FAILURE);
   }
-- 
2.20.1

No need to require the user to pass in 0 for flags when the language
has the ability for a default parameter.

This approach works because we never have more than one optional
argument per function, but should we need more optional arguments,
we'd probably need to teach the generator to split type call's args
into mandatory and optional args, as python's PyArg_ParseTuple wants
only a single | separating mandatory from optional arguments.
---
 generator/generator        | 89 +++++++++++++++++++++++++++-----------
 python/t/400-pread.py      |  2 +-
 python/t/410-pwrite.py     |  2 +-
 python/t/500-aio-pread.py  |  2 +-
 python/t/510-aio-pwrite.py |  2 +-
 5 files changed, 67 insertions(+), 30 deletions(-)

diff --git a/generator/generator b/generator/generator
index 8e81d8f..919341c 100755
--- a/generator/generator
+++ b/generator/generator
@@ -823,6 +823,7 @@ and arg  | Int of string            (* small int *)
 | Int64 of string          (* 64 bit signed int *)
 | Opaque of string         (* opaque object, void* in C *)
+| OptUInt32 of string      (* optional 32 bit unsigned int, default 0 *)
 | Path of string           (* filename or path *)
 | SockAddrAndLen of string * string (* struct sockaddr * + socklen_t *)
 | String of string         (* string *)
@@ -1219,7 +1220,7 @@ with the server.";

   "pread", {
     default_call with
-    args = [ BytesOut ("buf", "count"); UInt64
"offset"; UInt32 "flags" ];
+    args = [ BytesOut ("buf", "count"); UInt64
"offset"; OptUInt32 "flags" ];
     ret = RErr;
     shortdesc = "read from the NBD server";
     longdesc = "\
@@ -1235,7 +1236,7 @@ protocol extensions).";

   "pwrite", {
     default_call with
-    args = [ BytesIn ("buf", "count"); UInt64
"offset"; UInt32 "flags" ];
+    args = [ BytesIn ("buf", "count"); UInt64
"offset"; OptUInt32 "flags" ];
     ret = RErr;
     shortdesc = "write to the NBD server";
     longdesc = "\
@@ -1270,7 +1271,7 @@ C<nbd_aio_disconnect>.";

   "flush", {
     default_call with
-    args = [ UInt32 "flags" ]; ret = RErr;
+    args = [ OptUInt32 "flags" ]; ret = RErr;
     shortdesc = "flushing pending write requests";
     longdesc = "\
 Issue the flush command to the NBD server.  The function should
@@ -1284,7 +1285,7 @@ protocol extensions).";

   "trim", {
     default_call with
-    args = [ UInt64 "count"; UInt64 "offset"; UInt32
"flags" ];
+    args = [ UInt64 "count"; UInt64 "offset"; OptUInt32
"flags" ];
     ret = RErr;
     shortdesc = "send trim to the NBD server";
     longdesc = "\
@@ -1303,7 +1304,7 @@ C<nbd_can_fua>).";

   "cache", {
     default_call with
-    args = [ UInt64 "count"; UInt64 "offset"; UInt32
"flags" ];
+    args = [ UInt64 "count"; UInt64 "offset"; OptUInt32
"flags" ];
     ret = RErr;
     shortdesc = "send cache (prefetch) to the NBD server";
     longdesc = "\
@@ -1320,7 +1321,7 @@ protocol extensions).";

   "zero", {
     default_call with
-    args = [ UInt64 "count"; UInt64 "offset"; UInt32
"flags" ];
+    args = [ UInt64 "count"; UInt64 "offset"; OptUInt32
"flags" ];
     ret = RErr;
     shortdesc = "send write zeroes to the NBD server";
     longdesc = "\
@@ -1347,7 +1348,7 @@ punching a hole.";
                                   UInt64 "offset";
                                   ArrayAndLen (UInt32 "entries",
                                                "nr_entries")]);
-             UInt32 "flags" ];
+             OptUInt32 "flags" ];
     ret = RErr;
     shortdesc = "read the block status of the given range";
     longdesc = "\
@@ -1477,7 +1478,7 @@ on the connection.";
   "aio_pread", {
     default_call with
     args = [ BytesPersistOut ("buf", "count"); UInt64
"offset";
-             UInt32 "flags" ];
+             OptUInt32 "flags" ];
     ret = RInt64;
     shortdesc = "read from the NBD server";
     longdesc = "\
@@ -1491,7 +1492,8 @@ 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";
+             OptUInt32 "flags" ];
     ret = RInt64;
     shortdesc = "write to the NBD server";
     longdesc = "\
@@ -1505,7 +1507,7 @@ parameters behave as documented in
C<nbd_pwrite>.";

   "aio_disconnect", {
     default_call with
-    args = [ UInt32 "flags" ]; ret = RErr;
+    args = [ OptUInt32 "flags" ]; ret = RErr;
     shortdesc = "disconnect from the NBD server";
     longdesc = "\
 Issue the disconnect command to the NBD server.  This is
@@ -1520,7 +1522,7 @@ however, C<nbd_shutdown> will call this function if
appropriate.";

   "aio_flush", {
     default_call with
-    args = [ UInt32 "flags" ]; ret = RInt64;
+    args = [ OptUInt32 "flags" ]; ret = RInt64;
     shortdesc = "send flush command to the NBD server";
     longdesc = "\
 Issue the flush command to the NBD server.  This returns the
@@ -1532,7 +1534,7 @@ in C<nbd_flush>.";

   "aio_trim", {
     default_call with
-    args = [ UInt64 "count"; UInt64 "offset"; UInt32
"flags" ];
+    args = [ UInt64 "count"; UInt64 "offset"; OptUInt32
"flags" ];
     ret = RInt64;
     shortdesc = "send trim to the NBD server";
     longdesc = "\
@@ -1545,7 +1547,7 @@ in C<nbd_trim>.";

   "aio_cache", {
     default_call with
-    args = [ UInt64 "count"; UInt64 "offset"; UInt32
"flags" ];
+    args = [ UInt64 "count"; UInt64 "offset"; OptUInt32
"flags" ];
     ret = RInt64;
     shortdesc = "send cache (prefetch) to the NBD server";
     longdesc = "\
@@ -1558,7 +1560,7 @@ in C<nbd_cache>.";

   "aio_zero", {
     default_call with
-    args = [ UInt64 "count"; UInt64 "offset"; UInt32
"flags" ];
+    args = [ UInt64 "count"; UInt64 "offset"; OptUInt32
"flags" ];
     ret = RInt64;
     shortdesc = "send write zeroes to the NBD server";
     longdesc = "\
@@ -1577,7 +1579,7 @@ in C<nbd_zero>.";
                                   UInt64 "offset";
                                   ArrayAndLen (UInt32 "entries",
                                                "nr_entries")]);
-             UInt32 "flags" ];
+             OptUInt32 "flags" ];
     ret = RInt64;
     shortdesc = "send block status to the NBD server";
     longdesc = "\
@@ -2514,6 +2516,7 @@ let rec name_of_arg = function
 | Int n -> [n]
 | Int64 n -> [n]
 | Opaque n -> [n]
+| OptUInt32 n -> [n]
 | Path n -> [n]
 | SockAddrAndLen (n, len) -> [n; len]
 | String n -> [n]
@@ -2545,6 +2548,7 @@ let rec print_c_arg_list ?(handle = false) args        |
Int n -> pr "int %s" n
       | Int64 n -> pr "int64_t %s" n
       | Opaque n -> pr "void *%s" n
+      | OptUInt32 n -> pr "uint32_t %s" n
       | Path n
       | String n -> pr "const char *%s" n
       | StringList n -> pr "char **%s" n
@@ -2945,8 +2949,8 @@ let print_python_binding name { args; ret }           (*
The following not yet implemented for callbacks XXX *)
          | ArrayAndLen _ | Bool _ | BytesIn _ | BytesOut _
          | BytesPersistIn _ | BytesPersistOut _ | Callback _
-         | Int _ | Int64 _ | Path _ | SockAddrAndLen _ | StringList _
-         | UInt _ | UInt32 _ -> pr "  abort ();\n"
+         | Int _ | Int64 _ | OptUInt32 _ | Path _ | SockAddrAndLen _
+         | StringList _ | UInt _ | UInt32 _ -> pr "  abort ();\n"
        ) args;
        pr "\n";

@@ -2960,8 +2964,8 @@ let print_python_binding name { args; ret }           (*
The following not yet implemented for callbacks XXX *)
          | ArrayAndLen _ | Bool _ | BytesIn _ | BytesOut _
          | BytesPersistIn _ | BytesPersistOut _ | Callback _
-         | Int _ | Int64 _ | Path _ | SockAddrAndLen _ | StringList _
-         | UInt _ | UInt32 _ -> pr "  abort ();\n"
+         | Int _ | Int64 _ | OptUInt32 _ | Path _ | SockAddrAndLen _
+         | StringList _ | UInt _ | UInt32 _ -> pr "  abort ();\n"
        ) args;
        pr " \")\"";
        List.iter (
@@ -2973,8 +2977,8 @@ let print_python_binding name { args; ret }           (*
The following not yet implemented for callbacks XXX *)
          | ArrayAndLen _ | Bool _ | BytesIn _ | BytesOut _
          | BytesPersistIn _ | BytesPersistOut _ | Callback _
-         | Int _ | Int64 _ | Path _ | SockAddrAndLen _ | StringList _
-         | UInt _ | UInt32 _ -> pr "  abort ();\n"
+         | Int _ | Int64 _ | OptUInt32 _ | Path _ | SockAddrAndLen _
+         | StringList _ | UInt _ | UInt32 _ -> pr "  abort ();\n"
        ) args;
        pr ");\n";
        pr "  Py_INCREF (py_args);\n";
@@ -3004,8 +3008,8 @@ let print_python_binding name { args; ret }           (*
The following not yet implemented for callbacks XXX *)
          | ArrayAndLen _ | Bool _ | BytesIn _ | BytesOut _
          | BytesPersistIn _ | BytesPersistOut _ | Callback _
-         | Int _ | Int64 _ | Path _ | SockAddrAndLen _ | StringList _
-         | UInt _ | UInt32 _ -> ()
+         | Int _ | Int64 _ | OptUInt32 _ | Path _ | SockAddrAndLen _
+         | StringList _ | UInt _ | UInt32 _ -> ()
        ) args;
        pr "}\n";
        pr "\n"
@@ -3054,6 +3058,9 @@ let print_python_binding name { args; ret }         pr
"  long long %s; /* really int64_t */\n" n
     | Opaque n ->
        pr "  PyObject *%s;\n" n
+    | OptUInt32 n ->
+       pr "  uint32_t %s_u32;\n" n;
+       pr "  unsigned int %s = 0; /* really uint32_t */\n" n
     | Path n -> pr "  char *%s = NULL;\n" n
     | SockAddrAndLen (n, _) ->
        pr "  /* XXX Complicated - Python uses a tuple of
different\n";
@@ -3088,6 +3095,7 @@ let print_python_binding name { args; ret }      | Int n
-> pr " \"i\""
     | Int64 n -> pr " \"L\""
     | Opaque _ -> pr " \"O\""
+    | OptUInt32 n -> pr " \"|I\""
     | Path n -> pr " \"O&\""
     | SockAddrAndLen (n, _) -> pr " \"O\""
     | String n -> pr " \"s\""
@@ -3111,6 +3119,7 @@ let print_python_binding name { args; ret }      | Int n
-> pr ", &%s" n
     | Int64 n -> pr ", &%s" n
     | Opaque n -> pr ", &%s" n
+    | OptUInt32 n -> pr ", &%s" n
     | Path n -> pr ", PyUnicode_FSConverter, &%s" n
     | SockAddrAndLen (n, _) -> pr ", &%s" n
     | String n -> pr ", &%s" n
@@ -3160,6 +3169,7 @@ let print_python_binding name { args; ret }      | Int64 n
-> pr "  %s_i64 = %s;\n" n n
     | Opaque n ->
        pr "  callback_data.data = %s;\n" n
+    | OptUInt32 n -> pr "  %s_u32 = %s;\n" n n
     | Path _ -> ()
     | SockAddrAndLen _ ->
        pr "  abort (); /* XXX SockAddrAndLen not implemented */\n";
@@ -3187,6 +3197,7 @@ let print_python_binding name { args; ret }      | Int n
-> pr ", %s" n
     | Int64 n -> pr ", %s_i64" n
     | Opaque _ -> pr ", &callback_data"
+    | OptUInt32 n -> pr ", %s_u32" n
     | Path n -> pr ", %s" n
     | SockAddrAndLen (n, _) -> pr ", /* XXX */ (void *) %s, 0" n
     | String n -> pr ", %s" n
@@ -3220,6 +3231,7 @@ let print_python_binding name { args; ret }      | Int _
     | Int64 _
     | Opaque _
+    | OptUInt32 _
     | Path _
     | SockAddrAndLen _
     | String _
@@ -3261,6 +3273,7 @@ let print_python_binding name { args; ret }      | Int _
-> ()
     | Int64 _ -> ()
     | Opaque _ -> ()
+    | OptUInt32 _ -> ()
     | Path n -> pr "  free (%s);\n" n
     | SockAddrAndLen _ -> ()
     | String n -> ()
@@ -3304,7 +3317,7 @@ Python bindings for libnbd
 import nbd
 h = nbd.NBD ()
 h.connect_tcp (\"localhost\", \"nbd\")
-buf = h.pread (512, 0, 0)
+buf = h.pread (512, 0)

 Read the libnbd(3) man page to find out how to use the API.
 '''
@@ -3344,6 +3357,29 @@ class NBD (object):

   List.iter (
     fun (name, { args; shortdesc; longdesc }) ->
+      let params = List.map (
+                     function
+                     | ArrayAndLen (UInt32 n, _) -> n
+                     | ArrayAndLen _ -> assert false
+                     | Bool n -> n
+                     | BytesIn (n, _) | BytesPersistIn (n, _) -> n
+                     | BytesPersistOut (n, _) -> n
+                     | BytesOut (_, count) -> count
+                     | Callback (n, _) -> n
+                     | Int n -> n
+                     | Int64 n -> n
+                     | Opaque n -> n
+                     | OptUInt32 n -> n
+                     | Path n -> n
+                     | SockAddrAndLen (n, _) -> n
+                     | String n -> n
+                     | StringList n -> n
+                     | UInt n -> n
+                     | UInt32 n -> n
+                     | UInt64 n -> n
+                   ) args in
+      let params = List.map ((^) ", ") params in
+      let params = String.concat "" params in
       let args = List.map (
                      function
                      | ArrayAndLen (UInt32 n, _) -> n
@@ -3356,6 +3392,7 @@ class NBD (object):
                      | Int n -> n
                      | Int64 n -> n
                      | Opaque n -> n
+                     | OptUInt32 n -> n ^ " = 0"
                      | Path n -> n
                      | SockAddrAndLen (n, _) -> n
                      | String n -> n
@@ -3371,7 +3408,7 @@ class NBD (object):
       let longdesc = pod2text longdesc in
       pr "    def %s (self%s):\n" name args;
       pr "        '''▶ %s\n\n%s'''\n"
shortdesc (String.concat "\n" longdesc);
-      pr "        return libnbdmod.%s (self._o%s)\n" name args;
+      pr "        return libnbdmod.%s (self._o%s)\n" name params;
       pr "\n"
   ) handle_calls;

@@ -3405,7 +3442,7 @@ is an open NBD handle called ‘h’.

 h.connect_tcp (\"remote\", \"10809\")   # Connect to a
remote server.
 h.get_size ()                       # Get size of the remote disk.
-buf = h.pread (512, 0, 0)           # Read the first sector.
+buf = h.pread (512, 0)              # Read the first sector.
 exit() or Ctrl-D                    # Quit the shell
 help (nbd)                          # Display documentation
 '''
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/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.20.1

When structured replies are negotiated, the server may advertise
support for the DF flag (the server promises to return at most one
data/hole chunk, or to fail with EOVERFLOW if the chunk would be too
large). As both nbdkit and qemu-nbd support this flag (the former only
trivially, but the latter by not compressing holes over the wire), it
is worth exposing to clients.  While most clients will probably not be
using it, especially as long as we don't have an aio method for
getting at individual chunks, supporting the flag now at least helps
in testing server implementations.
---
 generator/generator | 21 +++++++++++++++++++--
 lib/flags.c         | 11 +++++++++++
 lib/rw.c            |  8 +++++++-
 3 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/generator/generator b/generator/generator
index 919341c..3f300fd 100755
--- a/generator/generator
+++ b/generator/generator
@@ -1165,6 +1165,18 @@ the server does not.  Can return an error if we have not
 connected to and completed the handshake with the server.";
   };

+  "can_df", {
+    default_call with
+    args = []; ret = RBool;
+    shortdesc = "does the server support the don't fragment flag to
pread?";
+    longdesc = "\
+Returns true if the server supports structured reads with an
+ability to request a non-fragmented read (see C<nbd_pread>,
+C<nbd_aio_pread>).  Returns false if the server does not.
+Can return an error if we have not connected to and completed
+the handshake with the server.";
+  };
+
   "can_multi_conn", {
     default_call with
     args = []; ret = RBool;
@@ -1230,8 +1242,12 @@ can only read all or nothing using this call.  The call
 returns when the data has been read fully into C<buf> or there is an
 error.

-The C<flags> parameter must be C<0> for now (it exists for future
NBD
-protocol extensions).";
+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 fragment
+a read reply across more than one packet, even if using multiple
+packets would otherwise allow a more efficient representation of
+holes contained in the region (if that is supported - some servers
+cannot do this, see C<nbd_can_df>).";
   };

   "pwrite", {
@@ -1774,6 +1790,7 @@ let constants = [

   "CMD_FLAG_FUA",        1 lsl 0;
   "CMD_FLAG_NO_HOLE",    1 lsl 1;
+  "CMD_FLAG_DF",         1 lsl 2;
   "CMD_FLAG_REQ_ONE",    1 lsl 3;
 ]

diff --git a/lib/flags.c b/lib/flags.c
index 421a7d2..cdbc28f 100644
--- a/lib/flags.c
+++ b/lib/flags.c
@@ -42,6 +42,11 @@ nbd_internal_set_size_and_flags (struct nbd_handle *h,
     return -1;
   }

+  if (eflags & NBD_FLAG_SEND_DF && !h->structured_replies) {
+    debug (h, "server lacks structured replies, ignoring claim of
df");
+    eflags &= ~NBD_FLAG_SEND_DF;
+  }
+
   h->exportsize = exportsize;
   h->eflags = eflags;
   return 0;
@@ -95,6 +100,12 @@ nbd_unlocked_can_zero (struct nbd_handle *h)
   return get_flag (h, NBD_FLAG_SEND_WRITE_ZEROES);
 }

+int
+nbd_unlocked_can_df (struct nbd_handle *h)
+{
+  return get_flag (h, NBD_FLAG_SEND_DF);
+}
+
 int
 nbd_unlocked_can_multi_conn (struct nbd_handle *h)
 {
diff --git a/lib/rw.c b/lib/rw.c
index feaf468..343c340 100644
--- a/lib/rw.c
+++ b/lib/rw.c
@@ -234,11 +234,17 @@ int64_t
 nbd_unlocked_aio_pread (struct nbd_handle *h, void *buf,
                         size_t count, uint64_t offset, uint32_t flags)
 {
-  if (flags != 0) {
+  if ((flags & ~LIBNBD_CMD_FLAG_DF) != 0) {
     set_error (EINVAL, "invalid flag: %" PRIu32, flags);
     return -1;
   }

+  if ((flags & LIBNBD_CMD_FLAG_DF) != 0 &&
+      nbd_unlocked_can_df (h) != 1) {
+    set_error (EINVAL, "server does not support the DF flag");
+    return -1;
+  }
+
   return nbd_internal_command_common (h, 0, NBD_CMD_READ, offset, count,
                                       buf, NULL);
 }
-- 
2.20.1

Patches 1 & 2 are fine, ACK.  I have a separate comment about patch 4
coming up.

In libguestfs we handle optional args by putting them in a
separate list (so instead of { args; ret } we have we have
{ args; optargs; ret }).

This gets translated into a language like Python in the natural
way.  However for C it's translated into a varargs list with
a rather complex system of flags, see:

  http://libguestfs.org/guestfs.3.html#calls-with-optional-arguments

This has the advantage of supporting optional arguments that can be
nullable (or non-zero default value for ints) vs. not present.

On the other hand this may be overkill for libnbd.  If the only
optional argument we support we care about is a non-zero int (or say a
non-nullable string where NULL = not present) then mixing the optional
args into the same list could be fine.

Do we have a good idea about what optional arguments apart from
non-zero ints we might need in future?

Also, does Python really require optional arguments to be last in the
list?  (OCaml can have them anywhere in the list)

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

On Mon, May 27, 2019 at 09:01:01PM -0500, Eric Blake
wrote:
> diff --git a/lib/rw.c b/lib/rw.c
> index feaf468..343c340 100644
> --- a/lib/rw.c
> +++ b/lib/rw.c
> @@ -234,11 +234,17 @@ int64_t
>  nbd_unlocked_aio_pread (struct nbd_handle *h, void *buf,
>                          size_t count, uint64_t offset, uint32_t flags)
>  {
> -  if (flags != 0) {
> +  if ((flags & ~LIBNBD_CMD_FLAG_DF) != 0) {
>      set_error (EINVAL, "invalid flag: %" PRIu32, flags);
>      return -1;
>    }
> 
> +  if ((flags & LIBNBD_CMD_FLAG_DF) != 0 &&
> +      nbd_unlocked_can_df (h) != 1) {
> +    set_error (EINVAL, "server does not support the DF flag");
> +    return -1;
> +  }
I'm confused why you'd want to specify this flag to pread.  From the
caller point of view they shouldn't care about whether the reply on
the wire is fragmented or not?

(I can understand why you'd need this flag if we implemented a
separate structured_pread call.)

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top