[PATCH libnbd] lib: Remove cookie parameter from completion callbacks.
by Richard W.M. Jones
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
4 years, 9 months
[PATCH libnbd] lib: Use symbol versions.
by Richard W.M. Jones
This patch adds support for symbol versions. It is based on what
libvirt does.
The generated syms file looks like:
LIBNBD_1.0 {
global:
nbd_...;
nbd_...;
local: *;
};
In a future stable 1.2 release, new symbols would go into a new
section which would look like this:
LIBNBD_1.2 {
global:
nbd_new_symbol;
nbd_another_new_symbol;
local: *;
} LIBNBD_1.0;
In my testing the ‘local:’ label is needed. For some reason libvirt
doesn’t use it.
The change appears to be working in as much as I can see the symbol
versions appearing, and the test/example programs continue to run.
$ nm -D --with-symbol-versions lib/.libs/libnbd.so.0 | grep LIBNBD
0000000000000000 A LIBNBD_1.0@(a)LIBNBD_1.0
0000000000005470 T nbd_add_meta_context@(a)LIBNBD_1.0
00000000000085d0 T nbd_aio_block_status@(a)LIBNBD_1.0
0000000000008710 T nbd_aio_block_status_callback@(a)LIBNBD_1.0
0000000000008110 T nbd_aio_cache@(a)LIBNBD_1.0
0000000000008230 T nbd_aio_cache_callback@(a)LIBNBD_1.0
00000000000089e0 T nbd_aio_command_completed@(a)LIBNBD_1.0
0000000000006f20 T nbd_aio_connect@(a)LIBNBD_1.0
0000000000007320 T nbd_aio_connect_command@(a)LIBNBD_1.0
[etc]
$ nm -D --with-symbol-versions examples/.libs/glib-main-loop | grep LIBNBD
U nbd_aio_connect_command(a)LIBNBD_1.0
U nbd_aio_get_direction(a)LIBNBD_1.0
U nbd_aio_get_fd(a)LIBNBD_1.0
U nbd_aio_in_flight(a)LIBNBD_1.0
U nbd_aio_is_ready(a)LIBNBD_1.0
U nbd_aio_notify_read(a)LIBNBD_1.0
U nbd_aio_notify_write(a)LIBNBD_1.0
U nbd_aio_peek_command_completed(a)LIBNBD_1.0
U nbd_aio_pread_callback(a)LIBNBD_1.0
U nbd_aio_pwrite_callback(a)LIBNBD_1.0
U nbd_close(a)LIBNBD_1.0
U nbd_create(a)LIBNBD_1.0
U nbd_get_debug(a)LIBNBD_1.0
U nbd_get_error(a)LIBNBD_1.0
Rich.
4 years, 9 months
[PATCH] Add Rust bindings
by Hiroyuki Katsura
I fixed the patch I submitted before based on comments, and there are some
commits which are merged or divided. So, I will re-send all the patches.
Regards,
Hiroyuki Katsura
4 years, 9 months
[nbdkit PATCH] delay: Avoid numeric overflow
by Eric Blake
Attempting delay-read=1000 results in no delay whatsoever: 1000
seconds, scaled to milliseconds, stored in int, then subjected to
.tv_nsec = (ms * 1000000) % 1000000000
results in .tv_nsec being set to 567587328 thanks to 32-bit overflow,
but that in turn results in instant EINVAL failure of nanosleep().
Fix it by diagnosing failure to fit in an int during config, and avoid
math that overflows during delay(). Forbid negative delays while at it.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
I was trying to test what happens when a non-fatal signal is sent to
nbdkit (by adding a no-op SIGUSR1 handler). Most of the time, sending
SIGUSR1 to the process as a whole ended up cutting the poll() in a
different thread short (and not the nanosleep()), but by directing
SIGUSR1 to the thread stuck in nanosleep, I confirmed that nanosleep()
indeed returns early with EINTR, and we end up with insufficient
delay. But getting to that point in my testing required that I first
diagnosed why my attempt at a 1000-second sleep acted like no delay at
all.
I plan on adding an nbdkit_sleep() wrapper function, which will resume
the sleep on unrelated EINTR (fixing the theoretical problem of not
sleeping long enough - theoretical since it only matters if we handle
a signal but want to continue execution, but all our current handled
signals instead request process termination) as well as the problem of
keeping nbdkit alive too long (by specifically returning an error to
let the delay filter know that it is time to exit, rather than
finishing the sleep, whether it was a signal that interrupted a
different thread or merely detection of EOF from the client).
filters/delay/delay.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/filters/delay/delay.c b/filters/delay/delay.c
index 11681c88..80eb33b0 100644
--- a/filters/delay/delay.c
+++ b/filters/delay/delay.c
@@ -32,6 +32,7 @@
#include <config.h>
+#include <limits.h>
#include <stdio.h>
#include <stdlib.h>
#include <stdint.h>
@@ -54,7 +55,7 @@ parse_delay (const char *key, const char *value)
int r;
if (len > 2 && strcmp (&value[len-2], "ms") == 0) {
- if (sscanf (value, "%d", &r) == 1)
+ if (sscanf (value, "%d", &r) == 1 && r >= 0)
return r;
else {
nbdkit_error ("cannot parse %s in milliseconds parameter: %s",
@@ -63,8 +64,14 @@ parse_delay (const char *key, const char *value)
}
}
else {
- if (sscanf (value, "%d", &r) == 1)
+ if (sscanf (value, "%d", &r) == 1 && r >= 0) {
+ if (r * 1000LL > INT_MAX) {
+ nbdkit_error ("seconds parameter %s is too large: %s",
+ key, value);
+ return -1;
+ }
return r * 1000;
+ }
else {
nbdkit_error ("cannot parse %s in seconds parameter: %s",
key, value);
@@ -79,7 +86,7 @@ delay (int ms)
if (ms > 0) {
const struct timespec ts = {
.tv_sec = ms / 1000,
- .tv_nsec = (ms * 1000000) % 1000000000
+ .tv_nsec = (ms % 1000) * 1000000,
};
nanosleep (&ts, NULL);
}
--
2.20.1
4 years, 10 months
[PATCH libnbd] api: New nbd_kill_command API for sending a signal to the command subprocess.
by Richard W.M. Jones
Reverts commit 387cbe67c3db27e8a61117fedb6e7fad76e409ef.
---
generator/generator | 18 +++++++++++++++++-
lib/handle.c | 28 +++++++++++++++++++++++++++-
tests/closure-lifetimes.c | 4 +++-
3 files changed, 47 insertions(+), 3 deletions(-)
diff --git a/generator/generator b/generator/generator
index 2cd83f1..25e4aa5 100755
--- a/generator/generator
+++ b/generator/generator
@@ -1191,7 +1191,9 @@ has been made.";
longdesc = "\
Run the command as a subprocess and connect to it over
stdin/stdout. This is for use with NBD servers which can
-behave like inetd clients, such as C<nbdkit --single>.";
+behave like inetd clients, such as C<nbdkit --single>.
+
+See also C<nbd_kill_command>.";
};
"read_only", {
@@ -2244,6 +2246,20 @@ The release number is incremented for each release along a particular
branch.";
};
+ "kill_command", {
+ default_call with
+ args = [ Int "signal" ]; ret = RErr;
+ shortdesc = "kill server running as a subprocess";
+ longdesc = "\
+This call may be used to kill the server running as a subprocess
+that was previously created using C<nbd_connect_command>. You
+do not need to use this call. It is only needed if the server
+does not exit when the socket is closed.
+
+The C<signal> flag is the optional signal number to send
+(see L<signal(7)>). If signal is C<0> then C<SIGTERM> is sent.";
+ };
+
"supports_tls", {
default_call with
args = []; ret = RBool; is_locked = false; may_set_error = false;
diff --git a/lib/handle.c b/lib/handle.c
index 1fe4467..ccfea42 100644
--- a/lib/handle.c
+++ b/lib/handle.c
@@ -22,6 +22,8 @@
#include <stdlib.h>
#include <unistd.h>
#include <errno.h>
+#include <signal.h>
+#include <assert.h>
#include <netdb.h>
#include <sys/types.h>
#include <sys/wait.h>
@@ -124,7 +126,7 @@ nbd_close (struct nbd_handle *h)
freeaddrinfo (h->result);
if (h->sock)
h->sock->ops->close (h->sock);
- if (h->pid >= 0) /* XXX kill it? */
+ if (h->pid > 0)
waitpid (h->pid, NULL, 0);
free (h->export_name);
@@ -215,6 +217,30 @@ nbd_unlocked_get_version (struct nbd_handle *h)
return PACKAGE_VERSION;
}
+int
+nbd_unlocked_kill_command (struct nbd_handle *h, int signal)
+{
+ if (h->pid == -1) {
+ set_error (ESRCH, "no subprocess exists");
+ return -1;
+ }
+ assert (h->pid > 0);
+
+ if (signal == 0)
+ signal = SIGTERM;
+ if (signal < 0) {
+ set_error (EINVAL, "invalid signal number: %d", signal);
+ return -1;
+ }
+
+ if (kill (h->pid, signal) == -1) {
+ set_error (errno, "kill");
+ return -1;
+ }
+
+ return 0;
+}
+
/* NB: is_locked = false, may_set_error = false. */
int
nbd_unlocked_supports_tls (struct nbd_handle *h)
diff --git a/tests/closure-lifetimes.c b/tests/closure-lifetimes.c
index 910bdf4..3ddf4c1 100644
--- a/tests/closure-lifetimes.c
+++ b/tests/closure-lifetimes.c
@@ -35,7 +35,7 @@ static char *nbdkit_delay[] =
{ "nbdkit", "-s", "--exit-with-parent", "-v",
"--filter=delay",
"null", "size=512",
- "delay-read=3",
+ "delay-read=10",
NULL };
static unsigned debug_fn_valid;
@@ -134,6 +134,7 @@ main (int argc, char *argv[])
assert (read_cb_free == 1);
assert (completion_cb_free == 1);
+ nbd_kill_command (nbd, 0);
nbd_close (nbd);
/* Test command callbacks are freed if the handle is closed without
@@ -149,6 +150,7 @@ main (int argc, char *argv[])
read_cb, NULL,
completion_cb, NULL, 0);
if (cookie == -1) NBD_ERROR;
+ nbd_kill_command (nbd, 0);
nbd_close (nbd);
assert (read_cb_free == 1);
--
2.22.0
4 years, 10 months
[libnbd PATCH] lib: Call read/extent(FREE) before callback(VALID|FREE)
by Eric Blake
Some callers of nbd_aio_pread_structured_callback or
nbd_aio_block_status_callback share the same opaque pointer to both
calls, with the intent to only free the pointer on the completion
callback. Although our documentation does not explicitly specify the
order in which callbacks are made, allowing the callback(FREE) to
occur prior to the read(FREE) puts a burden on the client to not
dereference the user_data in the read() function if the VALID bit is
not set. If the client follows our suggested practice of an immediate
return 0 if the VAILD bit is not set, then this is the case, but we
can't guarantee all clients will use that pattern. And, it's fairly
easy to guarantee that ALL of our read/extent callbacks are made prior
to any of our callback() use, by utilizing the FINISH state of
structured replies to catch the few cases where we are unable to send
VALID|FREE.
Note that nbd_internal_retire_and_free_command still has to check
whether we have freed the callbacks, as nbd_close() can strand
commands before the FINISH state.
---
generator/states-reply-structured.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/generator/states-reply-structured.c b/generator/states-reply-structured.c
index 20b0e9e..ff5b727 100644
--- a/generator/states-reply-structured.c
+++ b/generator/states-reply-structured.c
@@ -540,11 +540,20 @@ valid_flags (struct nbd_handle *h)
return 0;
REPLY.STRUCTURED_REPLY.FINISH:
+ struct command *cmd = h->reply_cmd;
uint16_t flags;
flags = be16toh (h->sbuf.sr.structured_reply.flags);
- if (flags & NBD_REPLY_FLAG_DONE)
+ if (flags & NBD_REPLY_FLAG_DONE) {
+ if (cmd->type == NBD_CMD_BLOCK_STATUS && cmd->cb.fn.extent)
+ cmd->cb.fn.extent (LIBNBD_CALLBACK_FREE, cmd->cb.fn_user_data,
+ NULL, 0, NULL, 0, NULL);
+ if (cmd->type == NBD_CMD_READ && cmd->cb.fn.read)
+ cmd->cb.fn.read (LIBNBD_CALLBACK_FREE, cmd->cb.fn_user_data,
+ NULL, 0, 0, 0, NULL);
+ cmd->cb.fn.read = NULL;
SET_NEXT_STATE (%^FINISH_COMMAND);
+ }
else {
h->reply_cmd = NULL;
SET_NEXT_STATE (%.READY);
--
2.20.1
4 years, 10 months
[libnbd PATCH] lib: Reduce number of read/block_status callbacks
by Eric Blake
When the server sets NBD_REPLY_FLAG_DONE on a data or block status
chunk, we can use that fact to pass (VALID|FREE) and avoid a separate
callback later just for FREE.
---
As I've been promising in other threads...
generator/states-reply-structured.c | 33 +++++++++++++++++++++++++----
1 file changed, 29 insertions(+), 4 deletions(-)
diff --git a/generator/states-reply-structured.c b/generator/states-reply-structured.c
index 2ef8d20..20b0e9e 100644
--- a/generator/states-reply-structured.c
+++ b/generator/states-reply-structured.c
@@ -18,6 +18,19 @@
/* State machine for parsing structured replies from the server. */
+static unsigned
+valid_flags (struct nbd_handle *h)
+{
+ unsigned valid = LIBNBD_CALLBACK_VALID;
+ uint16_t flags = be16toh (h->sbuf.sr.structured_reply.flags);
+
+ if (flags & NBD_REPLY_FLAG_DONE)
+ valid |= LIBNBD_CALLBACK_FREE;
+ return valid;
+}
+
+/*----- End of prologue. -----*/
+
/* STATE MACHINE */ {
REPLY.STRUCTURED_REPLY.START:
/* We've only read the simple_reply. The structured_reply is longer,
@@ -293,16 +306,19 @@
}
if (cmd->type == NBD_CMD_READ && cmd->cb.fn.read) {
int scratch = error;
+ unsigned valid = valid_flags (h);
/* Different from successful reads: inform the callback about the
* current error rather than any earlier one. If the callback fails
* without setting errno, then use the server's error below.
*/
- if (cmd->cb.fn.read (LIBNBD_CALLBACK_VALID, cmd->cb.fn_user_data,
+ if (cmd->cb.fn.read (valid, cmd->cb.fn_user_data,
cmd->data + (offset - cmd->offset),
0, offset, LIBNBD_READ_ERROR, &scratch) == -1)
if (cmd->error == 0)
cmd->error = scratch;
+ if (valid & LIBNBD_CALLBACK_FREE)
+ cmd->cb.fn.read = NULL; /* because we've freed it */
}
}
@@ -384,13 +400,16 @@
assert (cmd); /* guaranteed by CHECK */
if (cmd->cb.fn.read) {
int error = cmd->error;
+ unsigned valid = valid_flags (h);
- if (cmd->cb.fn.read (LIBNBD_CALLBACK_VALID, cmd->cb.fn_user_data,
+ if (cmd->cb.fn.read (valid, cmd->cb.fn_user_data,
cmd->data + (offset - cmd->offset),
length - sizeof offset, offset,
LIBNBD_READ_DATA, &error) == -1)
if (cmd->error == 0)
cmd->error = error ? error : EPROTO;
+ if (valid & LIBNBD_CALLBACK_FREE)
+ cmd->cb.fn.read = NULL; /* because we've freed it */
}
SET_NEXT_STATE (%FINISH);
@@ -446,13 +465,16 @@
memset (cmd->data + offset, 0, length);
if (cmd->cb.fn.read) {
int error = cmd->error;
+ unsigned valid = valid_flags (h);
- if (cmd->cb.fn.read (LIBNBD_CALLBACK_VALID, cmd->cb.fn_user_data,
+ if (cmd->cb.fn.read (valid, cmd->cb.fn_user_data,
cmd->data + offset, length,
cmd->offset + offset,
LIBNBD_READ_HOLE, &error) == -1)
if (cmd->error == 0)
cmd->error = error ? error : EPROTO;
+ if (valid & LIBNBD_CALLBACK_FREE)
+ cmd->cb.fn.read = NULL; /* because we've freed it */
}
SET_NEXT_STATE(%FINISH);
@@ -498,12 +520,15 @@
if (meta_context) {
/* Call the caller's extent function. */
int error = cmd->error;
+ unsigned valid = valid_flags (h);
- if (cmd->cb.fn.extent (LIBNBD_CALLBACK_VALID, cmd->cb.fn_user_data,
+ if (cmd->cb.fn.extent (valid, cmd->cb.fn_user_data,
meta_context->name, cmd->offset,
&h->bs_entries[1], (length-4) / 4, &error) == -1)
if (cmd->error == 0)
cmd->error = error ? error : EPROTO;
+ if (valid & LIBNBD_CALLBACK_FREE)
+ cmd->cb.fn.extent = NULL; /* because we've freed it */
}
else
/* Emit a debug message, but ignore it. */
--
2.20.1
4 years, 10 months
[libnbd PATCH] generator: Let nbd_aio_get_direction return unsigned
by Eric Blake
The return value is a bitmask, and is already documented as never
failing, hence it makes sense to return an unsigned value to make it
easier to do bitwise operations without worrying about potential
undefined C semantics on a signed value.
---
I'm not sure if we want this, or even if we do if my OCaml was the
most concise, but it matches the sentiment of our recent effort to let
the valid_flag and status parameters to callbacks be unsigned.
generator/generator | 93 +++++++++++++++++++++++++++++----------------
lib/is-state.c | 2 +-
2 files changed, 61 insertions(+), 34 deletions(-)
diff --git a/generator/generator b/generator/generator
index 2cd83f1..bb3a1b5 100755
--- a/generator/generator
+++ b/generator/generator
@@ -873,6 +873,7 @@ and ret =
| RInt (* return a small int, -1 = error *)
| RInt64 (* 64 bit int, -1 = error *)
| RString (* return a newly allocated string, caller frees *)
+| RUInt (* return a bitmask, no error possible *)
and permitted_state =
| Created (* can be called in the START state *)
| Connecting (* can be called when connecting/handshaking *)
@@ -2013,7 +2014,7 @@ Do not do anything else with the file descriptor.";
"aio_get_direction", {
default_call with
- args = []; ret = RInt; is_locked = false; may_set_error = false;
+ args = []; ret = RUInt; is_locked = false; may_set_error = false;
shortdesc = "return the read or write direction";
longdesc = "\
Return the current direction of this connection, which means
@@ -3040,13 +3041,15 @@ let () =
(* !may_set_error is incompatible with permitted_states != [] because
* an incorrect state will result in set_error being called by the
- * generated wrapper.
+ * generated wrapper. It is also incompatible with RUint.
*)
List.iter (
function
| name, { permitted_states = (_::_); may_set_error = false } ->
failwithf "%s: if may_set_error is false, permitted_states must be empty (any permitted state)"
name
+ | name, { ret = RUInt; may_set_error = true } ->
+ failwithf "%s: if ret is RUInt, may_set_error must be false" name
| _ -> ()
) handle_calls
@@ -3189,6 +3192,7 @@ let print_call name args ret =
| RConstString -> pr "const char *"
| RInt64 -> pr "int64_t "
| RString -> pr "char *"
+ | RUInt -> pr "unsigned "
);
pr "nbd_%s " name;
print_arg_list ~handle:true args
@@ -3329,10 +3333,11 @@ let generate_lib_api_c () =
| RBool
| RErr
| RFd
- | RInt -> "int", "-1"
- | RConstString -> "const char *", "NULL"
- | RInt64 -> "int64_t", "-1"
- | RString -> "char *", "NULL" in
+ | RInt -> "int", Some "-1"
+ | RConstString -> "const char *", Some "NULL"
+ | RInt64 -> "int64_t", Some "-1"
+ | RString -> "char *", Some "NULL"
+ | RUInt -> "unsigned", None in
pr "%s\n" ret_c_type;
pr "nbd_%s " name;
@@ -3347,6 +3352,7 @@ let generate_lib_api_c () =
| RConstString -> pr " const char *ret;\n"
| RInt64 -> pr " int64_t ret;\n"
| RString -> pr " char *ret;\n"
+ | RUInt -> pr " unsigned ret;\n"
);
pr "\n";
if may_set_error then (
@@ -3356,8 +3362,11 @@ let generate_lib_api_c () =
if is_locked then
pr " pthread_mutex_lock (&h->lock);\n";
if permitted_states <> [] then (
+ let value = match errcode with
+ | Some value -> value
+ | None -> assert false in
pr " if (!%s_in_permitted_state (h)) {\n" name;
- pr " ret = %s;\n" errcode;
+ pr " ret = %s;\n" value;
pr " goto out;\n";
pr " }\n"
);
@@ -3412,31 +3421,37 @@ let print_api (name, { args; ret; permitted_states; shortdesc; longdesc;
match ret with
| RBool ->
pr "This call returns a boolean value.\n";
- "-1"
+ Some "-1"
| RConstString ->
pr "This call returns a statically allocated string.\n";
pr "You B<must not> try to free the string.\n";
- "NULL"
+ Some "NULL"
| RErr ->
pr "If the call is successful the function returns C<0>.\n";
- "-1"
+ Some "-1"
| RFd ->
pr "This call returns a file descriptor.\n";
- "-1"
+ Some "-1"
| RInt ->
pr "This call returns an integer E<ge> 0.\n";
- "-1"
+ Some "-1"
| RInt64 ->
pr "This call returns a 64 bit signed integer E<ge> 0.\n";
- "-1"
+ Some "-1"
| RString ->
pr "This call returns a string. The caller must free the\n";
pr "returned string to avoid a memory leak.\n";
- "NULL"
+ Some "NULL"
+ | RUInt ->
+ pr "This call returns a bitmask.\n";
+ None
in
pr "\n";
if may_set_error then (
- pr "On error C<%s> is returned.\n" errcode;
+ let value = match errcode with
+ | Some value -> value
+ | None -> assert false in
+ pr "On error C<%s> is returned.\n" value;
pr "See L<libnbd(3)/ERROR HANDLING> for how to get further details\n";
pr "of the error.\n"
)
@@ -3683,7 +3698,7 @@ PyInit_libnbdmod (void)
}
"
-let print_python_binding name { args; ret } =
+let print_python_binding name { args; ret; may_set_error } =
(* Functions with a Closure parameter are special because we
* have to generate wrapper functions which translate the
* callbacks back to Python.
@@ -3835,6 +3850,7 @@ let print_python_binding name { args; ret } =
| RConstString -> pr " const char *ret;\n"
| RInt64 -> pr " int64_t ret;\n"
| RString -> pr " char *ret;\n";
+ | RUInt -> pr " unsigned ret;\n"
);
pr " PyObject *py_ret;\n";
List.iter (
@@ -4018,14 +4034,17 @@ let print_python_binding name { args; ret } =
| UInt64 n -> pr ", %s_u64" n
) args;
pr ");\n";
- (match ret with
- | RBool | RErr | RFd | RInt | RInt64 -> pr " if (ret == -1) {\n";
- | RConstString | RString -> pr " if (ret == NULL) {\n";
+ if may_set_error then (
+ (match ret with
+ | RBool | RErr | RFd | RInt | RInt64 -> pr " if (ret == -1) {\n";
+ | RConstString | RString -> pr " if (ret == NULL) {\n";
+ | RUInt -> assert false
+ );
+ pr " raise_exception ();\n";
+ pr " py_ret = NULL;\n";
+ pr " goto out;\n";
+ pr " }\n"
);
- pr " raise_exception ();\n";
- pr " py_ret = NULL;\n";
- pr " goto out;\n";
- pr " }\n";
(* Convert the result back to a Python object and return it. *)
let use_ret = ref true in
@@ -4062,7 +4081,8 @@ let print_python_binding name { args; ret } =
pr " py_ret = Py_None;\n";
pr " Py_INCREF (py_ret);\n"
| RFd
- | RInt ->
+ | RInt
+ | RUInt ->
pr " py_ret = PyLong_FromLong (ret);\n"
| RInt64 ->
pr " py_ret = PyLong_FromLongLong (ret);\n"
@@ -4072,7 +4092,9 @@ let print_python_binding name { args; ret } =
);
pr "\n";
- pr " out:\n";
+ if may_set_error then (
+ pr " out:\n"
+ );
List.iter (
function
| ArrayAndLen (UInt32 n, len) -> pr " free (%s);\n" n
@@ -4347,6 +4369,7 @@ and ocaml_ret_to_string = function
| RInt -> "int"
| RInt64 -> "int64"
| RString -> "string"
+ | RUInt -> "int"
let rec name_of_ocaml_arg = function
| OCamlHandle -> "h"
@@ -4747,9 +4770,10 @@ let print_ocaml_binding (name, { args; ret }) =
let errcode =
match ret with
- | RBool | RErr | RFd | RInt | RInt64 -> pr " int r;\n"; "-1"
- | RConstString -> pr " const char *r;\n"; "NULL"
- | RString -> pr " char *r;\n"; "NULL" in
+ | RBool | RErr | RFd | RInt | RInt64 -> pr " int r;\n"; Some "-1"
+ | RConstString -> pr " const char *r;\n"; Some "NULL"
+ | RString -> pr " char *r;\n"; Some "NULL"
+ | RUInt -> pr " unsigned r;\n"; None in
pr "\n";
pr " caml_enter_blocking_section ();\n";
pr " r = nbd_%s " name;
@@ -4757,14 +4781,17 @@ let print_ocaml_binding (name, { args; ret }) =
pr ";\n";
pr " caml_leave_blocking_section ();\n";
pr "\n";
- pr " if (r == %s)\n" errcode;
- pr " nbd_internal_ocaml_raise_error ();\n";
- pr "\n";
+ (match errcode with
+ | Some code ->
+ pr " if (r == %s)\n" code;
+ pr " nbd_internal_ocaml_raise_error ();\n";
+ pr "\n"
+ | None -> ()
+ );
(match ret with
| RBool -> pr " rv = Val_bool (r);\n"
| RErr -> pr " rv = Val_unit;\n"
- | RFd -> pr " rv = Val_int (r);\n"
- | RInt -> pr " rv = Val_int (r);\n"
+ | RFd | RInt | RUInt -> pr " rv = Val_int (r);\n"
| RInt64 -> pr " rv = caml_copy_int64 (r);\n"
| RConstString -> pr " rv = caml_copy_string (r);\n"
| RString ->
diff --git a/lib/is-state.c b/lib/is-state.c
index d3335fb..1a85c7a 100644
--- a/lib/is-state.c
+++ b/lib/is-state.c
@@ -144,7 +144,7 @@ nbd_unlocked_aio_is_closed (struct nbd_handle *h)
return nbd_internal_is_state_closed (get_public_state (h));
}
-int
+unsigned
nbd_unlocked_aio_get_direction (struct nbd_handle *h)
{
return nbd_internal_aio_get_direction (get_public_state (h));
--
2.20.1
4 years, 10 months
