[Libguestfs] [PATCH nbdkit v5 FINAL 01/19] server: Implement extents/can_extents calls for plugins and filters.

[Richard W.M. Jones]

On Thu, Mar 28, 2019 at 09:24:25PM -0500, Eric Blake wrote:
> On 3/28/19 11:18 AM, Richard W.M. Jones wrote:
> > This new pair of callbacks allows plugins to describe which extents in
> > the virtual disk are allocated, holes or zeroes.
> > ---
> 
> > +++ 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?

Yes - I think we got all the permissions back so I'll submit
that change shortly.

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

Yes it's an off-by-one error, so I'll fix it for both.

> > +    nbdkit_error ("nbdkit_extents_new: "
> > +                  "start (%" PRIu64 ") or end (%" PRIu64 ") >= INT64_MAX",
> > +                  start, end);
> > +    errno = ERANGE;
> > +    return NULL;
> > +  }
> > +
> > +  /* 0-length ranges are possible, so start == end is not an error. */
> > +  if (start > end) {
> 
> The protocol declares them to be unspecified behavior (so a portable
> client shouldn't be doing it). Doesn't valid_range() in protocol.c
> already filter out 0-length requests from the client?  Thus, I'm
> wondering if this should be >= instead of >

I think if we do that we'll end up getting errors from filters (or
complicating filters).  It seems to be valid for a filter to want to
query the extents of a zero-sized slice of a plugin, even though from
a quick examination I don't think any existing filters do that today.

> > +  if (offset >= exts->end)
> > +    return 0;
> 
> Is returning 0 always right, or should we also check whether the user
> tried to pass in offset > end as their very first nbdkit_extents_add
> (where we know that because nr_extents is 0 that it is an API usage error).

If we allow zero sized ranges then is that true?  If zero sized ranges
are disallowed then I agree we could check for that.

> > +  /* Shorten extents that overlap the end of the range. */
> > +  if (offset + length >= exts->end) {
> > +    overlap = offset + length - exts->end;
> > +    length -= overlap;
> > +  }
> > +
> > +  /* If there are existing extents, the new extent must be contiguous. */
> > +  if (exts->nr_extents > 0) {
> > +    const struct nbdkit_extent *ep;
> > +
> > +    ep = &exts->extents[exts->nr_extents-1];
> > +    if (offset != ep->offset + ep->length) {
> > +      nbdkit_error ("nbdkit_add_extent: "
> > +                    "extents must be added in ascending order and "
> > +                    "must be contiguous");
> > +      return -1;
> > +    }
> > +  }
> > +  else {
> > +    /* If there are no existing extents, and the new extent is
> > +     * entirely before start, ignore it.
> 
> ...should this one also mention that we can't check for API usage errors
> for extents prior to start?

Yes, I'll update the comment.

I should note that if we stored some kind of "last position" then we
could check for API violations in all cases I think.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top