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

[Eric Blake]

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?

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


> +int
> +nbdkit_add_extent (struct nbdkit_extents *exts,
> +                   uint64_t offset, uint64_t length, uint32_t type)
> +{
> +  uint64_t overlap;
> +
> +  if (length == 0)
> +    return 0;
> +
> +  /* Ignore extents beyond the end of the range.  Unfortunately we
> +   * lost the information about whether this is contiguous with a
> +   * previously added extent so we can't check for API usage errors.

Nice comment...

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

> +
> +  /* 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?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20190328/94ef36fa/attachment.sig>