[Libguestfs] [PATCH nbdkit 1/9] server: Implement extents/can_extents calls for plugins and filters.

[Eric Blake]

On 3/19/19 11:34 AM, Richard W.M. Jones wrote:
> This pair of calls allows plugins to describe which extents in the
> virtual disk are allocated, holes or zeroes.
> ---

Resuming where I left off, if it helps:


> +#ifdef IN_TEST_EXTENTS
> +/* These functions are only compiled for and called from the unit tests. */
> +
> +void
> +dump_extents_map (const struct nbdkit_extents_map *map)
> +{
> +  size_t i;
> +
> +  fprintf (stderr, "map %p:\n", map);
> +  fprintf (stderr, "\tnr_extents=%zu allocated=%zu\n",
> +           map->nr_extents, map->allocated);
> +  for (i = 0; i < map->nr_extents; ++i) {
> +    fprintf (stderr, "\textent %zu:", i);
> +    fprintf (stderr,
> +             "\toffset=%" PRIu64 " length=%" PRIu64 " "
> +             "last=%" PRIu64 " type=%" PRIu32 "\n",

Worth using fixed-width formatting so that consecutive lines are
tabular, and/or using hex offsets (in addition to decimal) for easier
spotting of alignments?

> +             map->extents[i].offset, map->extents[i].length,
> +             map->extents[i].offset + map->extents[i].length - 1,
> +             map->extents[i].type);
> +  }
> +}
> +
> +void
> +validate_extents_map (const struct nbdkit_extents_map *map)
> +{
> +  size_t i;
> +  bool fail = false;
> +
> +  if (map->nr_extents > map->allocated) {
> +    fprintf (stderr, "validate_extents_map: nr_extents > allocated\n");
> +    fail = true;
> +  }
> +
> +  for (i = 0; i < map->nr_extents; ++i) {

Of course, if we failed the previous check, this now reads beyond the
end of the allocation. But it's debug code.

> +    /* Extent length must be > 0. */
> +    if (map->extents[i].length == 0) {
> +      fprintf (stderr, "validate_extents_map: %zu: extent length must be > 0\n",
> +               i);
> +      fail = true;
> +    }
> +
> +    /* Extent length must not wrap around, indicating that we have
> +     * computed a negative length (as unsigned) somewhere.
> +     */
> +    if (map->extents[i].offset + map->extents[i].length <
> +        map->extents[i].offset) {
> +      fprintf (stderr, "validate_extents_map: %zu: negative length\n",
> +               i);
> +      fail = true;
> +    }
> +

No validation that extents don't exceed INT64_MAX?

> +    if (i > 0) {
> +      /* Extents are stored in strictly ascending order. */
> +      if (map->extents[i-1].offset >= map->extents[i].offset) {
> +        fprintf (stderr,
> +                 "validate_extents_map: %zu: "
> +                 "not strictly ascending order\n", i);
> +        fail = true;
> +      }
> +
> +      /* Adjacent extents must not overlap. */
> +      if (map->extents[i-1].offset + map->extents[i-1].length >
> +          map->extents[i].offset) {
> +        fprintf (stderr,
> +                 "validate_extents_map: %zu: "
> +                 "overlapping extents\n", i);
> +        fail = true;
> +      }
> +
> +      /* Adjacent extents either have a different type or are
> +       * separated by a gap.

This changes if you don't allow gaps (it does sound like the
implementation will be easier if it is gap-free...)

> +       */
> +      if (map->extents[i-1].type == map->extents[i].type &&
> +          map->extents[i-1].offset + map->extents[i-1].length ==
> +          map->extents[i].offset) {
> +        fprintf (stderr,
> +                 "validate_extents_map: %zu: "
> +                 "adjacent extents with same type not coalesced\n", i);
> +        fail = true;
> +      }
> +    }
> +  }
> +
> +  if (fail) {
> +    dump_extents_map (map);
> +    abort ();
> +  }
> +}

> @@ -511,6 +530,21 @@ filter_can_zero (struct backend *b, struct connection *conn)
>      return f->backend.next->can_zero (f->backend.next, conn);
>  }
>  
> +static int
> +filter_can_extents (struct backend *b, struct connection *conn)
> +{
> +  struct backend_filter *f = container_of (b, struct backend_filter, backend);
> +  void *handle = connection_get_handle (conn, f->backend.i);
> +  struct b_conn nxdata = { .b = f->backend.next, .conn = conn };
> +
> +  debug ("%s: can_extents", f->name);
> +
> +  if (f->filter.can_extents)
> +    return f->filter.can_extents (&next_ops, &nxdata, handle);
> +  else
> +    return f->backend.next->can_extents (f->backend.next, conn);

For now, should we default filter.can_extents to false until we
completed the work for all filters?

> +++ b/server/test-extents.c

> +
> +static int
> +compare (uint64_t offset, uint64_t length, uint32_t type, void *mapv)
> +{
> +  const struct nbdkit_extents_map *map = mapv;
> +  size_t j;
> +
> +  /* nbdkit_extents_foreach_range should ensure this is true even if
> +   * there are extents which go below or above the range.
> +   */
> +  assert (offset >= 0);

Dead assertion (always true since offset is unsigned).

> +
> +  /* Read out the extents and compare them to the disk. */
> +  assert (nbdkit_extents_foreach (map, compare, map,
> +                                  NBDKIT_EXTENTS_FOREACH_FLAG_RANGE,
> +                                  0, DISK_SIZE-1) == 0);
> +

The test is rendered less powerful when NDEBUG is defined.  But it
doesn't affect production code (and maybe we could ensure that this file
#undef NDEBUG regardless of what the user decides via -D for assertions
in the overall project)


-- 
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/20190320/41118ec8/attachment.sig>