similar to: [nbdkit PATCH] offset, partition: Fix .extents with non-zero offset

On 3/28/19 11:18 AM, Richard W.M. Jones wrote: > Allows you to safely use nbdkit-offset-filter on top of a plugin > supporting extents. > --- > filters/offset/offset.c | 36 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 36 insertions(+) > > + > + extents2 = nbdkit_extents_new (offs + offset, real_size - offset); > + if (extents2 == NULL) { > + *err =

While working on adding assertions to pthread_mutex_lock calls, I noticed that the truncate filter's use of mutex didn't really protect us, and isn't really necessary. Cleaning that up also spotted a couple of other potential cleanups. Eric Blake (4): filters: Drop useless .open callbacks truncate: Fix corruption when plugin changes per-connection size truncate: Test for safe

On 3/28/19 11:18 AM, Richard W.M. Jones wrote: > --- > filters/truncate/truncate.c | 55 +++++++++++++++++++++++++++++++++++++ > 1 file changed, 55 insertions(+) > > + extents2 = nbdkit_extents_new (0, real_size_copy); > + if (offset + count <= real_size_copy) > + n = count; > + else > + n = real_size_copy - offset; > + if (next_ops->extents

This has already been pushed upstream. I am simply posting these here so we have a reference in the mailing list in case we find bugs later (as I'm sure we will - it's a complex patch series). Great thanks to Eric Blake for tireless review on this one. It also seems to have identified a few minor bugs in qemu along the way. Rich.

On 3/20/19 5:11 PM, Richard W.M. Jones wrote: > Allows you to safely use nbdkit-offset-filter on top of a plugin > supporting extents. > --- > filters/offset/offset.c | 35 +++++++++++++++++++++++++++++++++++ > 1 file changed, 35 insertions(+) If we don't change our minds on the interface in patch 1, then this looks correct. > +/* Extents. */ > +static int >

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

There's more that can be done (in particular, use of CLEANUP_FREE), but this is enough to at least see if I'm on the right track. I couldn't figure out an obvious difference between common/include and common/utils, but it looks like the former is for things that are inlineable via .h only, while the latter is when you need to link in a convenience library, so this landed in the

On 4/23/19 2:06 PM, Eric Blake wrote: > Now that cleanup.h is in common code, we can use it in our > filters. The first round focuses just on places that called > nbdkit_extents_free(), as all three callers had multiple exit paths > that definitely benefit from the macro. > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > filters/offset/offset.c | 13

Ultimately, both the blocksize and swab filters want to return aligned extents to the client. I'm posting this as a snapshot of my work in progress on how I plan to get there (it's not quite working yet, but I'm done for today and wanted to at least document my ideas). I might also add a convenience function for nbdkit_extents_offset, since we have a number of filters that repeat the

Not sure what version we're up to, but this reimplements extents using the new simpler structure described in this thread: https://www.redhat.com/archives/libguestfs/2019-March/msg00077.html I also fixed most of the things that Eric pointed out in the previous review, although I need to go back over his replies and check I've got everything. This needs a bit more testing. However the

I'm not sure exactly which version we're up to, but let's say it's version 4. I'm a lot happier with this version: - all filters have been reviewed and changed where I think that's necessary - can_extents is properly defined and implemented now - NBD protocol is followed - I believe it addresses all previous review points where possible The "only" thing

This fix isn't exhaustive but it fixes some obvious problems in the filters. Rich.

I do have a question about whether patch 2 is right, or whether I've exposed a bigger problem in the truncate (and possibly other) filter, but the rest seem fairly straightforward. Eric Blake (4): server: Check for pthread lock failures truncate: Factor out reading real_size under mutex plugins: Check for mutex failures filters: Check for mutex failures filters/cache/cache.c

On 3/20/19 5:11 PM, Richard W.M. Jones wrote: > This pair of calls allows plugins to describe which extents in the > virtual disk are allocated, holes or zeroes. > --- I see you've already made a couple of tweaks to your block-status branch since posting this (skipping 0-legnth add_extent, and coalescing consecutive add_extents with the same type), but here's some more review:

--- filters/partition/partition.c | 37 +++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/filters/partition/partition.c b/filters/partition/partition.c index c1fe7a1..bc60480 100644 --- a/filters/partition/partition.c +++ b/filters/partition/partition.c @@ -38,6 +38,7 @@ #include <stdint.h> #include <string.h> #include <inttypes.h> +#include

Allows you to safely use nbdkit-offset-filter on top of a plugin supporting extents. --- filters/offset/offset.c | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/filters/offset/offset.c b/filters/offset/offset.c index 058571d..f949f6d 100644 --- a/filters/offset/offset.c +++ b/filters/offset/offset.c @@ -37,6 +37,7 @@ #include <stdlib.h> #include

Now that cleanup.h is in common code, we can use it in our filters. The first round focuses just on places that called nbdkit_extents_free(), as all three callers had multiple exit paths that definitely benefit from the macro. Signed-off-by: Eric Blake <eblake@redhat.com> --- filters/offset/offset.c | 13 +++++-------- filters/partition/partition.c | 12 ++++--------

On Thu, Mar 28, 2019 at 04:18:38PM +0000, Richard W.M. Jones wrote: >--- > filters/partition/partition.c | 37 +++++++++++++++++++++++++++++++++++ > 1 file changed, 37 insertions(+) > >diff --git a/filters/partition/partition.c b/filters/partition/partition.c >index c1fe7a1..bc60480 100644 >--- a/filters/partition/partition.c >+++ b/filters/partition/partition.c >@@ -38,6

On Tue, Jul 07, 2020 at 05:22:46PM -0500, Eric Blake wrote: [...] > +/* Compute aligned extents on behalf of a filter. */ > +int > +nbdkit_extents_aligned (struct nbdkit_next_ops *next_ops, > + nbdkit_backend *nxdata, > + uint32_t count, uint64_t offset, > + uint32_t flags, uint32_t align, > +

--- filters/truncate/truncate.c | 55 +++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/filters/truncate/truncate.c b/filters/truncate/truncate.c index b95432a..e8e56f7 100644 --- a/filters/truncate/truncate.c +++ b/filters/truncate/truncate.c @@ -285,6 +285,60 @@ truncate_zero (struct nbdkit_next_ops *next_ops, void *nxdata, return 0; } +/* Extents. */