search for: nbdkit_extent_add

...before > returning, and where nbdkit collects the extents itself based on the > sequence of calls rather than making the plugin itself manage an array. > > nbdkit plugin > receive client BLOCK_STATUS call > initialize array > call plugin.extents > call nbdkit_extent_add() > add extent to array > call nbdkit_extent_add() > add extent to array > ... > return > process the array Yes I agree with both these points. In the second case a concrete implementation might look like this (without error checking):...

I think the extents map is just too complicated and is unnecessarily so. How about instead we define the plugin interface to be: int can_extents (void *handle); // as before int extents (void *handle, uint32_t count, uint64_t offset, uint32_t flags, struct nbdkit_extents_list *list); and have the extents_list be a simple list. The first extent you add must start at offset.

...on-zero if .can_extents returns false? > + > +The callback should detect and return the list of extents overlapping > +the range C<[offset...offset+count-1]>. The C<extents_map> parameter > +points to an opaque object which the callback should fill in by > +calling C<nbdkit_extent_add> etc. See L</Extents map> below. > + > +The C<flags> parameter may contain the flag C<NBDKIT_FLAG_REQ_ONE> > +which means that the client is only requesting information about the > +extent overlapping C<offset>. The plugin may ignore this flag, or as > +a...

Second version based on nbdkit_extent* calls, as discussed here: https://www.redhat.com/archives/libguestfs/2019-March/msg00033.html Note in particular there is some subtlety about how filters would work in this implementation. See docs/nbdkit-filter.pod for the details. Rich.

...but are not required to return more than one extent > (regardless of flags). Well, required to return at least one for the command to succeed (if the return 0, then we synthesize an error, perhaps EIO, to keep the connection to the client still alive). I take it you'd still have to use an nbdkit_extent_add(list, count, type), so that additions to the list are still calling malloc() from nbdkit's perspective (as you don't want the plugin to be realloc()ing the list)? > > This would be simple enough to implement for both VDDK and file. (For > VDDK the plugin needs to synthesize the...

...'ll clarify this too. > > +The callback should detect and return the list of extents overlapping > > +the range C<[offset...offset+count-1]>. The C<extents_map> parameter > > +points to an opaque object which the callback should fill in by > > +calling C<nbdkit_extent_add> etc. See L</Extents map> below. > > + > > +The C<flags> parameter may contain the flag C<NBDKIT_FLAG_REQ_ONE> > > +which means that the client is only requesting information about the > > +extent overlapping C<offset>. The plugin may ignore this...

...tion will not be called if C<.can_extents> returned false. + +The callback should detect and return the list of extents overlapping +the range C<[offset...offset+count-1]>. The C<extents_map> parameter +points to an opaque object which the callback should fill in by +calling C<nbdkit_extent_add> etc. See L</Extents map> below. + +The C<flags> parameter may contain the flag C<NBDKIT_FLAG_REQ_ONE> +which means that the client is only requesting information about the +extent overlapping C<offset>. The plugin may ignore this flag, or as +an optimization it may ret...

...case is to treat the whole virtual +disk as if it was allocated. + +The callback should detect and return the list of extents overlapping +the range C<[offset...offset+count-1]>. The C<extents_map> parameter +points to an opaque object which the callback should fill in by +calling C<nbdkit_extent_add> etc. See L</Extents map> below. + +The C<flags> parameter may contain the flag C<NBDKIT_FLAG_REQ_ONE> +which means that the client is only requesting information about the +extent overlapping C<offset>. The plugin may ignore this flag, or as +an optimization it may ret...

I'm not sure which version we're up to now. Anyway I believe this addresses all the points that Eric raised in: https://www.redhat.com/archives/libguestfs/2019-March/msg00038.html https://www.redhat.com/archives/libguestfs/2019-March/msg00040.html In particular: - default state of extents_map is all allocated disk - support hole + non-zero - you can now iterate with bounds -

...must call one or more times per extent, before returning, and where nbdkit collects the extents itself based on the sequence of calls rather than making the plugin itself manage an array. nbdkit plugin receive client BLOCK_STATUS call initialize array call plugin.extents call nbdkit_extent_add() add extent to array call nbdkit_extent_add() add extent to array ... return process the array -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org

...l errors)? > +++ b/docs/nbdkit-plugin.pod > +The callback should detect and return the list of extents overlapping > +the range C<[offset...offset+count-1]>. The C<extents_map> parameter > +points to an opaque object which the callback should fill in by > +calling C<nbdkit_extent_add> etc. See L</Extents map> below. > + > +The C<flags> parameter may contain the flag C<NBDKIT_FLAG_REQ_ONE> > +which means that the client is only requesting information about the > +extent overlapping C<offset>. The plugin may ignore this flag, or as > +a...

This tentative commit implements extents/can_extents, roughly as discussed in the previous thread here: https://www.redhat.com/archives/libguestfs/2019-March/msg00017.html I can't say that I'm a big fan of having the plugin allocate an extents array. There are no other plugin callbacks currently where we require the plugin to allocate complex data structures (or indeed do any allocation

...e than one extent > > (regardless of flags). > > Well, required to return at least one for the command to succeed (if the > return 0, then we synthesize an error, perhaps EIO, to keep the > connection to the client still alive). > > I take it you'd still have to use an nbdkit_extent_add(list, count, > type), so that additions to the list are still calling malloc() from > nbdkit's perspective (as you don't want the plugin to be realloc()ing > the list)? Yes I think we should preserve an abstract data type, not least because it would allow us to implement a differe...

I want to post this but mainly for discussion and early review. It's not safe for these patches to all go upstream yet (because not all filters have been checked/adjusted), but if any patches were to go upstream then probably 1 & 2 only are safe. File, VDDK, memory and data plugins all work, although I have only done minimal testing on them. The current tests, such as they are, all

...case is to treat the whole virtual +disk as if it was allocated. + +The callback should detect and return the list of extents overlapping +the range C<[offset...offset+count-1]>. The C<extents_map> parameter +points to an opaque object which the callback should fill in by +calling C<nbdkit_extent_add> etc. See L</Extents map> below. + +The C<flags> parameter may contain the flag C<NBDKIT_FLAG_REQ_ONE> +which means that the client is only requesting information about the +extent overlapping C<offset>. The plugin may ignore this flag, or as +an optimization it may ret...

...cs/nbdkit-plugin.pod > > > +The callback should detect and return the list of extents overlapping > > +the range C<[offset...offset+count-1]>. The C<extents_map> parameter > > +points to an opaque object which the callback should fill in by > > +calling C<nbdkit_extent_add> etc. See L</Extents map> below. > > + > > +The C<flags> parameter may contain the flag C<NBDKIT_FLAG_REQ_ONE> > > +which means that the client is only requesting information about the > > +extent overlapping C<offset>. The plugin may ignore this...

...d filters. --- server/nbdkit.syms | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/server/nbdkit.syms b/server/nbdkit.syms index 672abd2..95ef067 100644 --- a/server/nbdkit.syms +++ b/server/nbdkit.syms @@ -42,6 +42,10 @@ nbdkit_absolute_path; nbdkit_debug; nbdkit_error; + nbdkit_extent_add; + nbdkit_extents_free; + nbdkit_extents_foreach; + nbdkit_extents_new; nbdkit_parse_bool; nbdkit_parse_size; nbdkit_read_password; -- 2.20.1

...ext_ops->zero (nxdata, count, offs + offset, flags, err); } +/* Extents. */ +static int +subtract_offset (uint64_t offs, uint64_t length, uint32_t type, + void *vp) +{ + struct nbdkit_extents_map *extents_map = vp; + + assert (offs >= offset); + offs -= offset; + return nbdkit_extent_add (extents_map, offs, length, type); +} + +static int +offset_extents (struct nbdkit_next_ops *next_ops, void *nxdata, + void *handle, uint32_t count, uint64_t offs, uint32_t flags, + struct nbdkit_extents_map *extents_map, + int *err) +{ + struct nbdkit_...

...itable value. void nbdkit_extents_free (struct nbdkit_extents *); diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod index 272ec67..e9dc34f 100644 --- a/docs/nbdkit-plugin.pod +++ b/docs/nbdkit-plugin.pod @@ -812,7 +812,7 @@ guarantee that trimmed blocks read back as zeroes. C<nbdkit_extent_add> returns C<0> on success or C<-1> on failure. On failure C<nbdkit_error> and/or C<nbdkit_set_error> has already been -called. +called. C<errno> will be set to a suitable value. =head1 THREADS diff --git a/server/extents.c b/server/extents.c index 105c4a7..d3...

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