search for: nbdkit_get_ext

...tatic uint64_t cache_end; + +static void +cacheextents_unload (void) +{ + nbdkit_extents_free (cache_extents); +} + +static int +cacheextents_add (struct nbdkit_extents *extents) +{ + size_t i = 0; + + for (i = 0; i < nbdkit_extents_count (cache_extents); i++) { + struct nbdkit_extent ex = nbdkit_get_extent (cache_extents, i); + if (nbdkit_add_extent (extents, ex.offset, ex.length, ex.type) < 0) + return -1; + } + + return 0; +} + +static int +cacheextents_fill (struct nbdkit_extents *extents) +{ + size_t i = 0; + size_t count = nbdkit_extents_count (extents); + struct nbdkit_extent...

.... I have no particular comment about the name. A few more things inline below. > +static int > +cacheextents_add (struct nbdkit_extents *extents) > +{ > + size_t i = 0; > + > + for (i = 0; i < nbdkit_extents_count (cache_extents); i++) { > + struct nbdkit_extent ex = nbdkit_get_extent (cache_extents, i); > + if (nbdkit_add_extent (extents, ex.offset, ex.length, ex.type) < 0) This really only returns -1 on failure, and not any other negative values. > + return -1; > + } > + > + return 0; > +} > + > +static int > +cacheextents_fill (str...

...t 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 @@ offset_extents (struct nbdkit_next_ops *next_ops, void *nxdata, for (i = 0; i < nbdkit_extents_count (extents2); ++i) { e = nbdkit_get_extent (extents2, i); e.offset -= offset; - if (nbdkit_add_extent (extents, e.offset, e.length, e.type) == -1) + if (nbdkit_add_extent (extents, e.offset, e.length, e.type) == -1) { + *err = errno; return -1; + } } return 0; } diff --git a/filters/partition/partition.c...

...64_t cache_end; + +static void +cacheextents_unload (void) +{ + nbdkit_extents_free (cache_extents); +} + +static int +cacheextents_add (struct nbdkit_extents *extents, int *err) +{ + size_t i = 0; + + for (i = 0; i < nbdkit_extents_count (cache_extents); i++) { + struct nbdkit_extent ex = nbdkit_get_extent (cache_extents, i); + if (nbdkit_add_extent (extents, ex.offset, ex.length, ex.type) == -1) { + *err = errno; + return -1; + } + } + + return 0; +} + +static int +cacheextents_fill (struct nbdkit_extents *extents, int *err) +{ + size_t i = 0; + size_t count = nbdkit_extents_c...

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

...id) > +{ > + nbdkit_extents_free (cache_extents); > +} > + > +static int > +cacheextents_add (struct nbdkit_extents *extents, int *err) > +{ > + size_t i = 0; > + > + for (i = 0; i < nbdkit_extents_count (cache_extents); i++) { > + struct nbdkit_extent ex = nbdkit_get_extent (cache_extents, i); > + if (nbdkit_add_extent (extents, ex.offset, ex.length, ex.type) == -1) { > + *err = errno; > + return -1; > + } > + } > + > + return 0; > +} > + > +static int > +cacheextents_fill (struct nbdkit_extents *extents, int *err)...

...+151,15 @@ offset_extents (struct nbdkit_next_ops *next_ops, void *nxdata, } if (next_ops->extents (nxdata, count, offs + offset, flags, extents2, err) == -1) - goto error; + return -1; for (i = 0; i < nbdkit_extents_count (extents2); ++i) { e = nbdkit_get_extent (extents2, i); e.offset -= offset; if (nbdkit_add_extent (extents, e.offset, e.length, e.type) == -1) - goto error; + return -1; } - nbdkit_extents_free (extents2); return 0; - - error: - nbdkit_extents_free (extents2); - return -1; } static struct nbdkit_filter f...

...urn -1; > + } Here, we are careful to set *err. > + if (next_ops->extents (nxdata, count, offs + offset, > + flags, extents2, err) == -1) > + goto error; And here. > + > + for (i = 0; i < nbdkit_extents_count (extents2); ++i) { > + e = nbdkit_get_extent (extents2, i); > + e.offset -= offset; > + if (nbdkit_add_extent (extents, e.offset, e.length, e.type) == -1) > + goto error; But here, *err remains unchanged. Is this a problem? Should nbdkit_add_extent() guarantee that errno is sane on failure (right now, it does not)? Af...

...fset, flags, extents2, err) == -1) { > - nbdkit_extents_free (extents2); > + if (next_ops->extents (nxdata, n, offset, flags, extents2, err) == -1) > return -1; > - } > > for (i = 0; i < nbdkit_extents_count (extents2); ++i) { > struct nbdkit_extent e = nbdkit_get_extent (extents2, i); > > - if (nbdkit_add_extent (extents, e.offset, e.length, e.type) == -1) { > - nbdkit_extents_free (extents2); > + if (nbdkit_add_extent (extents, e.offset, e.length, e.type) == -1) > return -1; > - } Of course, we have to re-add the {} if...

...+ n = real_size_copy - offset; > + if (next_ops->extents (nxdata, n, offset, flags, extents2, err) == -1) { > + nbdkit_extents_free (extents2); > + return -1; > + } > + > + for (i = 0; i < nbdkit_extents_count (extents2); ++i) { > + struct nbdkit_extent e = nbdkit_get_extent (extents2, i); > + > + if (nbdkit_add_extent (extents, e.offset, e.length, e.type) == -1) { > + nbdkit_extents_free (extents2); > + return -1; > + } > + } > + nbdkit_extents_free (extents2); > + Should we be using the CLEANUP_EXTENTS_FREE macro here and...

...set, ...) + { + size_t i; + struct nbdkit_extents *extents2; + struct nbdkit_extent e; + + extents2 = nbdkit_extents_new (offset + shift); + next_ops->extents (nxdata, count, offset + shift, flags, extents2, err); + for (i = 0; i < nbdkit_extents_count (extents2); ++i) { + e = nbdkit_get_extent (extents2, i); + e.offset -= shift; + nbdkit_add_extent (extents, e.offset, e.length, e.type); + } + nbdkit_extents_free (extents2); + } + +If there is an error, C<.extents> should call C<nbdkit_error> with an +error message B<and> return -1 with C<err> set to...

...* libnbdkit.so. > + */ > + return nbdkit_main (argc, argv); > +} > diff --git a/server/nbdkit.syms b/server/nbdkit.syms > index a67669b7..a516cc0f 100644 > --- a/server/nbdkit.syms > +++ b/server/nbdkit.syms > @@ -54,6 +54,7 @@ > nbdkit_extents_new; > nbdkit_get_extent; > nbdkit_is_tls; > + nbdkit_main; Do we want to export it as _nbdkit_main, to make it obvious that plugins shouldn't try calling it? That's cosmetic if you think it is worth it. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtua...

...+ + myfilter_extents (...) + { + size_t i; + struct nbdkit_extents *extents2; + struct nbdkit_extent e; + + extents2 = nbdkit_extents_new (offset); + next_ops->extents (nxdata, count, offset, flags, extents2, err); + for (i = 0; i < nbdkit_extents_size (extents2); ++i) { + e = nbdkit_get_extent (extents2, i); + e.offset = /* transform offset */; + nbdkit_add_extent (extents, e.offset, e.length, e.type); + } + nbdkit_extents_free (extents2); + } + +More complicated transformations may require the filter to allocate a +new extents list using C<nbdkit_extents_new> and fr...

...nbdkit_extent e; + int64_t size; + + size = next_ops->get_size (nxdata); + extents2 = nbdkit_extents_new (offset + shift, size - shift); + next_ops->extents (nxdata, count, offset + shift, flags, extents2, err); + for (i = 0; i < nbdkit_extents_count (extents2); ++i) { + e = nbdkit_get_extent (extents2, i); + e.offset -= shift; + nbdkit_add_extent (extents, e.offset, e.length, e.type); + } + nbdkit_extents_free (extents2); + } + +If there is an error, C<.extents> should call C<nbdkit_error> with an +error message B<and> return -1 with C<err> set to...

...>get_size (nxdata); - extents2 = nbdkit_extents_new (offset + shift, size - shift); + extents2 = nbdkit_extents_new (offset + shift, size); next_ops->extents (nxdata, count, offset + shift, flags, extents2, err); for (i = 0; i < nbdkit_extents_count (extents2); ++i) { e = nbdkit_get_extent (extents2, i); diff --git a/filters/offset/offset.c b/filters/offset/offset.c index efe5c6d1..00122770 100644 --- a/filters/offset/offset.c +++ b/filters/offset/offset.c @@ -140,9 +140,9 @@ offset_extents (struct nbdkit_next_ops *next_ops, void *nxdata, size_t i; CLEANUP_EXTENTS_FREE struc...

...6 +115,24 @@ struct nbdkit_extent { uint32_t type; }; -extern struct nbdkit_extents *nbdkit_extents_new (uint64_t start, uint64_t end); -extern void nbdkit_extents_free (struct nbdkit_extents *); -extern size_t nbdkit_extents_count (const struct nbdkit_extents *); -extern struct nbdkit_extent nbdkit_get_extent (const struct nbdkit_extents *, - size_t); -extern int nbdkit_extents_aligned (struct nbdkit_next_ops *next_ops, - nbdkit_backend *nxdata, - uint32_t count, uint64_t offset, -...

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

...+#if defined(NBDKIT_INTERNAL) || !defined(WINDOWS_COMPAT) extern struct nbdkit_extents *nbdkit_extents_new (uint64_t start, uint64_t end); extern void nbdkit_extents_free (struct nbdkit_extents *); extern size_t nbdkit_extents_count (const struct nbdkit_extents *); extern struct nbdkit_extent nbdkit_get_extent (const struct nbdkit_extents *, size_t); +#else +static struct nbdkit_extents *nbdkit_extents_new (uint64_t start, uint64_t end) +{ + return _nbdkit_functions.extents_new(start, end); +} +static void nbdkit_extents_free (struct nbdkit_extents *ext...

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.