[nbdkit PATCH] nbd: Another libnbd version bump
by Eric Blake
The 0.9.8 release breaks API, requiring a number of changes:
- Use symbolic constants instead of magic numbers/open-coded strings
(well, the string for "base:allocation" was present before this
libnbd bump)
- Change callbacks to drop the valid_flag parameter
- Add _is to nbd_read_only call
- Drop the _callback suffix on nbd_aio_FOO calls
- Add a struct for managing callback/user_data at once
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
Pushing, since we're releasing libnbd 0.9.8 today
configure.ac | 4 +--
plugins/nbd/nbd.c | 85 +++++++++++++++++++++--------------------------
README | 2 +-
3 files changed, 41 insertions(+), 50 deletions(-)
diff --git a/configure.ac b/configure.ac
index ee14516d..0b54c00a 100644
--- a/configure.ac
+++ b/configure.ac
@@ -721,12 +721,12 @@ AC_ARG_WITH([libnbd],
[],
[with_libnbd=check])
AS_IF([test "$with_libnbd" != "no"],[
- PKG_CHECK_MODULES([LIBNBD], [libnbd >= 0.9.6],[
+ PKG_CHECK_MODULES([LIBNBD], [libnbd >= 0.9.8],[
AC_SUBST([LIBNBD_CFLAGS])
AC_SUBST([LIBNBD_LIBS])
AC_DEFINE([HAVE_LIBNBD],[1],[libnbd found at compile time.])
],
- [AC_MSG_WARN([libnbd >= 0.9.6 not found, nbd plugin will be crippled])])
+ [AC_MSG_WARN([libnbd >= 0.9.8 not found, nbd plugin will be crippled])])
])
AM_CONDITIONAL([HAVE_LIBNBD], [test "x$LIBNBD_LIBS" != "x"])
diff --git a/plugins/nbd/nbd.c b/plugins/nbd/nbd.c
index f11e54d5..09c8891e 100644
--- a/plugins/nbd/nbd.c
+++ b/plugins/nbd/nbd.c
@@ -62,7 +62,7 @@ struct transaction {
sem_t sem;
uint32_t early_err;
uint32_t err;
- struct nbdkit_extents *extents;
+ nbd_completion_callback cb;
};
/* The per-connection handle */
@@ -160,11 +160,12 @@ nbdplug_config (const char *key, const char *value)
if (strcasecmp (value, "require") == 0 ||
strcasecmp (value, "required") == 0 ||
strcasecmp (value, "force") == 0)
- tls = 2;
+ tls = LIBNBD_TLS_REQUIRE;
else {
- tls = nbdkit_parse_bool (value);
- if (tls == -1)
+ r = nbdkit_parse_bool (value);
+ if (r == -1)
exit (EXIT_FAILURE);
+ tls = r ? LIBNBD_TLS_ALLOW : LIBNBD_TLS_DISABLE;
}
}
else if (strcmp (key, "tls-certificates") == 0) {
@@ -245,8 +246,9 @@ nbdplug_config_complete (void)
export = "";
if (tls == -1)
- tls = tls_certificates || tls_verify >= 0 || tls_username || tls_psk;
- if (tls > 0) {
+ tls = (tls_certificates || tls_verify >= 0 || tls_username || tls_psk)
+ ? LIBNBD_TLS_ALLOW : LIBNBD_TLS_DISABLE;
+ if (tls != LIBNBD_TLS_DISABLE) {
struct nbd_handle *nbd = nbd_create ();
if (!nbd) {
@@ -345,23 +347,12 @@ nbdplug_reader (void *handle)
return NULL;
}
-/* Prepare for a transaction. */
-static void
-nbdplug_prepare (struct transaction *trans)
-{
- memset (trans, 0, sizeof *trans);
- if (sem_init (&trans->sem, 0, 0))
- assert (false);
-}
-
+/* Callback used at end of a transaction. */
static int
-nbdplug_notify (unsigned valid_flag, void *opaque, int *error)
+nbdplug_notify (void *opaque, int *error)
{
struct transaction *trans = opaque;
- if (!(valid_flag & LIBNBD_CALLBACK_VALID))
- return 0;
-
/* There's a possible race here where trans->cookie has not yet been
* updated by nbdplug_register, but it's only an informational
* message.
@@ -376,6 +367,17 @@ nbdplug_notify (unsigned valid_flag, void *opaque, int *error)
return 1;
}
+/* Prepare for a transaction. */
+static void
+nbdplug_prepare (struct transaction *trans)
+{
+ memset (trans, 0, sizeof *trans);
+ if (sem_init (&trans->sem, 0, 0))
+ assert (false);
+ trans->cb.callback = nbdplug_notify;
+ trans->cb.user_data = trans;
+}
+
/* Register a cookie and kick the I/O thread. */
static void
nbdplug_register (struct handle *h, struct transaction *trans, int64_t cookie)
@@ -466,7 +468,7 @@ nbdplug_open_handle (int readonly)
goto err;
if (nbd_set_export_name (h->nbd, export) == -1)
goto err;
- if (nbd_add_meta_context (h->nbd, "base:allocation") == -1)
+ if (nbd_add_meta_context (h->nbd, LIBNBD_CONTEXT_BASE_ALLOCATION) == -1)
goto err;
if (nbd_set_tls (h->nbd, tls) == -1)
goto err;
@@ -570,7 +572,7 @@ static int
nbdplug_can_write (void *handle)
{
struct handle *h = handle;
- int i = nbd_read_only (h->nbd);
+ int i = nbd_is_read_only (h->nbd);
if (i == -1) {
nbdkit_error ("failure to check readonly flag: %s", nbd_get_error ());
@@ -674,7 +676,7 @@ static int
nbdplug_can_extents (void *handle)
{
struct handle *h = handle;
- int i = nbd_can_meta_context (h->nbd, "base:allocation");
+ int i = nbd_can_meta_context (h->nbd, LIBNBD_CONTEXT_BASE_ALLOCATION);
if (i == -1) {
nbdkit_error ("failure to check extents ability: %s", nbd_get_error ());
@@ -693,8 +695,8 @@ nbdplug_pread (void *handle, void *buf, uint32_t count, uint64_t offset,
assert (!flags);
nbdplug_prepare (&s);
- nbdplug_register (h, &s, nbd_aio_pread_callback (h->nbd, buf, count, offset,
- nbdplug_notify, &s, 0));
+ nbdplug_register (h, &s, nbd_aio_pread (h->nbd, buf, count, offset,
+ s.cb, 0));
return nbdplug_reply (h, &s);
}
@@ -709,8 +711,8 @@ nbdplug_pwrite (void *handle, const void *buf, uint32_t count, uint64_t offset,
assert (!(flags & ~NBDKIT_FLAG_FUA));
nbdplug_prepare (&s);
- nbdplug_register (h, &s, nbd_aio_pwrite_callback (h->nbd, buf, count, offset,
- nbdplug_notify, &s, f));
+ nbdplug_register (h, &s, nbd_aio_pwrite (h->nbd, buf, count, offset,
+ s.cb, f));
return nbdplug_reply (h, &s);
}
@@ -729,8 +731,7 @@ nbdplug_zero (void *handle, uint32_t count, uint64_t offset, uint32_t flags)
if (flags & NBDKIT_FLAG_FUA)
f |= LIBNBD_CMD_FLAG_FUA;
nbdplug_prepare (&s);
- nbdplug_register (h, &s, nbd_aio_zero_callback (h->nbd, count, offset,
- nbdplug_notify, &s, f));
+ nbdplug_register (h, &s, nbd_aio_zero (h->nbd, count, offset, s.cb, f));
return nbdplug_reply (h, &s);
}
@@ -744,8 +745,7 @@ nbdplug_trim (void *handle, uint32_t count, uint64_t offset, uint32_t flags)
assert (!(flags & ~NBDKIT_FLAG_FUA));
nbdplug_prepare (&s);
- nbdplug_register (h, &s, nbd_aio_trim_callback (h->nbd, count, offset,
- nbdplug_notify, &s, f));
+ nbdplug_register (h, &s, nbd_aio_trim (h->nbd, count, offset, s.cb, f));
return nbdplug_reply (h, &s);
}
@@ -758,23 +758,17 @@ nbdplug_flush (void *handle, uint32_t flags)
assert (!flags);
nbdplug_prepare (&s);
- nbdplug_register (h, &s, nbd_aio_flush_callback (h->nbd,
- nbdplug_notify, &s, 0));
+ nbdplug_register (h, &s, nbd_aio_flush (h->nbd, s.cb, 0));
return nbdplug_reply (h, &s);
}
static int
-nbdplug_extent (unsigned valid_flag, void *opaque,
- const char *metacontext, uint64_t offset,
+nbdplug_extent (void *opaque, const char *metacontext, uint64_t offset,
uint32_t *entries, size_t nr_entries, int *error)
{
- struct transaction *trans = opaque;
- struct nbdkit_extents *extents = trans->extents;
+ struct nbdkit_extents *extents = opaque;
- if (!(valid_flag & LIBNBD_CALLBACK_VALID))
- return 0;
-
- assert (strcmp (metacontext, "base:allocation") == 0);
+ assert (strcmp (metacontext, LIBNBD_CONTEXT_BASE_ALLOCATION) == 0);
assert (nr_entries % 2 == 0);
while (nr_entries) {
/* We rely on the fact that NBDKIT_EXTENT_* match NBD_STATE_* */
@@ -797,14 +791,12 @@ nbdplug_extents (void *handle, uint32_t count, uint64_t offset,
struct handle *h = handle;
struct transaction s;
uint32_t f = flags & NBDKIT_FLAG_REQ_ONE ? LIBNBD_CMD_FLAG_REQ_ONE : 0;
+ nbd_extent_callback extcb = { nbdplug_extent, extents };
assert (!(flags & ~NBDKIT_FLAG_REQ_ONE));
nbdplug_prepare (&s);
- s.extents = extents;
- nbdplug_register (h, &s, nbd_aio_block_status_callback (h->nbd, count, offset,
- nbdplug_extent, &s,
- nbdplug_notify, &s,
- f));
+ nbdplug_register (h, &s, nbd_aio_block_status (h->nbd, count, offset,
+ extcb, s.cb, f));
return nbdplug_reply (h, &s);
}
@@ -817,8 +809,7 @@ nbdplug_cache (void *handle, uint32_t count, uint64_t offset, uint32_t flags)
assert (!flags);
nbdplug_prepare (&s);
- nbdplug_register (h, &s, nbd_aio_cache_callback (h->nbd, count, offset,
- nbdplug_notify, &s, 0));
+ nbdplug_register (h, &s, nbd_aio_cache (h->nbd, count, offset, s.cb, 0));
return nbdplug_reply (h, &s);
}
diff --git a/README b/README
index 06c16dd1..b78f490a 100644
--- a/README
+++ b/README
@@ -113,7 +113,7 @@ For the linuxdisk plugin:
For the nbd plugin, to get URI and TLS support:
- - libnbd >= 0.9.6
+ - libnbd >= 0.9.8
For the Perl, example4 and tar plugins:
--
2.20.1
4 years, 9 months
[libnbd PATCH] docs: Ensure .3 files get built
by Eric Blake
Running 'make clean && make' loses all the .3 man pages produced from
generated .pod files. Why? When Makefile.inc does not exist (on the
first run), the value of $(api_built) is updated prior to re-reading
the Makefile, so when the 'all-am: Makefile $(MANS)' dependency from
automake is encountered, $(MANS) includes all the .3 files. But after
'make clean', Makefile.inc still exists, and it is included late
enough that $(MANS) still contains the pre-inclusion state, omitting
all the generated additions. The solution is to add yet another
dependency, which Automake leaves late enough to always occur after
$(MANS) has its full value.
Reported-by: Richard W.M. Jones <rjones(a)redhat.com>
Fixes: 0a8d3987
---
Counter-proposal for Rich's change. I'll go ahead and push it.
docs/Makefile.am | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/docs/Makefile.am b/docs/Makefile.am
index 30a972d..dd34a3a 100644
--- a/docs/Makefile.am
+++ b/docs/Makefile.am
@@ -21,8 +21,12 @@ api_built=
# Our use of sinclude to bypass Automake is intentional; this file won't
# exist in a fresh git checkout until after the generator has run, but
-# should already be present in any released tarball.
+# should already be present in any released tarball. But, since automake
+# can't see into this file, it did not hoist the resulting prerequisites
+# prior to its 'all-am: Makefile $(MANS)' rule, which gets parsed before
+# $(MANS) has grown in size, so we have to add a second all-am dependency.
sinclude $(srcdir)/Makefile.inc
+all-am: $(api_built:%=%.3)
generator_built = \
Makefile.inc \
--
2.20.1
4 years, 9 months
[PATCH libnbd] docs: Change docs/Makefile.inc back to a regular include, readd to git.
by Richard W.M. Jones
‘make clean && make’ was not rebuilding the docs/*.3 files. The
reason is obscure:
- docs/Makefile has rules:
MANS = $(man_MANS)
all: all-am
all-am: Makefile $(MANS)
- sinclude docs/Makefile.inc happened long after MANS is defined, so
MANS held the earlier version of $(man_MANS) without the api-built
man pages listed.
This was confirmed by looking at the output of ‘make -d -C docs all-am’
and examining what happens when rebuilding all-am.
The fix is to change docs/Makefile.inc back to a regular include,
which also means we have to readd this generated file to git to allow
cold clones to work.
Partly reverts commit 15ca7acd6f46188d6f713af8785e35336b0b71f7.
---
.gitignore | 1 -
docs/Makefile.am | 8 +---
docs/Makefile.inc | 98 +++++++++++++++++++++++++++++++++++++++++++++
generator/generator | 2 +-
4 files changed, 100 insertions(+), 9 deletions(-)
diff --git a/.gitignore b/.gitignore
index ccbbc14..4109865 100644
--- a/.gitignore
+++ b/.gitignore
@@ -39,7 +39,6 @@ Makefile.in
/depcomp
/docs/*.3
/docs/*.pod
-/docs/Makefile.inc
!/docs/libnbd.pod
!/docs/nbd_close.3
!/docs/nbd_create.pod
diff --git a/docs/Makefile.am b/docs/Makefile.am
index 30a972d..9a3f47b 100644
--- a/docs/Makefile.am
+++ b/docs/Makefile.am
@@ -16,13 +16,7 @@
# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
include $(top_srcdir)/subdir-rules.mk
-
-api_built=
-
-# Our use of sinclude to bypass Automake is intentional; this file won't
-# exist in a fresh git checkout until after the generator has run, but
-# should already be present in any released tarball.
-sinclude $(srcdir)/Makefile.inc
+include $(srcdir)/Makefile.inc
generator_built = \
Makefile.inc \
diff --git a/docs/Makefile.inc b/docs/Makefile.inc
new file mode 100644
index 0000000..cd620b9
--- /dev/null
+++ b/docs/Makefile.inc
@@ -0,0 +1,98 @@
+# NBD client library in userspace
+# WARNING: THIS FILE IS GENERATED FROM
+# generator/generator
+# ANY CHANGES YOU MAKE TO THIS FILE WILL BE LOST.
+#
+# Copyright (C) 2013-2019 Red Hat Inc.
+#
+# This library is free software; you can redistribute it and/or
+# modify it under the terms of the GNU Lesser General Public
+# License as published by the Free Software Foundation; either
+# version 2 of the License, or (at your option) any later version.
+#
+# This library is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+# Lesser General Public License for more details.
+#
+# You should have received a copy of the GNU Lesser General Public
+# License along with this library; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+
+api_built = \
+ nbd_set_debug \
+ nbd_get_debug \
+ nbd_set_debug_callback \
+ nbd_clear_debug_callback \
+ nbd_set_handle_name \
+ nbd_get_handle_name \
+ nbd_set_export_name \
+ nbd_get_export_name \
+ nbd_set_tls \
+ nbd_get_tls \
+ nbd_set_tls_certificates \
+ nbd_set_tls_verify_peer \
+ nbd_get_tls_verify_peer \
+ nbd_set_tls_username \
+ nbd_get_tls_username \
+ nbd_set_tls_psk_file \
+ nbd_add_meta_context \
+ nbd_connect_uri \
+ nbd_connect_unix \
+ nbd_connect_tcp \
+ nbd_connect_command \
+ nbd_is_read_only \
+ nbd_can_flush \
+ nbd_can_fua \
+ nbd_is_rotational \
+ nbd_can_trim \
+ nbd_can_zero \
+ nbd_can_df \
+ nbd_can_multi_conn \
+ nbd_can_cache \
+ nbd_can_meta_context \
+ nbd_get_size \
+ nbd_pread \
+ nbd_pread_structured \
+ nbd_pwrite \
+ nbd_shutdown \
+ nbd_flush \
+ nbd_trim \
+ nbd_cache \
+ nbd_zero \
+ nbd_block_status \
+ nbd_poll \
+ nbd_aio_connect \
+ nbd_aio_connect_uri \
+ nbd_aio_connect_unix \
+ nbd_aio_connect_tcp \
+ nbd_aio_connect_command \
+ nbd_aio_pread \
+ nbd_aio_pread_structured \
+ nbd_aio_pwrite \
+ nbd_aio_disconnect \
+ nbd_aio_flush \
+ nbd_aio_trim \
+ nbd_aio_cache \
+ nbd_aio_zero \
+ nbd_aio_block_status \
+ nbd_aio_get_fd \
+ nbd_aio_get_direction \
+ nbd_aio_notify_read \
+ nbd_aio_notify_write \
+ nbd_aio_is_created \
+ nbd_aio_is_connecting \
+ nbd_aio_is_ready \
+ nbd_aio_is_processing \
+ nbd_aio_is_dead \
+ nbd_aio_is_closed \
+ nbd_aio_command_completed \
+ nbd_aio_peek_command_completed \
+ nbd_aio_in_flight \
+ nbd_connection_state \
+ nbd_get_package_name \
+ nbd_get_version \
+ nbd_kill_subprocess \
+ nbd_supports_tls \
+ nbd_supports_uri \
+ $(NULL)
diff --git a/generator/generator b/generator/generator
index 6cc06cc..437f432 100755
--- a/generator/generator
+++ b/generator/generator
@@ -3766,7 +3766,7 @@ let generate_lib_api_c () =
let generate_docs_Makefile_inc () =
generate_header HashStyle;
- pr "api_built += \\\n";
+ pr "api_built = \\\n";
List.iter (
fun (name, _) ->
pr "\tnbd_%s \\\n" name;
--
2.22.0
4 years, 9 months
[PATCH libnbd] api: Rename nbd_kill_command -> nbd_kill_subprocess.
by Richard W.M. Jones
This is a simple renaming to avoid confusion over "command" when used
to mean an asynchronous request to the server and "command" meaning
the subprocess forked previously by libnbd.
---
generator/generator | 4 ++--
lib/handle.c | 2 +-
tests/closure-lifetimes.c | 4 ++--
tests/server-death.c | 4 ++--
4 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/generator/generator b/generator/generator
index 13cb0b9..6cc06cc 100755
--- a/generator/generator
+++ b/generator/generator
@@ -1315,7 +1315,7 @@ 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>.
-See also L<nbd_kill_command(3)>.";
+See also L<nbd_kill_subprocess(3)>.";
};
"is_read_only", {
@@ -2249,7 +2249,7 @@ The release number is incremented for each release along a particular
branch.";
};
- "kill_command", {
+ "kill_subprocess", {
default_call with
args = [ Int "signum" ]; ret = RErr;
shortdesc = "kill server running as a subprocess";
diff --git a/lib/handle.c b/lib/handle.c
index d599eef..b508744 100644
--- a/lib/handle.c
+++ b/lib/handle.c
@@ -254,7 +254,7 @@ nbd_unlocked_get_version (struct nbd_handle *h)
}
int
-nbd_unlocked_kill_command (struct nbd_handle *h, int signum)
+nbd_unlocked_kill_subprocess (struct nbd_handle *h, int signum)
{
if (h->pid == -1) {
set_error (ESRCH, "no subprocess exists");
diff --git a/tests/closure-lifetimes.c b/tests/closure-lifetimes.c
index 5a7cd9c..70788b6 100644
--- a/tests/closure-lifetimes.c
+++ b/tests/closure-lifetimes.c
@@ -143,7 +143,7 @@ main (int argc, char *argv[])
assert (read_cb_freed == 1);
assert (completion_cb_freed == 1);
- nbd_kill_command (nbd, 0);
+ nbd_kill_subprocess (nbd, 0);
nbd_close (nbd);
/* Test command callbacks are freed if the handle is closed without
@@ -162,7 +162,7 @@ main (int argc, char *argv[])
.free = completion_cb_free },
0);
if (cookie == -1) NBD_ERROR;
- nbd_kill_command (nbd, 0);
+ nbd_kill_subprocess (nbd, 0);
nbd_close (nbd);
assert (read_cb_freed == 1);
diff --git a/tests/server-death.c b/tests/server-death.c
index f572eab..0e9add0 100644
--- a/tests/server-death.c
+++ b/tests/server-death.c
@@ -102,8 +102,8 @@ main (int argc, char *argv[])
* actually exiting), although it's a race whether our signal
* arrives while nbdkit has a pending transaction.
*/
- if (nbd_kill_command (nbd, SIGKILL) == -1) {
- fprintf (stderr, "%s: test failed: nbd_kill_command: %s\n", argv[0],
+ if (nbd_kill_subprocess (nbd, SIGKILL) == -1) {
+ fprintf (stderr, "%s: test failed: nbd_kill_subprocess: %s\n", argv[0],
nbd_get_error ());
exit (EXIT_FAILURE);
}
--
2.22.0
4 years, 9 months
[nbdkit PATCH] data, memory: Optimize .zero > PAGE_SIZE
by Eric Blake
When sparse_array_zero() is used for a range larger than a page,
there's no need to waste time in memset() or is_zero() - we already
know the page will be free()d.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
Here's a fun one :)
common/sparse/sparse.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/common/sparse/sparse.c b/common/sparse/sparse.c
index cb44743c..5e085763 100644
--- a/common/sparse/sparse.c
+++ b/common/sparse/sparse.c
@@ -343,10 +343,13 @@ sparse_array_zero (struct sparse_array *sa, uint32_t count, uint64_t offset)
n = count;
if (p) {
- memset (p, 0, n);
+ if (n < PAGE_SIZE)
+ memset (p, 0, n);
+ else
+ assert (p == *l2_page);
/* If the whole page is now zero, free it. */
- if (is_zero (*l2_page, PAGE_SIZE)) {
+ if (n == PAGE_SIZE || is_zero (*l2_page, PAGE_SIZE)) {
if (sa->debug)
nbdkit_debug ("%s: freeing zero page at offset %" PRIu64,
__func__, offset);
--
2.20.1
4 years, 9 months
[libnbd PATCH] lib: Consolidate free callbacks to just happen at retire time
by Eric Blake
When we introduced valid_flags, there was an incentive to do as few
callbacks as possible, favoring cb(VALID|FREE) calls over the sequence
cb(VALID);cb(FREE). To make it work, we set .callback=NULL after an
early free, so that the later check during retirement didn't free
again.
But now that our .free callback is distinct from our other callbacks,
there is no longer an advantage to bundling things, and a significant
reduction in lines of code by just doing it in one place. This
basically reverts 9c8fccdf (which had slightly-questionable C
type-punning anyway) and 57150880.
---
lib/internal.h | 14 --------------
generator/states-reply-simple.c | 1 -
generator/states-reply-structured.c | 20 +-------------------
generator/states-reply.c | 1 -
generator/states.c | 1 -
lib/aio.c | 11 ++++++-----
lib/debug.c | 4 +++-
7 files changed, 10 insertions(+), 42 deletions(-)
diff --git a/lib/internal.h b/lib/internal.h
index dc3676a..1971613 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -277,20 +277,6 @@ struct command {
#define CALL_CALLBACK(cb, ...) \
(cb).callback ((cb).user_data, ##__VA_ARGS__)
-/* Free a callback.
- *
- * Note this works for any type of callback because the basic layout
- * of the struct is the same for all of them. Therefore casting cb to
- * nbd_completion_callback does not change the effective code.
- */
-#define FREE_CALLBACK(cb) \
- do { \
- nbd_completion_callback *_cb = (nbd_completion_callback *)&(cb); \
- if (_cb->callback != NULL && _cb->free != NULL) \
- _cb->free (_cb->user_data); \
- _cb->callback = NULL; \
- } while (0)
-
/* aio.c */
extern void nbd_internal_retire_and_free_command (struct command *);
diff --git a/generator/states-reply-simple.c b/generator/states-reply-simple.c
index 8e3d7f1..2f83e6f 100644
--- a/generator/states-reply-simple.c
+++ b/generator/states-reply-simple.c
@@ -69,7 +69,6 @@
cmd->offset, LIBNBD_READ_DATA,
&error) == -1)
cmd->error = error ? error : EPROTO;
- FREE_CALLBACK (cmd->cb.fn.chunk);
}
SET_NEXT_STATE (%^FINISH_COMMAND);
diff --git a/generator/states-reply-structured.c b/generator/states-reply-structured.c
index 2e327ce..58e83d4 100644
--- a/generator/states-reply-structured.c
+++ b/generator/states-reply-structured.c
@@ -295,7 +295,6 @@
}
if (cmd->type == NBD_CMD_READ && cmd->cb.fn.chunk.callback) {
int scratch = error;
- uint16_t flags = be16toh (h->sbuf.sr.structured_reply.flags);
/* Different from successful reads: inform the callback about the
* current error rather than any earlier one. If the callback fails
@@ -307,8 +306,6 @@
&scratch) == -1)
if (cmd->error == 0)
cmd->error = scratch;
- if (flags & NBD_REPLY_FLAG_DONE)
- FREE_CALLBACK (cmd->cb.fn.chunk);
}
}
@@ -390,15 +387,12 @@
assert (cmd); /* guaranteed by CHECK */
if (cmd->cb.fn.chunk.callback) {
int error = cmd->error;
- uint16_t flags = be16toh (h->sbuf.sr.structured_reply.flags);
if (CALL_CALLBACK (cmd->cb.fn.chunk, cmd->data + (offset - cmd->offset),
length - sizeof offset, offset,
LIBNBD_READ_DATA, &error) == -1)
if (cmd->error == 0)
cmd->error = error ? error : EPROTO;
- if (flags & NBD_REPLY_FLAG_DONE)
- FREE_CALLBACK (cmd->cb.fn.chunk);
}
SET_NEXT_STATE (%FINISH);
@@ -454,7 +448,6 @@
memset (cmd->data + offset, 0, length);
if (cmd->cb.fn.chunk.callback) {
int error = cmd->error;
- uint16_t flags = be16toh (h->sbuf.sr.structured_reply.flags);
if (CALL_CALLBACK (cmd->cb.fn.chunk,
cmd->data + offset, length,
@@ -462,8 +455,6 @@
LIBNBD_READ_HOLE, &error) == -1)
if (cmd->error == 0)
cmd->error = error ? error : EPROTO;
- if (flags & NBD_REPLY_FLAG_DONE)
- FREE_CALLBACK (cmd->cb.fn.chunk);
}
SET_NEXT_STATE(%FINISH);
@@ -509,7 +500,6 @@
if (meta_context) {
/* Call the caller's extent function. */
int error = cmd->error;
- uint16_t flags = be16toh (h->sbuf.sr.structured_reply.flags);
if (CALL_CALLBACK (cmd->cb.fn.extent,
meta_context->name, cmd->offset,
@@ -517,8 +507,6 @@
&error) == -1)
if (cmd->error == 0)
cmd->error = error ? error : EPROTO;
- if (flags & NBD_REPLY_FLAG_DONE)
- FREE_CALLBACK (cmd->cb.fn.extent);
}
else
/* Emit a debug message, but ignore it. */
@@ -530,17 +518,11 @@
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 (cmd->type == NBD_CMD_BLOCK_STATUS)
- FREE_CALLBACK (cmd->cb.fn.extent);
- if (cmd->type == NBD_CMD_READ)
- FREE_CALLBACK (cmd->cb.fn.chunk);
+ if (flags & NBD_REPLY_FLAG_DONE)
SET_NEXT_STATE (%^FINISH_COMMAND);
- }
else {
h->reply_cmd = NULL;
SET_NEXT_STATE (%.READY);
diff --git a/generator/states-reply.c b/generator/states-reply.c
index 41dcf8d..575a6d1 100644
--- a/generator/states-reply.c
+++ b/generator/states-reply.c
@@ -174,7 +174,6 @@ save_reply_state (struct nbd_handle *h)
assert (cmd->type != NBD_CMD_DISC);
r = CALL_CALLBACK (cmd->cb.completion, &error);
- FREE_CALLBACK (cmd->cb.completion);
switch (r) {
case -1:
if (error)
diff --git a/generator/states.c b/generator/states.c
index 263f60e..cfa9957 100644
--- a/generator/states.c
+++ b/generator/states.c
@@ -127,7 +127,6 @@ void abort_commands (struct nbd_handle *h,
assert (cmd->type != NBD_CMD_DISC);
r = CALL_CALLBACK (cmd->cb.completion, &error);
- FREE_CALLBACK (cmd->cb.completion);
switch (r) {
case -1:
if (error)
diff --git a/lib/aio.c b/lib/aio.c
index 38a27d0..4a219e4 100644
--- a/lib/aio.c
+++ b/lib/aio.c
@@ -32,11 +32,12 @@ void
nbd_internal_retire_and_free_command (struct command *cmd)
{
/* Free the callbacks. */
- if (cmd->type == NBD_CMD_BLOCK_STATUS)
- FREE_CALLBACK (cmd->cb.fn.extent);
- if (cmd->type == NBD_CMD_READ)
- FREE_CALLBACK (cmd->cb.fn.chunk);
- FREE_CALLBACK (cmd->cb.completion);
+ if (cmd->type == NBD_CMD_BLOCK_STATUS && cmd->cb.fn.extent.free)
+ cmd->cb.fn.extent.free (cmd->cb.fn.extent.user_data);
+ if (cmd->type == NBD_CMD_READ && cmd->cb.fn.chunk.free)
+ cmd->cb.fn.chunk.free (cmd->cb.fn.chunk.user_data);
+ if (cmd->cb.completion.free)
+ cmd->cb.completion.free (cmd->cb.completion.user_data);
free (cmd);
}
diff --git a/lib/debug.c b/lib/debug.c
index eec2051..a0e6636 100644
--- a/lib/debug.c
+++ b/lib/debug.c
@@ -41,7 +41,9 @@ nbd_unlocked_get_debug (struct nbd_handle *h)
int
nbd_unlocked_clear_debug_callback (struct nbd_handle *h)
{
- FREE_CALLBACK (h->debug_callback);
+ if (h->debug_callback.free)
+ h->debug_callback.free (h->debug_callback.user_data);
+ memset (&h->debug_callback, 0, sizeof h->debug_callback);
return 0;
}
--
2.20.1
4 years, 9 months
[PATCH libnbd 0/2] Use free callback to dereference NBD.Buffer.
by Richard W.M. Jones
In this patch series we use the newly introduced free callback
on the completion function to dererence the OCaml NBD.Buffer.
I will make the same kind of change for Python later in a
separate series.
The completion function is always called at the C level, even
if the OCaml program didn't use the optional argument. That's
because the free callback doesn't run otherwise.
There is a case for having the free callback run even if there is no
registered callback:
nbd_aio_pread (nbd, buf, sizeof buf, 0,
(nbd_completion_callback) { .callback = NULL,
.free = completion_free },
0);
but the semantics of that are a bit weird. Why would you need to
"free" a callback which doesn't exist? Would it be correct for the
library to free the callback immediately, rather than after the
theoretical completion callback would have run?
Rich.
4 years, 9 months