The cache filter .open has never done any useful work; since its introduction in c10d126f, it appears to exist purely as copy-and-paste from the cow filter (differing only in whether the readonly parameter is massaged before passing on to next). The error filter .open used to track a per-connection handle, but for testing purposes, it was changed to use only global state in commit b33ccbb8. Drop these two .open callbacks in favor of using nbdkit's default behavior, with no semantic change. All other filters with an .open callback either massage parameters passed to next, create a per-connection handle, or both.

Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- filters/cache/cache.c | 10 ---------- filters/error/error.c | 10 ---------- 2 files changed, 20 deletions(-) diff --git a/filters/cache/cache.c b/filters/cache/cache.c index b3fef42..19ce555 100644 --- a/filters/cache/cache.c +++ b/filters/cache/cache.c @@ -186,15 +186,6 @@ cache_config_complete (nbdkit_next_config_complete *next, void *nxdata) return next (nxdata); } -static void * -cache_open (nbdkit_next_open *next, void *nxdata, int readonly) -{ - if (next (nxdata, readonly) == -1) - return NULL; - - return NBDKIT_HANDLE_NOT_NEEDED; -} - /* Get the file size and ensure the cache is the correct size. */ static int64_t cache_get_size (struct nbdkit_next_ops *next_ops, void *nxdata, @@ -476,7 +467,6 @@ static struct nbdkit_filter filter = { .unload = cache_unload, .config = cache_config, .config_complete = cache_config_complete, - .open = cache_open, .prepare = cache_prepare, .get_size = cache_get_size, .pread = cache_pread, diff --git a/filters/error/error.c b/filters/error/error.c index add7566..8932292 100644 --- a/filters/error/error.c +++ b/filters/error/error.c @@ -252,15 +252,6 @@ error_config (nbdkit_next_config *next, void *nxdata, "error-pread*, error-pwrite*, error-trim*, error-zero*, error-extents*\n" \ " Apply settings only to read/write/etc" -static void * -error_open (nbdkit_next_open *next, void *nxdata, int readonly) -{ - if (next (nxdata, readonly) == -1) - return NULL; - - return NBDKIT_HANDLE_NOT_NEEDED; -} - /* This function injects a random error. */ static bool random_error (const struct error_settings *error_settings, @@ -366,7 +357,6 @@ static struct nbdkit_filter filter = { .unload = error_unload, .config = error_config, .config_help = error_config_help, - .open = error_open, .pread = error_pread, .pwrite = error_pwrite, .trim = error_trim, -- 2.20.1 _______________________________________________ Libguestfs mailing list Libguestfs(a)redhat.com https://www.redhat.com/mailman/listinfo/libguestfs

The truncate filter tried to be careful to lock access to setting or reading the real_size variable learned from calling next_ops->get_size, in anticipation of not behaving incorrectly if the NBD protocol makes dynamic resize supported, and where the global variable could serve as a cache rather than having to always call next_ops->get_size to recompute things. However, nbdkit-plugin.pod already documents that .get_size and other option negotiation callbacks may return different values on a per-connection basis, but that within a connection those results should be constant because they may be cached. And our lock does not prevent the following race: thread A thread B .prepare - lock - next_ops->get_size returns A - set real_size based on A - unlock .pread .prepare - lock - next_ops->get_size returns B - set real_size based on B - unlock - lock - set real_size_copy based on B - unlock - decide whether to call next_ops->pread using wrong real_size If we are worried that next_ops->get_size() can change (even if our current documentation says it should not), then we must call it every time, rather than relying on an older cached version of the size; conversely, if we are trying to cache things, then we are better off caching it in a per-connection handle instead of globally between connections. Until the NBD protocol provides a way to advertise new sizes to clients, then our per-connection handle is effectively readonly after .prepare, so we don't even have to worry about locks (and if we DO want correct locking, it would be better to have a rwlock with get_size holding write and all other ops holding a read for the duration of the call, rather than just for the moment it copies from a global variable). The next commit abuses the file plugin to demonstrate the above race where a global size cache changed by connection B can affect the behavior seen by connection A. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- filters/truncate/truncate.c | 162 +++++++++++++++++++----------------- 1 file changed, 87 insertions(+), 75 deletions(-) diff --git a/filters/truncate/truncate.c b/filters/truncate/truncate.c index 5f3370d..dfa9105 100644 --- a/filters/truncate/truncate.c +++ b/filters/truncate/truncate.c @@ -1,5 +1,5 @@ /* nbdkit - * Copyright (C) 2018 Red Hat Inc. + * Copyright (C) 2018-2019 Red Hat Inc. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions are @@ -54,15 +54,6 @@ static int64_t truncate_size = -1; static unsigned round_up = 0, round_down = 0; -/* The real size of the underlying plugin. */ -static uint64_t real_size; - -/* The calculated size after applying the parameters. */ -static uint64_t size; - -/* This lock protects the real_size and size fields. */ -static pthread_mutex_t lock = PTHREAD_MUTEX_INITIALIZER; - static int parse_round_param (const char *key, const char *value, unsigned *ret) { @@ -121,51 +112,90 @@ truncate_config (nbdkit_next_config *next, void *nxdata, "round-up=<N> Round up to next multiple of N.\n" \ "round-down=<N> Round down to multiple of N." -static int64_t truncate_get_size (struct nbdkit_next_ops *next_ops, void *nxdata, void *handle); +/* Per-connection state. Until the NBD protocol gains dynamic resize + * support, each connection remembers the size of the underlying + * plugin at open (even if that size differs between connections + * because the plugin tracks external resize effects). + */ +struct handle { + /* The real size of the underlying plugin. */ + uint64_t real_size; -/* In prepare, force a call to get_size which sets the real_size & size - * globals. + /* The calculated size after applying the parameters. */ + uint64_t size; +}; + +/* Open a connection. */ +static void * +truncate_open (nbdkit_next_open *next, void *nxdata, int readonly) +{ + struct handle *h; + + if (next (nxdata, readonly) == -1) + return NULL; + + h = malloc (sizeof *h); /* h is populated during .prepare */ + if (h == NULL) { + nbdkit_error ("malloc: %m"); + return NULL; + } + + return h; +} + +static void +truncate_close (void *handle) +{ + struct handle *h = handle; + + free (h); +} + +/* In prepare, force a call to next_ops->get_size in order to set + * per-connection real_size & size; these values are not changed + * during the life of the connection. */ static int truncate_prepare (struct nbdkit_next_ops *next_ops, void *nxdata, void *handle) { int64_t r; - - r = truncate_get_size (next_ops, nxdata, handle); - return r >= 0 ? 0 : -1; -} - -/* Get the size. As a side effect, calculate the size to serve. */ -static int64_t -truncate_get_size (struct nbdkit_next_ops *next_ops, void *nxdata, - void *handle) -{ - int64_t r, ret; + struct handle *h = handle; r = next_ops->get_size (nxdata); if (r == -1) return -1; - pthread_mutex_lock (&lock); - - real_size = size = r; + h->real_size = h->size = r; /* The truncate, round-up and round-down parameters are treated as * separate operations. It's possible to specify more than one, * although perhaps not very useful. */ if (truncate_size >= 0) - size = truncate_size; + h->size = truncate_size; if (round_up > 0) - size = ROUND_UP (size, round_up); + h->size = ROUND_UP (h->size, round_up); if (round_down > 0) - size = ROUND_DOWN (size, round_down); - ret = size; + h->size = ROUND_DOWN (h->size, round_down); - pthread_mutex_unlock (&lock); + return r >= 0 ? 0 : -1; +} - return ret; +/* Get the size. As a side effect, calculate the size to serve. */ +static int64_t +truncate_get_size (struct nbdkit_next_ops *next_ops, void *nxdata, + void *handle) +{ + struct handle *h = handle; + + /* If the NBD protocol and nbdkit adds dynamic resize, we'll need a + * rwlock where get_size holds write lock and all other ops hold + * read lock. Until then, NBD sizes are unchanging (even if the + * underlying plugin can react to external size changes), so just + * returned what we cached at connection open. + */ + return h->size; } /* Read data. */ @@ -176,17 +206,13 @@ truncate_pread (struct nbdkit_next_ops *next_ops, void *nxdata, { int r; uint32_t n; - uint64_t real_size_copy; + struct handle *h = handle; - pthread_mutex_lock (&lock); - real_size_copy = real_size; - pthread_mutex_unlock (&lock); - - if (offset < real_size_copy) { - if (offset + count <= real_size_copy) + if (offset < h->real_size) { + if (offset + count <= h->real_size) n = count; else - n = real_size_copy - offset; + n = h->real_size - offset; r = next_ops->pread (nxdata, buf, n, offset, flags, err); if (r == -1) return -1; @@ -209,17 +235,13 @@ truncate_pwrite (struct nbdkit_next_ops *next_ops, void *nxdata, { int r; uint32_t n; - uint64_t real_size_copy; + struct handle *h = handle; - pthread_mutex_lock (&lock); - real_size_copy = real_size; - pthread_mutex_unlock (&lock); - - if (offset < real_size_copy) { - if (offset + count <= real_size_copy) + if (offset < h->real_size) { + if (offset + count <= h->real_size) n = count; else - n = real_size_copy - offset; + n = h->real_size - offset; r = next_ops->pwrite (nxdata, buf, n, offset, flags, err); if (r == -1) return -1; @@ -246,17 +268,13 @@ truncate_trim (struct nbdkit_next_ops *next_ops, void *nxdata, uint32_t flags, int *err) { uint32_t n; - uint64_t real_size_copy; + struct handle *h = handle; - pthread_mutex_lock (&lock); - real_size_copy = real_size; - pthread_mutex_unlock (&lock); - - if (offset < real_size_copy) { - if (offset + count <= real_size_copy) + if (offset < h->real_size) { + if (offset + count <= h->real_size) n = count; else - n = real_size_copy - offset; + n = h->real_size - offset; return next_ops->trim (nxdata, n, offset, flags, err); } return 0; @@ -269,17 +287,13 @@ truncate_zero (struct nbdkit_next_ops *next_ops, void *nxdata, uint32_t flags, int *err) { uint32_t n; - uint64_t real_size_copy; + struct handle *h = handle; - pthread_mutex_lock (&lock); - real_size_copy = real_size; - pthread_mutex_unlock (&lock); - - if (offset < real_size_copy) { - if (offset + count <= real_size_copy) + if (offset < h->real_size) { + if (offset + count <= h->real_size) n = count; else - n = real_size_copy - offset; + n = h->real_size - offset; return next_ops->zero (nxdata, n, offset, flags, err); } return 0; @@ -292,21 +306,17 @@ truncate_extents (struct nbdkit_next_ops *next_ops, void *nxdata, uint32_t flags, struct nbdkit_extents *extents, int *err) { uint32_t n; - uint64_t real_size_copy; + struct handle *h = handle; CLEANUP_EXTENTS_FREE struct nbdkit_extents *extents2 = NULL; size_t i; - pthread_mutex_lock (&lock); - real_size_copy = real_size; - pthread_mutex_unlock (&lock); - /* If the entire request is beyond the end of the underlying plugin * then this is the easy case: return a hole up to the end of the * file. */ - if (offset >= real_size_copy) { + if (offset >= h->real_size) { int r = nbdkit_add_extent (extents, - real_size_copy, truncate_size - real_size_copy, + h->real_size, truncate_size - h->real_size, NBDKIT_EXTENT_ZERO|NBDKIT_EXTENT_HOLE); if (r == -1) *err = errno; @@ -322,15 +332,15 @@ truncate_extents (struct nbdkit_next_ops *next_ops, void *nxdata, * have to create a new extents array, ask the plugin, then copy the * returned data to the original array. */ - extents2 = nbdkit_extents_new (offset, real_size_copy); + extents2 = nbdkit_extents_new (offset, h->real_size); if (extents2 == NULL) { *err = errno; return -1; } - if (offset + count <= real_size_copy) + if (offset + count <= h->real_size) n = count; else - n = real_size_copy - offset; + n = h->real_size - offset; if (next_ops->extents (nxdata, n, offset, flags, extents2, err) == -1) return -1; @@ -352,6 +362,8 @@ static struct nbdkit_filter filter = { .version = PACKAGE_VERSION, .config = truncate_config, .config_help = truncate_config_help, + .open = truncate_open, + .close = truncate_close, .prepare = truncate_prepare, .get_size = truncate_get_size, .pread = truncate_pread,

Test the just-fixed bug in the truncate filter. Although it is in general unsafe to shrink an image being served by the file plugin, the test is careful to only issue operations on connection A after the file has been resized back to its original length, in order to prove whether the smaller size observed during the short-lived connection B bleeds over into the pread results seen by connection A. If this test is applied without the previous patch, the second read from A sees all zeroes instead of the desired pattern 2. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- tests/Makefile.am | 2 + tests/test-truncate4.sh | 85 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 87 insertions(+) create mode 100755 tests/test-truncate4.sh diff --git a/tests/Makefile.am b/tests/Makefile.am index 4b7aa22..4148793 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -122,6 +122,7 @@ EXTRA_DIST = \ test-truncate1.sh \ test-truncate2.sh \ test-truncate3.sh \ + test-truncate4.sh \ test-truncate-extents.sh \ test-vddk.sh \ test-vddk-real.sh \ @@ -937,6 +938,7 @@ TESTS += \ test-truncate1.sh \ test-truncate2.sh \ test-truncate3.sh \ + test-truncate4.sh \ test-truncate-extents.sh # xz filter test. diff --git a/tests/test-truncate4.sh b/tests/test-truncate4.sh new file mode 100755 index 0000000..e4be626 --- /dev/null +++ b/tests/test-truncate4.sh @@ -0,0 +1,85 @@ +#!/usr/bin/env bash +# nbdkit +# Copyright (C) 2019 Red Hat Inc. +# +# Redistribution and use in source and binary forms, with or without +# modification, are permitted provided that the following conditions are +# met: +# +# * Redistributions of source code must retain the above copyright +# notice, this list of conditions and the following disclaimer. +# +# * Redistributions in binary form must reproduce the above copyright +# notice, this list of conditions and the following disclaimer in the +# documentation and/or other materials provided with the distribution. +# +# * Neither the name of Red Hat nor the names of its contributors may be +# used to endorse or promote products derived from this software without +# specific prior written permission. +# +# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS IS'' AND +# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, +# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A +# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR +# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF +# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND +# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, +# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT +# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF +# SUCH DAMAGE. + +# Regression test when next_ops->get_size changes between connections. +# For now, NBD does not support dynamic resize; but the file plugin +# reads size from the file system for each new connection, at which +# point the client remembers that size for the life of the connection. +# We are testing that connection A can still see the tail of a file, +# even when connection B is opened while the file was temporarily +# shorter (if the actions of connection B affect the size visible +# through connection A, we didn't isolate per-connection state). + +source ./functions.sh +set -e +set -x + +requires qemu-io --version + +sock=`mktemp -u` +files="truncate4.out truncate4.pid $sock truncate4.data" +rm -f $files +cleanup_fn rm -f $files + +# Initial file contents: 1k of pattern 1 +truncate -s 1024 truncate4.data +qemu-io -c 'w -P 1 0 1024' -f raw truncate4.data + +# Run nbdkit with file plugin and truncate filter in front. +start_nbdkit -P truncate4.pid -U $sock \ + --filter=truncate \ + file truncate4.data \ + round-up=1024 + +fail=0 +exec 4>&1 # Save original stdout +{ + exec 5>&1 >&4 # Save connection A, set stdout back to original + echo 'Reading from connection A, try 1' + echo 'r -P 1 0 1024' >&5 + sleep 1 + echo 'Resizing down' + truncate -s 512 truncate4.data + echo 'Reading from connection B' + echo 'r -P 1 0 512' | qemu-io -f raw nbd:unix:$sock >> truncate4.out + echo 'Restoring size' + truncate -s 1024 truncate4.data + qemu-io -c 'w -P 2 0 1024' -f raw truncate4.data + echo 'Reading from connection A, try 2' + echo 'r -P 2 512 512' >&5 + echo 'quit' >&5 +} | qemu-io -f raw nbd:unix:$sock >> truncate4.out || fail=1 +exec 4>&- + +cat truncate4.out +grep 'Pattern verification failed' truncate4.out && fail=1 +exit $fail