[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

[Libguestfs] [PATCH libnbd] lib: Remove cookie parameter from completion callbacks.



As discussed in this thread, the parameter is an invitation to write
code with race conditions:

https://www.redhat.com/archives/libguestfs/2019-July/thread.html#00309
---
 docs/libnbd.pod                               |  6 +-
 examples/glib-main-loop.c                     | 10 ++--
 examples/strict-structured-reads.c            |  2 +-
 generator/generator                           | 57 +++++++++++++------
 generator/states-reply.c                      |  2 +-
 generator/states.c                            |  2 +-
 lib/aio.c                                     |  2 +-
 lib/internal.h                                |  2 +-
 .../test_505_aio_pread_structured_callback.ml |  3 +-
 python/t/505-aio-pread-callback.py            |  3 +-
 tests/closure-lifetimes.c                     |  3 +-
 tests/server-death.c                          |  2 +-
 12 files changed, 55 insertions(+), 39 deletions(-)

diff --git a/docs/libnbd.pod b/docs/libnbd.pod
index 25d31f9..55dd74d 100644
--- a/docs/libnbd.pod
+++ b/docs/libnbd.pod
@@ -554,10 +554,8 @@ same nbd object, as it would cause deadlock.
 
 All of the low-level commands have a completion callback variant that
 registers a callback function used right before the command is marked
-complete.  The completion callback will be invoked with C<cookie> set
-to the same value returned by the original API such as
-C<nbd_aio_pread_callback> (in rare cases, it is possible that the
-completion callback may fire before the original API has returned).
+complete.
+
 When C<valid_flag> includes C<LIBNBD_CALLBACK_VALID>, and the
 completion callback returns C<1>, the command is automatically retired
 (there is no need to call C<nbd_aio_command_completed>); for any other
diff --git a/examples/glib-main-loop.c b/examples/glib-main-loop.c
index 826651e..05a59e3 100644
--- a/examples/glib-main-loop.c
+++ b/examples/glib-main-loop.c
@@ -268,11 +268,9 @@ static GMainLoop *loop;
 
 static void connected (struct NBDSource *source);
 static gboolean read_data (gpointer user_data);
-static int finished_read (unsigned valid_flag, void *vp,
-                          int64_t rcookie, int *error);
+static int finished_read (unsigned valid_flag, void *vp, int *error);
 static gboolean write_data (gpointer user_data);
-static int finished_write (unsigned valid_flag, void *vp,
-                           int64_t wcookie, int *error);
+static int finished_write (unsigned valid_flag, void *vp, int *error);
 
 int
 main (int argc, char *argv[])
@@ -396,7 +394,7 @@ read_data (gpointer user_data)
 
 /* This callback is called from libnbd when any read command finishes. */
 static int
-finished_read (unsigned valid_flag, void *vp, int64_t unused, int *error)
+finished_read (unsigned valid_flag, void *vp, int *error)
 {
   struct buffer *buffer = vp;
 
@@ -444,7 +442,7 @@ write_data (gpointer user_data)
 
 /* This callback is called from libnbd when any write command finishes. */
 static int
-finished_write (unsigned valid_flag, void *vp, int64_t unused, int *error)
+finished_write (unsigned valid_flag, void *vp, int *error)
 {
   struct buffer *buffer = vp;
 
diff --git a/examples/strict-structured-reads.c b/examples/strict-structured-reads.c
index 2279301..db2eaf1 100644
--- a/examples/strict-structured-reads.c
+++ b/examples/strict-structured-reads.c
@@ -131,7 +131,7 @@ read_chunk (unsigned valid_flag, void *opaque,
 }
 
 static int
-read_verify (unsigned valid_flag, void *opaque, int64_t cookie, int *error)
+read_verify (unsigned valid_flag, void *opaque, int *error)
 {
   int ret = 0;
 
diff --git a/generator/generator b/generator/generator
index 99cc411..82f46a2 100755
--- a/generator/generator
+++ b/generator/generator
@@ -1729,7 +1729,7 @@ C<nbd_pread>.";
     default_call with
     args = [ BytesPersistOut ("buf", "count"); UInt64 "offset";
              Closure { cbname="callback";
-                       cbargs=[Int64 "cookie"; Mutable (Int "error")] };
+                       cbargs=[Mutable (Int "error")] };
              Flags "flags" ];
     ret = RInt64;
     permitted_states = [ Connected ];
@@ -1737,8 +1737,11 @@ C<nbd_pread>.";
     longdesc = "\
 Issue a read command to the NBD server.  This returns the
 unique positive 64 bit cookie for this command, or C<-1> on
-error.  If this command returns a cookie, then C<callback>
+error.
+
+When the command completes, C<callback>
 will be invoked as described in L<libnbd(3)/Completion callbacks>.
+
 Note that you must ensure C<buf> is valid until the command has
 completed.  Other parameters behave as documented in C<nbd_pread>.";
   };
@@ -1773,8 +1776,7 @@ documented in C<nbd_pread_structured>.";
                                UInt "status";
                                Mutable (Int "error"); ]};
              Closure { cbname="callback";
-                       cbargs=[Int64 "cookie";
-                               Mutable (Int "error"); ]};
+                       cbargs=[Mutable (Int "error"); ]};
              Flags "flags" ];
     ret = RInt64;
     permitted_states = [ Connected ];
@@ -1782,8 +1784,11 @@ documented in C<nbd_pread_structured>.";
     longdesc = "\
 Issue a read command to the NBD server.  This returns the
 unique positive 64 bit cookie for this command, or C<-1> on
-error.  If this command returns a cookie, then C<callback>
+error.
+
+When the command completes, C<callback>
 will be invoked as described in L<libnbd(3)/Completion callbacks>.
+
 Other parameters behave as documented in C<nbd_pread_structured>.";
   };
 
@@ -1807,7 +1812,7 @@ C<nbd_pwrite>.";
     default_call with
     args = [ BytesPersistIn ("buf", "count"); UInt64 "offset";
              Closure { cbname="callback";
-                       cbargs=[Int64 "cookie"; Mutable (Int "error")]};
+                       cbargs=[Mutable (Int "error")]};
              Flags "flags" ];
     ret = RInt64;
     permitted_states = [ Connected ];
@@ -1815,8 +1820,11 @@ C<nbd_pwrite>.";
     longdesc = "\
 Issue a write command to the NBD server.  This returns the
 unique positive 64 bit cookie for this command, or C<-1> on
-error.  If this command returns a cookie, then C<callback>
+error.
+
+When the command completes, C<callback>
 will be invoked as described in L<libnbd(3)/Completion callbacks>.
+
 Note that you must ensure C<buf> is valid until the command has
 completed.  Other parameters behave as documented in C<nbd_pwrite>.";
   };
@@ -1860,7 +1868,7 @@ Parameters behave as documented in C<nbd_flush>.";
   "aio_flush_callback", {
     default_call with
     args = [ Closure { cbname="callback";
-                       cbargs=[Int64 "cookie"; Mutable (Int "error")]};
+                       cbargs=[Mutable (Int "error")]};
              Flags "flags" ];
     ret = RInt64;
     permitted_states = [ Connected ];
@@ -1868,8 +1876,11 @@ Parameters behave as documented in C<nbd_flush>.";
     longdesc = "\
 Issue the flush command to the NBD server.  This returns the
 unique positive 64 bit cookie for this command, or C<-1> on
-error.  If this command returns a cookie, then C<callback>
+error.
+
+When the command completes, C<callback>
 will be invoked as described in L<libnbd(3)/Completion callbacks>.
+
 Other parameters behave as documented in C<nbd_flush>.";
   };
 
@@ -1891,7 +1902,7 @@ Parameters behave as documented in C<nbd_trim>.";
     default_call with
     args = [ UInt64 "count"; UInt64 "offset";
              Closure { cbname="callback";
-                       cbargs=[Int64 "cookie"; Mutable (Int "error")]};
+                       cbargs=[Mutable (Int "error")]};
              Flags "flags" ];
     ret = RInt64;
     permitted_states = [ Connected ];
@@ -1899,8 +1910,11 @@ Parameters behave as documented in C<nbd_trim>.";
     longdesc = "\
 Issue a trim command to the NBD server.  This returns the
 unique positive 64 bit cookie for this command, or C<-1> on
-error.  If this command returns a cookie, then C<callback>
+error.
+
+When the command completes, C<callback>
 will be invoked as described in L<libnbd(3)/Completion callbacks>.
+
 Other parameters behave as documented in C<nbd_trim>.";
   };
 
@@ -1922,7 +1936,7 @@ Parameters behave as documented in C<nbd_cache>.";
     default_call with
     args = [ UInt64 "count"; UInt64 "offset";
              Closure { cbname="callback";
-                       cbargs=[Int64 "cookie"; Mutable (Int "error")]};
+                       cbargs=[Mutable (Int "error")]};
              Flags "flags" ];
     ret = RInt64;
     permitted_states = [ Connected ];
@@ -1930,8 +1944,11 @@ Parameters behave as documented in C<nbd_cache>.";
     longdesc = "\
 Issue the cache (prefetch) command to the NBD server.  This
 returns the unique positive 64 bit cookie for this command, or
-C<-1> on error.  If this command returns a cookie, then C<callback>
+C<-1> on error.
+
+When the command completes, C<callback>
 will be invoked as described in L<libnbd(3)/Completion callbacks>.
+
 Other parameters behave as documented in C<nbd_cache>.";
   };
 
@@ -1953,7 +1970,7 @@ Parameters behave as documented in C<nbd_zero>.";
     default_call with
     args = [ UInt64 "count"; UInt64 "offset";
              Closure { cbname="callback";
-                       cbargs=[Int64 "cookie"; Mutable (Int "error")]};
+                       cbargs=[Mutable (Int "error")]};
              Flags "flags" ];
     ret = RInt64;
     permitted_states = [ Connected ];
@@ -1961,8 +1978,11 @@ Parameters behave as documented in C<nbd_zero>.";
     longdesc = "\
 Issue a write zeroes command to the NBD server.  This returns the
 unique positive 64 bit cookie for this command, or C<-1> on
-error.  If this command returns a cookie, then C<callback>
+error.
+
+When the command completes, C<callback>
 will be invoked as described in L<libnbd(3)/Completion callbacks>.
+
 Other parameters behave as documented in C<nbd_zero>.";
   };
 
@@ -1995,7 +2015,7 @@ Parameters behave as documented in C<nbd_block_status>.";
                                             "nr_entries");
                                Mutable (Int "error")]};
              Closure { cbname="callback";
-                       cbargs=[Int64 "cookie"; Mutable (Int "error")]};
+                       cbargs=[Mutable (Int "error")]};
              Flags "flags" ];
     ret = RInt64;
     permitted_states = [ Connected ];
@@ -2003,8 +2023,11 @@ Parameters behave as documented in C<nbd_block_status>.";
     longdesc = "\
 Send the block status command to the NBD server.  This returns the
 unique positive 64 bit cookie for this command, or C<-1> on
-error.  If this command returns a cookie, then C<callback>
+error.
+
+When the command completes, C<callback>
 will be invoked as described in L<libnbd(3)/Completion callbacks>.
+
 Other parameters behave as documented in C<nbd_block_status>.";
   };
 
diff --git a/generator/states-reply.c b/generator/states-reply.c
index 8f62923..078d67f 100644
--- a/generator/states-reply.c
+++ b/generator/states-reply.c
@@ -174,7 +174,7 @@ save_reply_state (struct nbd_handle *h)
 
     assert (cmd->type != NBD_CMD_DISC);
     r = cmd->cb.callback (LIBNBD_CALLBACK_VALID|LIBNBD_CALLBACK_FREE,
-                          cmd->cb.user_data, cookie, &error);
+                          cmd->cb.user_data, &error);
     cmd->cb.callback = NULL; /* because we've freed it */
     switch (r) {
     case -1:
diff --git a/generator/states.c b/generator/states.c
index a7f7ca0..654e4c8 100644
--- a/generator/states.c
+++ b/generator/states.c
@@ -127,7 +127,7 @@ void abort_commands (struct nbd_handle *h,
 
       assert (cmd->type != NBD_CMD_DISC);
       r = cmd->cb.callback (LIBNBD_CALLBACK_VALID|LIBNBD_CALLBACK_FREE,
-                            cmd->cb.user_data, cmd->cookie, &error);
+                            cmd->cb.user_data, &error);
       cmd->cb.callback = NULL; /* because we've freed it */
       switch (r) {
       case -1:
diff --git a/lib/aio.c b/lib/aio.c
index 94c394a..1c11dbd 100644
--- a/lib/aio.c
+++ b/lib/aio.c
@@ -40,7 +40,7 @@ nbd_internal_retire_and_free_command (struct command *cmd)
                      NULL, 0, 0, 0, NULL);
   if (cmd->cb.callback)
     cmd->cb.callback (LIBNBD_CALLBACK_FREE, cmd->cb.user_data,
-                      0, NULL);
+                      NULL);
 
   free (cmd);
 }
diff --git a/lib/internal.h b/lib/internal.h
index 14d2ef0..90ce6aa 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -255,7 +255,7 @@ typedef int (*read_fn) (unsigned valid_flag, void *user_data,
                         const void *buf, size_t count,
                         uint64_t offset, unsigned status, int *error);
 typedef int (*callback_fn) (unsigned valid_flag, void *user_data,
-                            int64_t cookie, int *error);
+                            int *error);
 
 struct command_cb {
   union {
diff --git a/ocaml/tests/test_505_aio_pread_structured_callback.ml b/ocaml/tests/test_505_aio_pread_structured_callback.ml
index 59edec9..27b7ebf 100644
--- a/ocaml/tests/test_505_aio_pread_structured_callback.ml
+++ b/ocaml/tests/test_505_aio_pread_structured_callback.ml
@@ -43,12 +43,11 @@ let chunk user_data buf2 offset s err =
   assert (offset = 0_L);
   assert (s = Int32.to_int NBD.read_data)
 
-let callback user_data cookie err =
+let callback user_data err =
   if user_data <= 43 then
     assert (!err = 0)
   else
     assert (!err = 100);
-  assert (cookie > Int64.of_int (0));
   err := 101;
   assert (user_data = 42)
 
diff --git a/python/t/505-aio-pread-callback.py b/python/t/505-aio-pread-callback.py
index 4baf92a..9246616 100644
--- a/python/t/505-aio-pread-callback.py
+++ b/python/t/505-aio-pread-callback.py
@@ -33,13 +33,12 @@ def chunk (user_data, buf2, offset, s, err):
     assert offset == 0
     assert s == nbd.READ_DATA
 
-def callback (user_data, cookie, err):
+def callback (user_data, err):
     print ("in callback, user_data %d" % user_data)
     if user_data <= 43:
         assert err.value == 0
     else:
         assert err.value == errno.EPROTO
-    assert cookie > 0
     err.value = errno.ENOMEM
     assert user_data == 42
 
diff --git a/tests/closure-lifetimes.c b/tests/closure-lifetimes.c
index 3ddf4c1..b225d68 100644
--- a/tests/closure-lifetimes.c
+++ b/tests/closure-lifetimes.c
@@ -73,8 +73,7 @@ read_cb (unsigned valid_flag, void *opaque,
 }
 
 static int
-completion_cb (unsigned valid_flag, void *opaque,
-               int64_t cookie, int *error)
+completion_cb (unsigned valid_flag, void *opaque, int *error)
 {
   assert (completion_cb_free == 0);
 
diff --git a/tests/server-death.c b/tests/server-death.c
index 56e7e51..7854527 100644
--- a/tests/server-death.c
+++ b/tests/server-death.c
@@ -33,7 +33,7 @@ static bool trim_retired;
 static const char *progname;
 
 static int
-callback (unsigned valid_flag, void *ignored, int64_t cookie, int *error)
+callback (unsigned valid_flag, void *ignored, int *error)
 {
   if (!(valid_flag & LIBNBD_CALLBACK_VALID))
     return 0;
-- 
2.22.0


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]