search for: nbdkit_extents_new

...0) bytes allocated at offset 0 bytes (0x0) nbdkit: sh[1]: error: extents: plugin must return at least one extent qemu-io: Failed to get allocation status: Invalid argument $ nbdkit -U - --filter=offset sh script offset=640k \ --run 'qemu-io -r -f raw -c map $nbd' nbdkit: sh[1]: error: nbdkit_extents_new: start (655360) >= end (393216) qemu-io: Failed to get allocation status: Invalid argument Fixes: 1a5e2d9c, 624abb36 Signed-off-by: Eric Blake <eblake@redhat.com> --- At least we're catching this now before 1.14. docs/nbdkit-filter.pod | 2 +- filters/offset/offset.c |...

.../truncate/truncate.c b/filters/truncate/truncate.c index 076ae22..0c5dedb 100644 --- a/filters/truncate/truncate.c +++ b/filters/truncate/truncate.c @@ -319,6 +319,10 @@ truncate_extents (struct nbdkit_next_ops *next_ops, void *nxdata, * returned data to the original array. */ extents2 = nbdkit_extents_new (offset, real_size_copy); + if (extents2 == NULL) { + *err = errno; + return -1; + } if (offset + count <= real_size_copy) n = count; else @@ -329,8 +333,10 @@ truncate_extents (struct nbdkit_next_ops *next_ops, void *nxdata, for (i = 0; i < nbdkit_extents_count (extents...

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

On 4/24/19 6:04 AM, Richard W.M. Jones wrote: > 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

...ps *next_ops, void *nxdata, > + void *handle, uint32_t count, uint64_t offs, uint32_t flags, > + struct nbdkit_extents *extents, int *err) > +{ > + size_t i; > + struct nbdkit_extents *extents2; > + struct nbdkit_extent e; > + > + extents2 = nbdkit_extents_new (offs + offset); > + if (extents2 == NULL) { > + *err = errno; > + return -1; > + } Ouch - nbdkit_extents_new() returns NULL for 'start >= INT64_MAX' without setting errno. Of course, that failure path should be unreachable here, but if you are going to assign *err b...

...(cmd == NBD_CMD_READ || cmd == NBD_CMD_WRITE) { > - buf = malloc (count); > + buf = threadlocal_buffer ((size_t) count); > if (buf == NULL) { > - out_of_memory: > - perror ("malloc"); > error = ENOMEM; Old code called perror() when nbdkit_extents_new() failed... > if (cmd == NBD_CMD_WRITE && > skip_over_write_buffer (conn->sockin, count) < 0) > @@ -673,8 +673,10 @@ protocol_recv_request_send_reply (struct connection *conn) > /* Allocate the extents list for block status only. */ > if...

...oes. > --- > +++ b/server/extents.c > @@ -0,0 +1,210 @@ > +/* nbdkit > + * Copyright (C) 2019 Red Hat Inc. > + * All rights reserved. We started work on this phrase but it got waylaid by extents work; maybe now is a good time to revisit that? > +struct nbdkit_extents * > +nbdkit_extents_new (uint64_t start, uint64_t end) > +{ > + struct nbdkit_extents *r; > + > + if (start >= INT64_MAX || end >= INT64_MAX) { Is this an off-by-one? INT64_MAX is a valid offset in our memory plugin, while INT64_MAX + 1 is not. > + nbdkit_error ("nbdkit_extents_new: "...

..., Mar 23, 2019 at 11:25:06AM -0500, Eric Blake wrote: > On 3/20/19 5:11 PM, Richard W.M. Jones wrote: > > + 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_exte...

...; + * called later about the hole. However we do need to make sure > + * that the extents array is truncated to the real size, hence we > + * have to create a new extents array, ask the plugin, then copy the > + * returned data to the original array. > + */ > + extents2 = nbdkit_extents_new (0, real_size_copy); Why 0 for start instead of offset? You get the same result either way (since the copying code ignores the prefix), but it's probably a lot more efficient to not have to copy the extents for the prefix of the file. -- Eric Blake, Principal Software Engineer Red Hat, Inc....

...extents, err); + } + +It is also possible for filters to transform the extents list received +back from the layer below. Without error checking it would look like +this: + + 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); + } +...

...lters to transform the extents list received > +back from the layer below. Without error checking it would look like > +this: > + > + 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....

...k +from the layer below. Without error checking it would look like this: + + myfilter_extents (..., uint32_t count, uint64_t offset, ...) + { + size_t i; + struct nbdkit_extents *extents2; + struct 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....

...sible for filters to transform the extents list received +back from the layer below. Without error checking it would look like +this: + + myfilter_extents (..., uint32_t count, uint64_t offset, ...) + { + 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); + } +...

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 = errno; > + return -1; > + } Here, we are careful to set *err. > + if (next_ops->extents (nxdata, count, offs + offset, > + flags, extents2, err) == -1) > + goto...

...+/* Appendable list of extents. */ +DEFINE_VECTOR_TYPE(extents, struct nbdkit_extent); + struct nbdkit_extents { - struct nbdkit_extent *extents; - size_t nr_extents, allocated; + extents extents; uint64_t start, end; /* end is one byte beyond the end of the range */ @@ -92,8 +95,7 @@ nbdkit_extents_new (uint64_t start, uint64_t end) nbdkit_error ("nbdkit_extents_new: malloc: %m"); return NULL; } - r->extents = NULL; - r->nr_extents = r->allocated = 0; + r->extents = (extents) empty_vector; r->start = start; r->end = end; r->next = -1; @@ -10...

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

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 (nxdata, n, offset, flags, extents2, err) == -1) { > + nbdkit_extents_free (extents2); > + return -1; > + }...

...} + else { + /* The plugin needs a fresh extents object each time, but + * with care, we can merge it into the callers' extents. + */ + extents tmp; + CLEANUP_EXTENTS_FREE struct nbdkit_extents *extents2 = NULL; + + extents2 = nbdkit_extents_new (e.offset + e.length, offset + align); + if (next_ops->extents (nxdata, offset + align - e.length, + e.offset + e.length, + flags & ~NBDKIT_FLAG_REQ_ONE, + extents2, err) == -1) +...

...he real main function in > + * 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....

...t want to change the return type of > .default_export to 'char *', because that would make our choice of > malloc()/free() part of the API, preventing either nbdkit or a plugin > from experimenting with an alternate allocator implementation. > Elsewhere, we have done things like nbdkit_extents_new(), so that even > though the client is directing allocation, the actual call to malloc() > is done by nbdkit proper, so that nbdkit later calling free() does not > tie the plugin's hands, and nbdkit could change its underlying > allocation without breaking ABI. (Note that nbdkit_re...