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