search for: real_size_copi

Add a helper function get_real_size() to make it easier to add sanity checking that mutex calls don't fail. Signed-off-by: Eric Blake <eblake@redhat.com> --- I'm a bit unsure why truncate.c needs a lock anyway. I guess it's because we are storing 'size' and 'real_size' as globals rather than per-connection values in the handle, and we're worrying about

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

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

This can truncate, extend, or round up/down to a multiple. --- common-rules.mk | 3 +- configure.ac | 1 + filters/offset/nbdkit-offset-filter.pod | 7 +- filters/partition/nbdkit-partition-filter.pod | 1 + filters/truncate/Makefile.am | 61 ++++ filters/truncate/nbdkit-truncate-filter.pod | 88

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

--- 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. */

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

On 3/28/19 11:18 AM, Richard W.M. Jones wrote: > --- > filters/truncate/truncate.c | 55 +++++++++++++++++++++++++++++++++++++ > 1 file changed, 55 insertions(+) > > + > + /* If the entire request is beyond the end of the underlying plugin > + * then this is the easy case: return a hole. > + */ > + if (offset >= real_size_copy) > + return

On Thu, Mar 28, 2019 at 09:39:23PM -0500, Eric Blake wrote: > 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) > > +

I have dropped the map filter from this series for now while I try to get it working. However I think the truncate filter is in a good shape. This incorporates all feedback from Eric's review. Also there are three small fixes to the filter code, all revealed when I was testing using multiple filters which we'd not done much of before. Rich.

On Thu, Mar 28, 2019 at 08:57:56PM -0500, Eric Blake wrote: > On 3/28/19 11:18 AM, Richard W.M. Jones wrote: > > --- > > filters/truncate/truncate.c | 55 +++++++++++++++++++++++++++++++++++++ > > 1 file changed, 55 insertions(+) > > > > > + > > + /* If the entire request is beyond the end of the underlying plugin > > + * then this is the easy

Thanks: Eric Blake for reporting the bug. --- filters/offset/offset.c | 4 +++- filters/partition/partition.c | 4 +++- filters/truncate/truncate.c | 8 +++++++- 3 files changed, 13 insertions(+), 3 deletions(-) diff --git a/filters/offset/offset.c b/filters/offset/offset.c index 24ccb4c..633a1c7 100644 --- a/filters/offset/offset.c +++ b/filters/offset/offset.c @@ -156,8 +156,10 @@

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

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 ++++--------

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

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 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.

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