Libguestfs - Jul 2020 - [nbdkit PATCH] blocksize: Fix .extents when plugin changes type within minblock

It is easy to demonstrate that our blocksize filter has a bug: if the
minblock= setting is a higher granularity than the underlying plugin
actually supports, and the client is trying to collect block status
for the entire disk, a mid-block transition in the plugin can result
in the filter rounding a request so small that it no longer makes
progress, causing the client to see:

nbd.Error: nbd_block_status: block-status: command failed: Invalid argument
(EINVAL)

Better is to round mid-block transitions up to the block size; even
though the client can make read/write requests smaller than the block
alignment, they should not see transitions in status at that
granularity.

Fixes: 2515532316
Signed-off-by: Eric Blake <eblake@redhat.com>
---

I'm pushing this already since the tests prove it makes a difference,
but it was tricky enough that I thought it worth documenting on-list.

 filters/blocksize/Makefile.am   |   6 +-
 tests/Makefile.am               |   4 +-
 filters/blocksize/blocksize.c   |  40 ++++++++++---
 tests/test-blocksize-extents.sh | 102 ++++++++++++++++++++++++++++++++
 4 files changed, 140 insertions(+), 12 deletions(-)
 create mode 100755 tests/test-blocksize-extents.sh

diff --git a/filters/blocksize/Makefile.am b/filters/blocksize/Makefile.am
index 54a09bdb..0cb6b1f8 100644
--- a/filters/blocksize/Makefile.am
+++ b/filters/blocksize/Makefile.am
@@ -1,5 +1,5 @@
 # nbdkit
-# Copyright (C) 2018 Red Hat Inc.
+# Copyright (C) 2018-2020 Red Hat Inc.
 #
 # Redistribution and use in source and binary forms, with or without
 # modification, are permitted provided that the following conditions are
@@ -43,12 +43,16 @@ nbdkit_blocksize_filter_la_SOURCES = \
 nbdkit_blocksize_filter_la_CPPFLAGS = \
 	-I$(top_srcdir)/include \
 	-I$(top_srcdir)/common/include \
+	-I$(top_srcdir)/common/utils \
 	$(NULL)
 nbdkit_blocksize_filter_la_CFLAGS = $(WARNINGS_CFLAGS)
 nbdkit_blocksize_filter_la_LDFLAGS = \
 	-module -avoid-version -shared $(SHARED_LDFLAGS) \
 	-Wl,--version-script=$(top_srcdir)/filters/filters.syms \
 	$(NULL)
+nbdkit_blocksize_filter_la_LIBADD = \
+	$(top_builddir)/common/utils/libutils.la \
+	$(NULL)

 if HAVE_POD

diff --git a/tests/Makefile.am b/tests/Makefile.am
index 481291e6..c0c7e981 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -1199,8 +1199,8 @@ test_layers_filter3_la_LDFLAGS = \
 	$(NULL)

 # blocksize filter test.
-TESTS += test-blocksize.sh
-EXTRA_DIST += test-blocksize.sh
+TESTS += test-blocksize.sh test-blocksize-extents.sh
+EXTRA_DIST += test-blocksize.sh test-blocksize-extents.sh

 # cache filter test.
 TESTS += \
diff --git a/filters/blocksize/blocksize.c b/filters/blocksize/blocksize.c
index 8504f985..c3a2d60d 100644
--- a/filters/blocksize/blocksize.c
+++ b/filters/blocksize/blocksize.c
@@ -43,6 +43,7 @@

 #include <nbdkit-filter.h>

+#include "cleanup.h"
 #include "minmax.h"
 #include "rounding.h"

@@ -372,16 +373,37 @@ blocksize_extents (struct nbdkit_next_ops *next_ops, void
*nxdata,
                    void *handle, uint32_t count, uint64_t offset,
                    uint32_t flags, struct nbdkit_extents *extents, int *err)
 {
-  /* Ask the plugin for blocksize-aligned data.  Since the extents
-   * list start is set to the real offset, everything before the
-   * offset is ignored automatically.  Also we only need to ask for
-   * maxlen of data, because it's fine to return less than the full
-   * count as long as we're making progress.
+  /* Ask the plugin for blocksize-aligned data.  Copying that into the
+   * callers' extents will then take care of truncating unaligned
+   * ends.  Also we only need to ask for maxlen of data, because it's
+   * fine to return less than the full count as long as we're making
+   * progress.
    */
-  return next_ops->extents (nxdata,
-                            MIN (count, maxlen),
-                            ROUND_DOWN (offset, minblock),
-                            flags, extents, err);
+  CLEANUP_EXTENTS_FREE struct nbdkit_extents *extents2 = NULL;
+  size_t i;
+  struct nbdkit_extent e;
+
+  extents2 = nbdkit_extents_new (ROUND_DOWN (offset, minblock),
+                                 ROUND_UP (offset + count, minblock));
+  if (extents2 == NULL) {
+    *err = errno;
+    return -1;
+  }
+
+  if (nbdkit_extents_aligned (next_ops, nxdata,
+                              MIN (ROUND_UP (count, minblock), maxlen),
+                              ROUND_DOWN (offset, minblock),
+                              flags, minblock, extents2, err) == -1)
+    return -1;
+
+  for (i = 0; i < nbdkit_extents_count (extents2); ++i) {
+    e = nbdkit_get_extent (extents2, i);
+    if (nbdkit_add_extent (extents, e.offset, e.length, e.type) == -1) {
+      *err = errno;
+      return -1;
+    }
+  }
+  return 0;
 }

 static int
diff --git a/tests/test-blocksize-extents.sh b/tests/test-blocksize-extents.sh
new file mode 100755
index 00000000..156777ad
--- /dev/null
+++ b/tests/test-blocksize-extents.sh
@@ -0,0 +1,102 @@
+#!/usr/bin/env bash
+# nbdkit
+# Copyright (C) 2018-2020 Red Hat Inc.
+#
+# Redistribution and use in source and binary forms, with or without
+# modification, are permitted provided that the following conditions are
+# met:
+#
+# * Redistributions of source code must retain the above copyright
+# notice, this list of conditions and the following disclaimer.
+#
+# * Redistributions in binary form must reproduce the above copyright
+# notice, this list of conditions and the following disclaimer in the
+# documentation and/or other materials provided with the distribution.
+#
+# * Neither the name of Red Hat nor the names of its contributors may be
+# used to endorse or promote products derived from this software without
+# specific prior written permission.
+#
+# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS
IS'' AND
+# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
+# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
+# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
+# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+# SUCH DAMAGE.
+
+# Demonstrate a fix for a bug where blocksize used to cause extents failures
+
+source ./functions.sh
+set -e
+set -x
+
+requires nbdsh --base-allocation -c 'exit(not h.supports_uri())'
+
+# Script a server that requires 512-byte aligned requests, reports only one
+# extent at a time, and with a hole placed unaligned to 4k bounds
+exts='
+if test $3 -gt $(( 32 * 1024 )); then
+  echo "EINVAL request too large" 2>&1
+  exit 1
+fi
+if test $(( ($3|$4) & 511 )) != 0; then
+  echo "EINVAL request unaligned" 2>&1
+  exit 1
+fi
+type+if test $(( $4 >= 512 && $4 < 8 * 1024 )) = 1; then
+  type=hole,zero
+fi
+echo $4 512 $type
+'
+
+# We also need an nbdsh script to parse all extents, coalescing adjacent
+# types for simplicity, as well as testing some unaligned probes.
+export script='
+size = h.get_size()
+offs = 0
+entries = []
+def f (metacontext, offset, e, err):
+    global entries
+    global offs
+    assert offs == offset
+    for length, flags in zip(*[iter(e)] * 2):
+        if entries and flags == entries[-1][1]:
+            entries[-1] = (entries[-1][0] + length, flags)
+        else:
+            entries.append((length, flags))
+        offs = offs + length
+
+# Test a loop over the entire device
+while offs < size:
+    h.block_status (size - offs, offs, f)
+assert entries == [(4096, 0), (4096, 3), (57344, 0)]
+
+# Unaligned status queries must also work
+offs = 1
+entries = []
+h.block_status (1, offs, f, nbd.CMD_FLAG_REQ_ONE)
+assert entries == [(1, 0)]
+
+offs = 512
+entries = []
+h.block_status (512, offs, f)
+assert entries == [(3584, 0)]
+
+offs = 4095
+entries=[]
+while offs < 4097:
+    h.block_status (4097 - offs, offs, f, nbd.CMD_FLAG_REQ_ONE)
+assert entries == [(1, 0), (1, 3)]
+'
+
+# Now run everything
+nbdkit -U - --filter=blocksize eval minblock=4k maxlen=32k \
+       get_size='echo 64k' pread='exit 1'
extents="$exts" \
+       --run 'nbdsh --base-allocation -u $uri -c "$script"'
-- 
2.27.0