August 2019 - Libguestfs - libguestfs List Archives

[nbdkit PATCH] nbd: Another libnbd version bump
by Eric Blake 15 Aug '19

15 Aug '19

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

2 3

ANNOUNCE: libnbd 0.9.8 - prerelease of high performance NBD client library
by Richard W.M. Jones 15 Aug '19

15 Aug '19

I'm pleased to announce a new high performance Network Block Device (NBD) client library called libnbd. It's written in C and there are also bindings available for Python, OCaml and (soon) Rust. 0.9.8 is the third pre-release before the stable 1.0 version where we freeze the API, so feedback on API-related issues is very welcome now. Download: http://download.libguestfs.org/libnbd/ Documentation: https://github.com/libguestfs/libnbd/blob/master/docs/libnbd.pod Fedora package: https://koji.fedoraproject.org/koji/packageinfo?packageID=28807 Debian package: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=933223 Git repo: https://github.com/libguestfs/libnbd Mailing list: libguestfs(a)redhat.com (no subscription required) Here are some of the things you can do with this library ... Connect to an NBD server and grab the first sector of the disk: https://github.com/libguestfs/libnbd/blob/a5f8fd2f0f48e9cf2487e23750b55f67b… High performance multi-threaded reads and writes, with multiple connections and multiple commands in flight on each connection: https://github.com/libguestfs/libnbd/blob/master/examples/threaded-reads-an… Integrate with glib main loop: https://github.com/libguestfs/libnbd/blob/master/examples/glib-main-loop.c Connect to an NBD server from an interactive shell: $ nbdkit -f linuxdisk . & $ nbdsh --connect nbd://localhost Welcome to nbdsh, the shell for interacting with Network Block Device (NBD) servers. nbd> h.get_size() 716266496 nbd> buf = h.pread (512, 0) nbd> print ("%r" % buf) [prints the first sector] Use ‘fio’ to benchmark an NBD server: $ nbdkit -U - memory size=256M \ --run 'export unixsocket ; fio examples/nbd.fio ' Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org

1 0

[libnbd PATCH] docs: Ensure .3 files get built
by Eric Blake 15 Aug '19

15 Aug '19

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

1 0

[PATCH libnbd] docs: Change docs/Makefile.inc back to a regular include, readd to git.
by Richard W.M. Jones 15 Aug '19

15 Aug '19

‘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

2 1

[PATCH libnbd] api: Rename nbd_kill_command -> nbd_kill_subprocess.
by Richard W.M. Jones 15 Aug '19

15 Aug '19

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

2 1

[PATCH libnbd v2 00/10] Callbacks and OCaml and Python persistent buffers.
by Richard W.M. Jones 15 Aug '19

15 Aug '19

This is a combination of these two earlier series: https://www.redhat.com/archives/libguestfs/2019-August/msg00235.html https://www.redhat.com/archives/libguestfs/2019-August/msg00240.html plus changes to allow .callback = NULL / .free != NULL, and to reduce the complexity of freeing callbacks. Although it's rather long there's nothing complex here. We might consider squashing some patches together. I believe I've addressed everything raised in the earlier reviews, with the exception that I'm not using Py_XDECREF because it seems a bit obscure (I'd never heard of it myself and it hides the true meaning of the statement). Rich.

2 17

[nbdkit PATCH] data, memory: Optimize .zero > PAGE_SIZE
by Eric Blake 15 Aug '19

15 Aug '19

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

2 2

[libnbd PATCH] lib: Consolidate free callbacks to just happen at retire time
by Eric Blake 15 Aug '19

15 Aug '19

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

2 2

[PATCH libnbd 0/3] Use free callback to hold ref to AIO buffer.
by Richard W.M. Jones 14 Aug '19

14 Aug '19

Basically the same as this patch series, but for Python: https://www.redhat.com/archives/libguestfs/2019-August/msg00235.html plus adding the 590 asynch test at the end. Rich.

2 5

[PATCH libnbd 0/2] Use free callback to dereference NBD.Buffer.
by Richard W.M. Jones 14 Aug '19

14 Aug '19

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.

2 4