Libguestfs - Jul 2019 - [libnbd PATCH] generator: Prefer closure opaque after function pointer in C

Existing practice tends to prefer the void* closure argument for C
callbacks to occur after the function pointer (POSIX: pthread_create;
glibc: qsort_r; glib: g_thread_new; libvirt:
virConnectDomainEventRegisterAny; etc.). It's also handy to think that
calling 'myfunc(cb, arg)' will eventually result in a call to
'cb(arg)'. While I couldn't quickly find any style guide
recommending
one way over another, and while it probably doesn't hurt which style
we choose internally as long as we stick to it, it is still probably
worth the notion of listing the closure argument last.

Of course, that means that this also reverts commit
abcdf71a7585f4f6cfe5943a0082ce0184e9b1fb.  It also means that this is
yet another API/ABI break - good thing we haven't declared stable API
yet.
---
 examples/strict-structured-reads.c |  4 +-
 generator/generator                | 27 ++++++-------
 interop/dirty-bitmap.c             |  6 +--
 interop/structured-read.c          |  6 +--
 lib/debug.c                        |  4 +-
 lib/handle.c                       |  2 +-
 lib/internal.h                     |  4 +-
 lib/rw.c                           | 61 +++++++++++++++---------------
 tests/meta-base-allocation.c       |  6 +--
 tests/oldstyle.c                   |  4 +-
 10 files changed, 62 insertions(+), 62 deletions(-)

diff --git a/examples/strict-structured-reads.c
b/examples/strict-structured-reads.c
index 1526344..92eb3e6 100644
--- a/examples/strict-structured-reads.c
+++ b/examples/strict-structured-reads.c
@@ -224,8 +224,8 @@ main (int argc, char *argv[])
     *r = (struct range) { .first = offset, .last = offset + maxsize, };
     *d = (struct data) { .offset = offset, .count = maxsize, .flags = flags,
                          .remaining = r, };
-    if (nbd_aio_pread_structured_callback (nbd, buf, sizeof buf, offset, d,
-                                           read_chunk, read_verify,
+    if (nbd_aio_pread_structured_callback (nbd, buf, sizeof buf, offset,
+                                           read_chunk, read_verify, d,
                                            flags) == -1) {
       fprintf (stderr, "%s\n", nbd_get_error ());
       exit (EXIT_FAILURE);
diff --git a/generator/generator b/generator/generator
index fab7281..485a76a 100755
--- a/generator/generator
+++ b/generator/generator
@@ -3173,7 +3173,7 @@ let rec c_name_of_arg = function
 | BytesPersistIn (n, len) -> [n; len]
 | BytesPersistOut (n, len) -> [n; len]
 | Closure (_, closures) ->
-   "user_data" :: List.map (fun { cbname } -> cbname) closures
+   List.map (fun { cbname } -> cbname) closures @ ["user_data"]
 | Flags n -> [n]
 | Int n -> [n]
 | Int64 n -> [n]
@@ -3211,12 +3211,13 @@ let rec print_c_arg_list ?(handle = false) ?(user_data =
false) args        | BytesOut (n, len)
       | BytesPersistOut (n, len) -> pr "void *%s, size_t %s" n len
       | Closure (_, cls) ->
-         pr "void *user_data";
          List.iter (
            fun { cbname; cbargs } ->
-             pr ", int (*%s) " cbname;
-             print_c_arg_list ~user_data:true cbargs
-         ) cls
+             pr "int (*%s) " cbname;
+             print_c_arg_list ~user_data:true cbargs;
+             pr ", "
+         ) cls;
+         pr "void *user_data"
       | Flags n -> pr "uint32_t %s" n
       | Int n -> pr "int %s" n
       | Int64 n -> pr "int64_t %s" n
@@ -3305,8 +3306,8 @@ let generate_include_libnbd_h ()    pr "#define
LIBNBD_HAVE_NBD_GET_ERRNO 1\n";
   pr "\n";
   pr "extern int nbd_add_close_callback (struct nbd_handle *h,\n";
-  pr "                                   void *user_data,\n";
-  pr "                                   nbd_close_callback cb);\n";
+  pr "                                   nbd_close_callback cb,\n";
+  pr "                                   void *user_data);\n";
   pr "#define LIBNBD_HAVE_NBD_ADD_CLOSE_CALLBACK 1\n";
   pr "\n";
   List.iter (
@@ -3593,7 +3594,7 @@ from other programming languages.

  typedef void (*nbd_close_callback) (void *user_data);
  int nbd_add_close_callback (struct nbd_handle *nbd,
-                             void *user_data, nbd_close_callback cb);
+                             nbd_close_callback cb, void *user_data);

 ";

@@ -4112,11 +4113,11 @@ let print_python_binding name { args; ret }      |
BytesPersistIn (n, _)
     | BytesPersistOut (n, _) -> pr ", %s_buf->data,
%s_buf->len" n n
     | Closure (_, cls) ->
-       pr ", user_data";
        List.iter (
          fun { cbname } ->
            pr ", %s_%s_wrapper" name cbname
-       ) cls
+       ) cls;
+       pr ", user_data"
     | Flags n -> pr ", %s_u32" n
     | Int n -> pr ", %s" n
     | Int64 n -> pr ", %s_i64" n
@@ -4195,7 +4196,7 @@ let print_python_binding name { args; ret }      | Closure
(false, _) -> ()
     | Closure (true, _) ->
        pr "  /* This ensures the callback data is freed eventually.
*/\n";
-       pr "  nbd_add_close_callback (h, user_data,
free_%s_user_data);\n" name
+       pr "  nbd_add_close_callback (h, free_%s_user_data,
user_data);\n" name
     | Flags _ -> ()
     | Int _ -> ()
     | Int64 _ -> ()
@@ -4342,7 +4343,7 @@ class NBD (object):
             | BytesIn (n, _) | BytesPersistIn (n, _) -> [n, None]
             | BytesPersistOut (n, _) -> [n, None]
             | BytesOut (_, count) -> [count, None]
-            | Closure (_, cls) -> 
+            | Closure (_, cls) ->
                List.map (fun { cbname } -> cbname, None) cls
             | Flags n -> [n, Some "0"]
             | Int n -> [n, None]
@@ -4888,7 +4889,7 @@ let print_ocaml_binding (name, { args; ret })    (match
has_closures with
    | None | Some false -> ()
    | Some true ->
-      pr "  nbd_add_close_callback (h, user_data,
free_%s_user_data);\n" name
+      pr "  nbd_add_close_callback (h, free_%s_user_data,
user_data);\n" name
   );

   let errcode diff --git a/interop/dirty-bitmap.c b/interop/dirty-bitmap.c
index 6b7493e..8f0087d 100644
--- a/interop/dirty-bitmap.c
+++ b/interop/dirty-bitmap.c
@@ -137,14 +137,14 @@ main (int argc, char *argv[])
   }

   data = (struct data) { .count = 2, };
-  if (nbd_block_status (nbd, exportsize, 0, &data, cb, 0) == -1) {
+  if (nbd_block_status (nbd, exportsize, 0, cb, &data, 0) == -1) {
     fprintf (stderr, "%s\n", nbd_get_error ());
     exit (EXIT_FAILURE);
   }
   assert (data.seen_base && data.seen_dirty);

   data = (struct data) { .req_one = true, .count = 2, };
-  if (nbd_block_status (nbd, exportsize, 0, &data, cb,
+  if (nbd_block_status (nbd, exportsize, 0, cb, &data,
                         LIBNBD_CMD_FLAG_REQ_ONE) == -1) {
     fprintf (stderr, "%s\n", nbd_get_error ());
     exit (EXIT_FAILURE);
@@ -153,7 +153,7 @@ main (int argc, char *argv[])

   /* Trigger a failed callback, to prove connection stays up. */
   data = (struct data) { .count = 2, .fail = true, };
-  if (nbd_block_status (nbd, exportsize, 0, &data, cb, 0) != -1) {
+  if (nbd_block_status (nbd, exportsize, 0, cb, &data, 0) != -1) {
     fprintf (stderr, "unexpected block status success\n");
     exit (EXIT_FAILURE);
   }
diff --git a/interop/structured-read.c b/interop/structured-read.c
index f9014c8..d00524f 100644
--- a/interop/structured-read.c
+++ b/interop/structured-read.c
@@ -143,7 +143,7 @@ main (int argc, char *argv[])

   memset (rbuf, 2, sizeof rbuf);
   data = (struct data) { .count = 2, };
-  if (nbd_pread_structured (nbd, rbuf, sizeof rbuf, 2048, &data, read_cb,
+  if (nbd_pread_structured (nbd, rbuf, sizeof rbuf, 2048, read_cb, &data,
                             0) == -1) {
     fprintf (stderr, "%s\n", nbd_get_error ());
     exit (EXIT_FAILURE);
@@ -153,7 +153,7 @@ main (int argc, char *argv[])
   /* Repeat with DF flag. */
   memset (rbuf, 2, sizeof rbuf);
   data = (struct data) { .df = true, .count = 1, };
-  if (nbd_pread_structured (nbd, rbuf, sizeof rbuf, 2048, &data, read_cb,
+  if (nbd_pread_structured (nbd, rbuf, sizeof rbuf, 2048, read_cb, &data,
                             LIBNBD_CMD_FLAG_DF) == -1) {
     fprintf (stderr, "%s\n", nbd_get_error ());
     exit (EXIT_FAILURE);
@@ -166,7 +166,7 @@ main (int argc, char *argv[])
    */
   memset (rbuf, 2, sizeof rbuf);
   data = (struct data) { .count = 2, .fail = true, };
-  if (nbd_pread_structured (nbd, rbuf, sizeof rbuf, 2048, &data, read_cb,
+  if (nbd_pread_structured (nbd, rbuf, sizeof rbuf, 2048, read_cb, &data,
                             0) != -1) {
     fprintf (stderr, "unexpected pread callback success\n");
     exit (EXIT_FAILURE);
diff --git a/lib/debug.c b/lib/debug.c
index 02d49f7..12c10f7 100644
--- a/lib/debug.c
+++ b/lib/debug.c
@@ -40,8 +40,8 @@ nbd_unlocked_get_debug (struct nbd_handle *h)

 int
 nbd_unlocked_set_debug_callback (struct nbd_handle *h,
-                                 void *data,
-                                 int (*debug_fn) (void *, const char *, const
char *))
+                                 int (*debug_fn) (void *, const char *, const
char *),
+                                 void *data)
 {
   h->debug_fn = debug_fn;
   h->debug_data = data;
diff --git a/lib/handle.c b/lib/handle.c
index 00d0ac2..5003227 100644
--- a/lib/handle.c
+++ b/lib/handle.c
@@ -203,7 +203,7 @@ nbd_unlocked_add_meta_context (struct nbd_handle *h, const
char *name)
  */
 int
 nbd_add_close_callback (struct nbd_handle *h,
-                        void *user_data, nbd_close_callback cb)
+                        nbd_close_callback cb, void *user_data)
 {
   int ret;
   struct close_callback *cc;
diff --git a/lib/internal.h b/lib/internal.h
index 347bf69..5288ca0 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -80,8 +80,8 @@ struct nbd_handle {

   /* For debugging. */
   bool debug;
-  void *debug_data;
   int (*debug_fn) (void *, const char *, const char *);
+  void *debug_data;

   /* Linked list of close callbacks. */
   struct close_callback *close_callbacks;
@@ -249,12 +249,12 @@ typedef int (*read_fn) (void *user_data, const void *buf,
size_t count,
 typedef int (*callback_fn) (void *user_data, int64_t cookie, int *error);

 struct command_cb {
-  void *user_data;
   union {
     extent_fn extent;
     read_fn read;
   } fn;
   callback_fn callback;
+  void *user_data;
 };

 struct command_in_flight {
diff --git a/lib/rw.c b/lib/rw.c
index f2fe4e0..d32f0dc 100644
--- a/lib/rw.c
+++ b/lib/rw.c
@@ -60,12 +60,12 @@ nbd_unlocked_pread (struct nbd_handle *h, void *buf,
 int
 nbd_unlocked_pread_structured (struct nbd_handle *h, void *buf,
                                size_t count, uint64_t offset,
-                               void *user_data, read_fn read, uint32_t flags)
+                               read_fn read, void *user_data, uint32_t flags)
 {
   int64_t ch;

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

@@ -145,11 +145,11 @@ nbd_unlocked_zero (struct nbd_handle *h,
 int
 nbd_unlocked_block_status (struct nbd_handle *h,
                            uint64_t count, uint64_t offset,
-                           void *user_data, extent_fn extent, uint32_t flags)
+                           extent_fn extent, void *user_data, uint32_t flags)
 {
   int64_t ch;

-  ch = nbd_unlocked_aio_block_status (h, count, offset, user_data, extent,
+  ch = nbd_unlocked_aio_block_status (h, count, offset, extent, user_data,
                                       flags);
   if (ch == -1)
     return -1;
@@ -257,10 +257,10 @@ nbd_unlocked_aio_pread (struct nbd_handle *h, void *buf,
 int64_t
 nbd_unlocked_aio_pread_callback (struct nbd_handle *h, void *buf,
                                  size_t count, uint64_t offset,
-                                 void *user_data, callback_fn callback,
+                                 callback_fn callback, void *user_data,
                                  uint32_t flags)
 {
-  struct command_cb cb = { .user_data = user_data, .callback = callback, };
+  struct command_cb cb = { .callback = callback, .user_data = user_data, };

   /* We could silently accept flag DF, but it really only makes sense
    * with callbacks, because otherwise there is no observable change
@@ -278,23 +278,22 @@ nbd_unlocked_aio_pread_callback (struct nbd_handle *h,
void *buf,
 int64_t
 nbd_unlocked_aio_pread_structured (struct nbd_handle *h, void *buf,
                                    size_t count, uint64_t offset,
-                                   void *user_data, read_fn read,
+                                   read_fn read, void *user_data,
                                    uint32_t flags)
 {
   return nbd_unlocked_aio_pread_structured_callback (h, buf, count, offset,
-                                                     user_data, read, NULL,
+                                                     read, NULL, user_data,
                                                      flags);
 }

 int64_t
 nbd_unlocked_aio_pread_structured_callback (struct nbd_handle *h, void *buf,
                                             size_t count, uint64_t offset,
-                                            void *user_data, read_fn read,
-                                            callback_fn callback,
-                                            uint32_t flags)
+                                            read_fn read, callback_fn callback,
+                                            void *user_data, uint32_t flags)
 {
-  struct command_cb cb = { .user_data = user_data, .fn.read = read,
-                           .callback = callback, };
+  struct command_cb cb = { .fn.read = read, .callback = callback,
+                           .user_data = user_data, };

   if ((flags & ~LIBNBD_CMD_FLAG_DF) != 0) {
     set_error (EINVAL, "invalid flag: %" PRIu32, flags);
@@ -323,10 +322,10 @@ nbd_unlocked_aio_pwrite (struct nbd_handle *h, const void
*buf,
 int64_t
 nbd_unlocked_aio_pwrite_callback (struct nbd_handle *h, const void *buf,
                                   size_t count, uint64_t offset,
-                                  void *user_data, callback_fn callback,
+                                  callback_fn callback, void *user_data,
                                   uint32_t flags)
 {
-  struct command_cb cb = { .user_data = user_data, .callback = callback, };
+  struct command_cb cb = { .callback = callback, .user_data = user_data, };

   if (nbd_unlocked_read_only (h) == 1) {
     set_error (EINVAL, "server does not support write operations");
@@ -355,10 +354,10 @@ nbd_unlocked_aio_flush (struct nbd_handle *h, uint32_t
flags)
 }

 int64_t
-nbd_unlocked_aio_flush_callback (struct nbd_handle *h, void *user_data,
-                                 callback_fn callback, uint32_t flags)
+nbd_unlocked_aio_flush_callback (struct nbd_handle *h, callback_fn callback,
+                                 void *user_data, uint32_t flags)
 {
-  struct command_cb cb = { .user_data = user_data, .callback = callback, };
+  struct command_cb cb = { .callback = callback, .user_data = user_data, };

   if (nbd_unlocked_can_flush (h) != 1) {
     set_error (EINVAL, "server does not support flush operations");
@@ -385,10 +384,10 @@ nbd_unlocked_aio_trim (struct nbd_handle *h,
 int64_t
 nbd_unlocked_aio_trim_callback (struct nbd_handle *h,
                                 uint64_t count, uint64_t offset,
-                                void *user_data, callback_fn callback,
+                                callback_fn callback, void *user_data,
                                 uint32_t flags)
 {
-  struct command_cb cb = { .user_data = user_data, .callback = callback, };
+  struct command_cb cb = { .callback = callback, .user_data = user_data, };

   if (nbd_unlocked_read_only (h) == 1) {
     set_error (EINVAL, "server does not support write operations");
@@ -425,10 +424,10 @@ nbd_unlocked_aio_cache (struct nbd_handle *h,
 int64_t
 nbd_unlocked_aio_cache_callback (struct nbd_handle *h,
                                  uint64_t count, uint64_t offset,
-                                 void *user_data, callback_fn callback,
+                                 callback_fn callback, void *user_data,
                                  uint32_t flags)
 {
-  struct command_cb cb = { .user_data = user_data, .callback = callback, };
+  struct command_cb cb = { .callback = callback, .user_data = user_data, };

   /* Actually according to the NBD protocol document, servers do exist
    * that support NBD_CMD_CACHE but don't advertise the
@@ -459,10 +458,10 @@ nbd_unlocked_aio_zero (struct nbd_handle *h,
 int64_t
 nbd_unlocked_aio_zero_callback (struct nbd_handle *h,
                                 uint64_t count, uint64_t offset,
-                                void *user_data, callback_fn callback,
+                                callback_fn callback, void *user_data,
                                 uint32_t flags)
 {
-  struct command_cb cb = { .user_data = user_data, .callback = callback, };
+  struct command_cb cb = { .callback = callback, .user_data = user_data, };

   if (nbd_unlocked_read_only (h) == 1) {
     set_error (EINVAL, "server does not support write operations");
@@ -492,22 +491,22 @@ nbd_unlocked_aio_zero_callback (struct nbd_handle *h,
 int64_t
 nbd_unlocked_aio_block_status (struct nbd_handle *h,
                                uint64_t count, uint64_t offset,
-                               void *user_data, extent_fn extent,
+                               extent_fn extent, void *user_data,
                                uint32_t flags)
 {
   return nbd_unlocked_aio_block_status_callback (h, count, offset,
-                                                 user_data, extent,
-                                                 NULL, flags);
+                                                 extent, NULL, user_data,
+                                                 flags);
 }

 int64_t
 nbd_unlocked_aio_block_status_callback (struct nbd_handle *h,
                                         uint64_t count, uint64_t offset,
-                                        void *user_data, extent_fn extent,
-                                        callback_fn callback, uint32_t flags)
+                                        extent_fn extent, callback_fn callback,
+                                        void *user_data, uint32_t flags)
 {
-  struct command_cb cb = { .user_data = user_data, .fn.extent = extent,
-                           .callback = callback, };
+  struct command_cb cb = { .fn.extent = extent, .callback = callback,
+                           .user_data = user_data, };

   if (!h->structured_replies) {
     set_error (ENOTSUP, "server does not support structured
replies");
diff --git a/tests/meta-base-allocation.c b/tests/meta-base-allocation.c
index 9b3ad1f..95e029b 100644
--- a/tests/meta-base-allocation.c
+++ b/tests/meta-base-allocation.c
@@ -100,19 +100,19 @@ main (int argc, char *argv[])

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

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

   id = 3;
-  if (nbd_block_status (nbd, 1024, 32768-512, &id, check_extent,
+  if (nbd_block_status (nbd, 1024, 32768-512, check_extent, &id,
                         LIBNBD_CMD_FLAG_REQ_ONE) == -1) {
     fprintf (stderr, "%s\n", nbd_get_error ());
     exit (EXIT_FAILURE);
diff --git a/tests/oldstyle.c b/tests/oldstyle.c
index ab8b211..f4397d3 100644
--- a/tests/oldstyle.c
+++ b/tests/oldstyle.c
@@ -125,7 +125,7 @@ main (int argc, char *argv[])
   /* Test again for callback operation. */
   memset (rbuf, 0, sizeof rbuf);
   if (nbd_pread_structured (nbd, rbuf, sizeof rbuf, 2 * sizeof rbuf,
-                            &calls, pread_cb, 0) == -1) {
+                            pread_cb, &calls, 0) == -1) {
     fprintf (stderr, "%s\n", nbd_get_error ());
     exit (EXIT_FAILURE);
   }
@@ -141,7 +141,7 @@ main (int argc, char *argv[])

   /* Also test that callback errors are reflected correctly. */
   if (nbd_pread_structured (nbd, rbuf, sizeof rbuf, 2 * sizeof rbuf,
-                            &calls, pread_cb, 0) != -1) {
+                            pread_cb, &calls, 0) != -1) {
     fprintf (stderr, "%s: expected failure from callback\n",
argv[0]);
     exit (EXIT_FAILURE);
   }
-- 
2.20.1

I checked over this and couldn't see anything wrong, and also it
compiles and passes the tests.  So I have pushed it because I want to
do a 0.1.8 release very soon.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html