Richard W.M. Jones
2019-Jul-30 15:36 UTC
[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
Eric Blake
2019-Jul-30 16:21 UTC
Re: [Libguestfs] [PATCH libnbd] lib: Remove cookie parameter from completion callbacks.
On 7/30/19 10:36 AM, Richard W.M. Jones wrote:> 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 > ---> +++ 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")] };No trailing ; in cbargs=[]...> @@ -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"); ]};...but there is here. Pre-existing, and OCaml tolerates our inconsistency, but it's probably nicer to uniformly skip the trailing ;.> 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>. > +Do we need wording anywhere (probably in the .pod, so we only state it once) that mentions that the callback routine is not invoked if nbd_aio_FOO_callback returns -1? ACK. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Eric Blake
2019-Jul-30 16:27 UTC
Re: [Libguestfs] [PATCH libnbd] lib: Remove cookie parameter from completion callbacks.
On 7/30/19 11:21 AM, Eric Blake wrote:>> @@ -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>. >> + > > Do we need wording anywhere (probably in the .pod, so we only state it > once) that mentions that the callback routine is not invoked if > nbd_aio_FOO_callback returns -1?Oh - that made me think. Now that we promise to invoke callback(FREE) after we are done using it, maybe we SHOULD guarantee that we use callback(FREE) even when returning -1? We could use stronger definitions of what lifetimes to expect on the error paths. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Richard W.M. Jones
2019-Jul-30 16:28 UTC
Re: [Libguestfs] [PATCH libnbd] lib: Remove cookie parameter from completion callbacks.
On Tue, Jul 30, 2019 at 11:21:25AM -0500, Eric Blake wrote:> On 7/30/19 10:36 AM, Richard W.M. Jones wrote: > > +When the command completes, C<callback> > > will be invoked as described in L<libnbd(3)/Completion callbacks>. > > + > > Do we need wording anywhere (probably in the .pod, so we only state it > once) that mentions that the callback routine is not invoked if > nbd_aio_FOO_callback returns -1?I guess it's implicit from the fact that returning -1 indicates an error. In fact the relationship ought to be stronger than that - the command should run if and only if nbd_aio_FOO_callback returns >= 0. (That's not actually true at the moment because an error can happen after we've enqueued the command.) Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/
Possibly Parallel Threads
- [PATCH libnbd 2/3] lib: Implement closure lifetimes.
- [PATCH libnbd v2 2/5] lib: Implement closure lifetimes.
- [PATCH libnbd v3 1/2] lib: Implement closure lifetimes.
- [PATCH libnbd 1/2] generator: Handle closure args (cbargs) specially.
- [PATCH libnbd] generator: Generate typedefs automatically for Closure arguments.