Libguestfs - Feb 2022 - [PATCH nbdkit v2 7/7] New filter: nbdkit-block-size-constraint-filter

This filter can add block size constraints to plugins which don't
already support them.  It can also enforce an error policy for badly
behaved clients which do not obey the block size constraints.
---
 docs/nbdkit-plugin.pod                        |   4 +-
 .../nbdkit-blocksize-policy-filter.pod        | 136 +++++++
 configure.ac                                  |   2 +
 filters/blocksize-policy/Makefile.am          |  70 ++++
 tests/Makefile.am                             |   4 +
 filters/blocksize-policy/policy.c             | 361 ++++++++++++++++++
 tests/test-blocksize-error-policy.sh          |  93 +++++
 tests/test-blocksize-policy.sh                | 117 ++++++
 8 files changed, 786 insertions(+), 1 deletion(-)

diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod
index 7f1da781..6c9b450f 100644
--- a/docs/nbdkit-plugin.pod
+++ b/docs/nbdkit-plugin.pod
@@ -894,7 +894,9 @@ that all client requests will obey the constraints.  A
client could
 still ignore the constraints.  nbdkit passes all requests through to
 the plugin, because what the plugin does depends on the plugin's
 policy.  It might decide to serve the requests correctly anyway, or
-reject them with an error.
+reject them with an error.  Plugins can avoid this complexity by using
+L<nbdkit-blocksize-policy-filter(1)> which allows both
+setting/adjusting the constraints, and selecting an error policy.
 
 The minimum block size must be E<ge> 1.  The maximum block size must
 be E<le> 0xffff_ffff.  minimum E<le> preferred E<le> maximum.
diff --git a/filters/blocksize-policy/nbdkit-blocksize-policy-filter.pod
b/filters/blocksize-policy/nbdkit-blocksize-policy-filter.pod
new file mode 100644
index 00000000..d02ecfe2
--- /dev/null
+++ b/filters/blocksize-policy/nbdkit-blocksize-policy-filter.pod
@@ -0,0 +1,136 @@
+=head1 NAME
+
+nbdkit-blocksize-policy-filter - set minimum, preferred and
+maximum block size, and apply error policy
+
+=head1 SYNOPSIS
+
+ nbdkit --filter=blocksize-policy PLUGIN
+        [blocksize-error-policy=allow|error]
+        [blocksize-minimum=N]
+        [blocksize-preferred=N]
+        [blocksize-maximum=N]
+
+=head1 DESCRIPTION
+
+C<nbdkit-blocksize-policy-filter> is an L<nbdkit(1)> filter that
+can add block size constraints to plugins which don't already support
+then.  It can also enforce an error policy for badly behaved clients
+which do not obey the block size constraints.
+
+For more information about block size constraints, see section
+"Block size constraints" in
+L<https://github.com/NetworkBlockDevice/nbd/blob/master/doc/proto.md>.
+
+The simplest usage is to place this filter on top of any plugin which
+does not advertise block size constraints, and set the
+C<blocksize-minimum>, C<blocksize-preferred> and
C<blocksize-maximum>
+parameters with the desired constraints.  For example:
+
+ nbdkit --filter=blocksize-policy memory 1G \
+        blocksize-preferred=32K
+
+would adjust L<nbdkit-memory-plugin(1)> so that clients should
+prefer 32K requests.  You can query the NBD server advertised constraints
+using L<nbdinfo(1)>:
+
+ $ nbdinfo nbd://localhost
+ [...]
+     block_size_minimum: 1
+     block_size_preferred: 32768
+     block_size_maximum: 4294967295
+
+The second part of this filter is adjusting the error policy when
+badly behaved clients do not obey the minimum or maximum request size.
+Normally nbdkit permits these requests, leaving it up to the plugin
+whether it rejects the request with an error or tries to process the
+request (eg. trying to split an over-large request).  With this filter
+you can use C<blocksize-error-policy=error> to reject these requests
+in the filter with EIO error.  The plugin will not see them.
+
+=head2 Combining with L<nbdkit-blocksize-filter(1)>
+
+A related filter is L<nbdkit-blocksize-filter(1)>.  That filter can
+split and combine requests for plugins that cannot handle requests
+under or over a particular size.
+
+Both filters may be used together like this (note that the order of
+the filters is important):
+
+  nbdkit --filter=blocksize-policy \
+         --filter=blocksize \
+         PLUGIN ... \
+         blocksize-error-policy=allow \
+         blocksize-minimum=64K minblock=64K
+
+This says to advertise a minimum block size of 64K.  Well-behaved
+clients will obey this.  Badly behaved clients will send requests
+S<E<lt> 64K> which will be converted to slow 64K read-modify-write
+cycles to the underlying plugin.  In either case the plugin will only
+see requests on 64K (or multiples of 64K) boundaries.
+
+=head1 PARAMETERS
+
+=over 4
+
+=item B<blocksize-error-policy=allow>
+
+=item B<blocksize-error-policy=error>
+
+If a client sends a request which is smaller than the permitted
+minimum size or larger than the permitted maximum size, or not aligned
+to the minimum size, C<blocksize-error-policy> chooses what the filter
+will do.  The default (and also nbdkit's default) is C<allow> which
+means pass the request through to the plugin.
+
+Use C<error> to return an EIO error back to the client.  The plugin
+will not see the badly formed request in this case.
+
+=item B<blocksize-minimum=>N
+
+=item B<blocksize-preferred=>N
+
+=item B<blocksize-maximum=>N
+
+Advertise minimum, preferred and/or maximum block size to the client.
+Well-behaved clients should obey these constraints.
+
+For each parameter, you can specify it as a size (using the usual
+modifiers like C<4K>).
+
+If the parameter is omitted then either the constraint advertised by
+the plugin itself is used, or a sensible default for plugins which do
+not advertise block size constraints.
+
+=back
+
+=head1 FILES
+
+=over 4
+
+=item F<$filterdir/nbdkit-blocksize-policy-filter.so>
+
+The filter.
+
+Use C<nbdkit --dump-config> to find the location of C<$filterdir>.
+
+=back
+
+=head1 VERSION
+
+C<nbdkit-limit-filter> first appeared in nbdkit 1.30.
+
+=head1 SEE ALSO
+
+L<nbdkit(1)>,
+L<nbdkit-blocksize-filter(1)>,
+L<nbdkit-filter(3)>,
+L<nbdkit-plugin(3)>.
+
+=head1 AUTHORS
+
+Richard W.M. Jones
+
+=head1 COPYRIGHT
+
+Copyright (C) 2022 Red Hat Inc.
diff --git a/configure.ac b/configure.ac
index 1c7200c9..f453664f 100644
--- a/configure.ac
+++ b/configure.ac
@@ -109,6 +109,7 @@ non_lang_plugins="\
 plugins="$(echo $(printf %s\\n $lang_plugins $non_lang_plugins | sort
-u))"
 filters="\
         blocksize \
+        blocksize-policy \
         cache \
         cacheextents \
         checkwrite \
@@ -1335,6 +1336,7 @@ AC_CONFIG_FILES([Makefile
                  plugins/zero/Makefile
                  filters/Makefile
                  filters/blocksize/Makefile
+                 filters/blocksize-policy/Makefile
                  filters/cache/Makefile
                  filters/cacheextents/Makefile
                  filters/checkwrite/Makefile
diff --git a/filters/blocksize-policy/Makefile.am
b/filters/blocksize-policy/Makefile.am
new file mode 100644
index 00000000..004c8474
--- /dev/null
+++ b/filters/blocksize-policy/Makefile.am
@@ -0,0 +1,70 @@
+# nbdkit
+# Copyright (C) 2019-2021 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.
+
+include $(top_srcdir)/common-rules.mk
+
+EXTRA_DIST = nbdkit-blocksize-policy-filter.pod
+
+filter_LTLIBRARIES = nbdkit-blocksize-policy-filter.la
+
+nbdkit_blocksize_policy_filter_la_SOURCES = \
+	policy.c \
+	$(top_srcdir)/include/nbdkit-filter.h \
+	$(NULL)
+
+nbdkit_blocksize_policy_filter_la_CPPFLAGS = \
+	-I$(top_srcdir)/common/include \
+	-I$(top_srcdir)/common/utils \
+	-I$(top_srcdir)/include \
+	$(NULL)
+nbdkit_blocksize_policy_filter_la_CFLAGS = $(WARNINGS_CFLAGS)
+nbdkit_blocksize_policy_filter_la_LDFLAGS = \
+	-module -avoid-version -shared $(NO_UNDEFINED_ON_WINDOWS) \
+	-Wl,--version-script=$(top_srcdir)/filters/filters.syms \
+	$(NULL)
+nbdkit_blocksize_policy_filter_la_LIBADD = \
+	$(top_builddir)/common/utils/libutils.la \
+	$(top_builddir)/common/replacements/libcompat.la \
+	$(IMPORT_LIBRARY_ON_WINDOWS) \
+	$(NULL)
+
+if HAVE_POD
+
+man_MANS = nbdkit-blocksize-policy-filter.1
+CLEANFILES += $(man_MANS)
+
+nbdkit-blocksize-policy-filter.1: nbdkit-blocksize-policy-filter.pod \
+		$(top_builddir)/podwrapper.pl
+	$(PODWRAPPER) --section=1 --man $@ \
+	    --html $(top_builddir)/html/$@.html \
+	    $<
+
+endif HAVE_POD
diff --git a/tests/Makefile.am b/tests/Makefile.am
index e888b1a0..66156a3b 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -1409,6 +1409,10 @@ test_layers_filter3_la_LIBADD =
$(IMPORT_LIBRARY_ON_WINDOWS)
 TESTS += test-blocksize.sh test-blocksize-extents.sh
 EXTRA_DIST += test-blocksize.sh test-blocksize-extents.sh
 
+# blocksize-policy filter test.
+TESTS += test-blocksize-policy.sh test-blocksize-error-policy.sh
+EXTRA_DIST += test-blocksize-policy.sh test-blocksize-error-policy.sh
+
 # cache filter test.
 TESTS += \
 	test-cache.sh \
diff --git a/filters/blocksize-policy/policy.c
b/filters/blocksize-policy/policy.c
new file mode 100644
index 00000000..2cc13ee2
--- /dev/null
+++ b/filters/blocksize-policy/policy.c
@@ -0,0 +1,361 @@
+/* nbdkit
+ * Copyright (C) 2019-2022 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.
+ */
+
+#include <config.h>
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <stdint.h>
+#include <inttypes.h>
+#include <string.h>
+#include <errno.h>
+
+#include <nbdkit-filter.h>
+
+#include "ispowerof2.h"
+
+/* Block size constraints configured on the command line (0 = unset). */
+static uint32_t config_minimum;
+static uint32_t config_preferred;
+static uint32_t config_maximum;
+
+/* Error policy. */
+static enum { ALLOW, ERROR } error_policy = ALLOW;
+
+static int
+policy_config (nbdkit_next_config *next, nbdkit_backend *nxdata,
+               const char *key, const char *value)
+{
+  int64_t r;
+
+  if (strcmp (key, "blocksize-error-policy") == 0) {
+    if (strcmp (value, "allow") == 0)
+      error_policy = ALLOW;
+    else if (strcmp (value, "error") == 0)
+      error_policy = ERROR;
+    else {
+      nbdkit_error ("unknown %s: %s", key, value);
+      return -1;
+    }
+    return 0;
+  }
+  else if (strcmp (key, "blocksize-minimum") == 0) {
+    r = nbdkit_parse_size (value);
+    if (r == -1 || r > UINT32_MAX) {
+    parse_error:
+      nbdkit_error ("%s: could not parse %s", key, value);
+      return -1;
+    }
+    config_minimum = r;
+    return 0;
+  }
+  else if (strcmp (key, "blocksize-preferred") == 0) {
+    r = nbdkit_parse_size (value);
+    if (r == -1 || r > UINT32_MAX) goto parse_error;
+    config_preferred = r;
+    return 0;
+  }
+  else if (strcmp (key, "blocksize-maximum") == 0) {
+    r = nbdkit_parse_size (value);
+    if (r == -1 || r > UINT32_MAX) goto parse_error;
+    config_maximum = r;
+    return 0;
+  }
+
+  return next (nxdata, key, value);
+}
+
+static int
+policy_config_complete (nbdkit_next_config_complete *next,
+                        nbdkit_backend *nxdata)
+{
+  /* These checks roughly reflect the same checks made in
+   * server/plugins.c: plugin_block_size
+   */
+
+  if (config_minimum) {
+    if (! is_power_of_2 (config_minimum)) {
+      nbdkit_error ("blocksize-minimum must be a power of 2");
+      return -1;
+    }
+    if (config_minimum > 65536) {
+      nbdkit_error ("blocksize-minimum must be <= 64K");
+      return -1;
+    }
+  }
+
+  if (config_preferred) {
+    if (! is_power_of_2 (config_preferred)) {
+      nbdkit_error ("blocksize-preferred must be a power of 2");
+      return -1;
+    }
+    if (config_preferred < 512 || config_preferred > 32 * 1024 * 1024) {
+      nbdkit_error ("blocksize-preferred must be between 512 and
32M");
+      return -1;
+    }
+  }
+
+  if (config_minimum && config_maximum) {
+    if (config_maximum != (uint32_t)-1 &&
+        (config_maximum % config_maximum) != 0) {
+      nbdkit_error ("blocksize-maximum must be -1 "
+                    "or a multiple of blocksize-minimum");
+      return -1;
+    }
+  }
+
+  if (config_minimum && config_preferred) {
+    if (config_minimum > config_preferred) {
+      nbdkit_error ("blocksize-minimum must be <=
blocksize-preferred");
+      return -1;
+    }
+  }
+
+  if (config_preferred && config_maximum) {
+    if (config_preferred > config_maximum) {
+      nbdkit_error ("blocksize-preferred must be <=
blocksize-maximum");
+      return -1;
+    }
+  }
+
+  return next (nxdata);
+}
+
+static int
+policy_block_size (nbdkit_next *next, void *handle,
+                   uint32_t *minimum, uint32_t *preferred, uint32_t *maximum)
+{
+  /* If the user has set all of the block size parameters then we
+   * don't need to ask the plugin, we can go ahead and advertise them.
+   */
+  if (config_minimum && config_preferred && config_maximum) {
+    *minimum = config_minimum;
+    *preferred = config_preferred;
+    *maximum = config_maximum;
+    return 0;
+  }
+
+  /* Otherwise, ask the plugin. */
+  if (next->block_size (next, minimum, preferred, maximum) == -1)
+    return -1;
+
+  /* If the user of this filter didn't configure anything, then return
+   * the plugin values (even if unset).
+   */
+  if (!config_minimum && !config_preferred && !config_maximum)
+    return 0;
+
+  /* Now we get to the awkward case where the user configured some
+   * values but not others.  There's all kinds of room for things to
+   * go wrong here, so try to check for obvious user errors as best we
+   * can.
+   */
+  if (*minimum == 0) {           /* Plugin didn't set anything. */
+    if (config_minimum)
+      *minimum = config_minimum;
+    else
+      *minimum = 1;
+
+    if (config_preferred)
+      *preferred = config_preferred;
+    else
+      *preferred = 4096;
+
+    if (config_maximum)
+      *maximum = config_maximum;
+    else
+      *maximum = 0xffffffff;
+  }
+  else {                        /* Plugin set some values. */
+    if (config_minimum)
+      *minimum = config_minimum;
+
+    if (config_preferred)
+      *preferred = config_preferred;
+
+    if (config_maximum)
+      *maximum = config_maximum;
+  }
+
+  if (*minimum > *preferred || *preferred > *maximum) {
+    nbdkit_error ("computed block size values are invalid, minimum %"
PRIu32
+                  " > preferred %" PRIu32
+                  " or preferred > maximum %" PRIu32,
+                  *minimum, *preferred, *maximum);
+    return -1;
+  }
+  return 0;
+}
+
+/* This function checks the error policy for all request functions below. */
+static int
+check_policy (nbdkit_next *next, const char *type,
+              uint32_t count, uint64_t offset, int *err)
+{
+  uint32_t minimum, preferred, maximum;
+
+  if (error_policy == ALLOW)
+    return 0;
+
+  /* Get the current block size constraints.  Note these are cached in
+   * the backend so if they've already been computed then this simply
+   * returns the cached values.  The plugin is only asked once per
+   * connection.
+   */
+  errno = 0;
+  if (next->block_size (next, &minimum, &preferred, &maximum) ==
-1) {
+    *err = errno ? : EIO;
+    return -1;
+  }
+
+  /* If there are no constraints, allow. */
+  if (minimum == 0)
+    return 0;
+
+  /* Check constraints. */
+  if (count < minimum) {
+    *err = EIO;
+    nbdkit_error ("client %s request rejected: "
+                  "count %" PRIu32 " is smaller than minimum
size %" PRIu32,
+                  type, count, minimum);
+    return -1;
+  }
+  if (count > maximum) {
+    *err = EIO;
+    nbdkit_error ("client %s request rejected: "
+                  "count %" PRIu32 " is larger than maximum size
%" PRIu32,
+                  type, count, maximum);
+    return -1;
+  }
+  if ((count % minimum) != 0) {
+    *err = EIO;
+    nbdkit_error ("client %s request rejected: "
+                  "count %" PRIu32 " is not a multiple "
+                  "of minimum size %" PRIu32,
+                  type, count, minimum);
+    return -1;
+  }
+  if ((offset % minimum) != 0) {
+    *err = EIO;
+    nbdkit_error ("client %s request rejected: "
+                  "offset %" PRIu64 " is not aligned to a
multiple "
+                  "of minimum size %" PRIu32,
+                  type, offset, minimum);
+    return -1;
+  }
+
+  return 0;
+}
+
+static int
+policy_pread (nbdkit_next *next,
+              void *handle, void *buf, uint32_t count, uint64_t offset,
+              uint32_t flags, int *err)
+{
+  if (check_policy (next, "pread", count, offset, err) == -1)
+    return -1;
+
+  return next->pread (next, buf, count, offset, flags, err);
+}
+
+static int
+policy_pwrite (nbdkit_next *next,
+               void *handle, const void *buf, uint32_t count, uint64_t offset,
+               uint32_t flags, int *err)
+{
+  if (check_policy (next, "write", count, offset, err) == -1)
+    return -1;
+
+  return next->pwrite (next, buf, count, offset, flags, err);
+}
+
+static int
+policy_zero (nbdkit_next *next,
+             void *handle, uint32_t count, uint64_t offset, uint32_t flags,
+             int *err)
+{
+  if (check_policy (next, "zero", count, offset, err) == -1)
+    return -1;
+
+  return next->zero (next, count, offset, flags, err);
+}
+
+static int
+policy_trim (nbdkit_next *next,
+             void *handle, uint32_t count, uint64_t offset, uint32_t flags,
+             int *err)
+{
+  if (check_policy (next, "trim", count, offset, err) == -1)
+    return -1;
+
+  return next->trim (next, count, offset, flags, err);
+}
+
+static int
+policy_cache (nbdkit_next *next,
+              void *handle, uint32_t count, uint64_t offset,
+              uint32_t flags, int *err)
+{
+  if (check_policy (next, "cache", count, offset, err) == -1)
+    return -1;
+
+  return next->cache (next, count, offset, flags, err);
+}
+
+static int
+policy_extents (nbdkit_next *next,
+                void *handle, uint32_t count, uint64_t offset, uint32_t flags,
+                struct nbdkit_extents *extents, int *err)
+{
+  if (check_policy (next, "extents", count, offset, err) == -1)
+    return -1;
+
+  return next->extents (next, count, offset, flags, extents, err);
+}
+
+static struct nbdkit_filter filter = {
+  .name              = "blocksize-policy",
+  .longname          = "nbdkit blocksize policy filter",
+  .config            = policy_config,
+  .config_complete   = policy_config_complete,
+
+  .block_size        = policy_block_size,
+
+  .pread             = policy_pread,
+  .pwrite            = policy_pwrite,
+  .zero              = policy_zero,
+  .trim              = policy_trim,
+  .cache             = policy_cache,
+  .extents           = policy_extents,
+};
+
+NBDKIT_REGISTER_FILTER(filter)
diff --git a/tests/test-blocksize-error-policy.sh
b/tests/test-blocksize-error-policy.sh
new file mode 100755
index 00000000..dc7d274c
--- /dev/null
+++ b/tests/test-blocksize-error-policy.sh
@@ -0,0 +1,93 @@
+#!/usr/bin/env bash
+# nbdkit
+# Copyright (C) 2019-2022 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.
+
+source ./functions.sh
+set -e
+set -x
+
+requires_plugin eval
+requires nbdsh -c 'print(h.get_block_size)'
+requires nbdsh -c 'print(h.get_strict_mode)'
+requires dd iflag=count_bytes </dev/null
+
+nbdkit -v -U - eval \
+       block_size="echo 512 4096 1M" \
+       get_size="echo 64M" \
+       pread=" dd if=/dev/zero count=\$3 iflag=count_bytes " \
+       --filter=blocksize-policy \
+       blocksize-error-policy=error \
+       --run '
+nbdsh \
+    -u "$uri" \
+    -c "assert h.get_block_size(nbd.SIZE_MINIMUM) == 512" \
+    -c "assert h.get_block_size(nbd.SIZE_PREFERRED) == 4096" \
+    -c "assert h.get_block_size(nbd.SIZE_MAXIMUM) == 1024 * 1024" \
+    -c "h.set_strict_mode(h.get_strict_mode() &
~nbd.STRICT_ALIGN)" \
+    -c "
+# These requests should work
+b = h.pread(512, 0)
+b = h.pread(512, 4096)
+b = h.pread(1024*1024, 0)
+" \
+    -c "
+# Count not a multiple of minimum size
+try:
+    h.pread(768, 0)
+    assert False
+except nbd.Error as ex:
+    assert ex.errno == \"EIO\"
+" \
+    -c "
+# Offset not a multiple of minimum size
+try:
+    h.pread(512, 768)
+    assert False
+except nbd.Error as ex:
+    assert ex.errno == \"EIO\"
+" \
+    -c "
+# Count smaller than minimum size
+try:
+    h.pread(256, 0)
+    assert False
+except nbd.Error as ex:
+    assert ex.errno == \"EIO\"
+" \
+    -c "
+# Count larger than maximum size
+try:
+    h.pread(2*1024*1024, 0)
+    assert False
+except nbd.Error as ex:
+    assert ex.errno == \"EIO\"
+"
+'
diff --git a/tests/test-blocksize-policy.sh b/tests/test-blocksize-policy.sh
new file mode 100755
index 00000000..4550c71f
--- /dev/null
+++ b/tests/test-blocksize-policy.sh
@@ -0,0 +1,117 @@
+#!/usr/bin/env bash
+# nbdkit
+# Copyright (C) 2019-2022 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.
+
+source ./functions.sh
+set -e
+set -x
+
+requires_plugin eval
+requires nbdsh -c 'print(h.get_block_size)'
+
+# Run nbdkit eval + filter and check resulting block size constraints
+# using some nbdsh.
+
+# No parameters.
+nbdkit -U - eval \
+       block_size="echo 64K 128K 32M" \
+       get_size="echo 0" \
+       --filter=blocksize-policy \
+       --run 'nbdsh \
+           -u "$uri" \
+           -c "assert h.get_block_size(nbd.SIZE_MINIMUM) == 64 *
1024" \
+           -c "assert h.get_block_size(nbd.SIZE_PREFERRED) == 128 *
1024" \
+           -c "assert h.get_block_size(nbd.SIZE_MAXIMUM) == 32 * 1024 *
1024" \
+      '
+
+# Adjust single values.
+nbdkit -U - eval \
+       block_size="echo 64K 128K 32M" \
+       get_size="echo 0" \
+       --filter=blocksize-policy \
+       blocksize-minimum=1 \
+       --run 'nbdsh \
+           -u "$uri" \
+           -c "assert h.get_block_size(nbd.SIZE_MINIMUM) == 1" \
+           -c "assert h.get_block_size(nbd.SIZE_PREFERRED) == 128 *
1024" \
+           -c "assert h.get_block_size(nbd.SIZE_MAXIMUM) == 32 * 1024 *
1024" \
+      '
+nbdkit -U - eval \
+       block_size="echo 64K 128K 32M" \
+       get_size="echo 0" \
+       --filter=blocksize-policy \
+       blocksize-preferred=64K \
+       --run 'nbdsh \
+           -u "$uri" \
+           -c "assert h.get_block_size(nbd.SIZE_MINIMUM) == 64 *
1024" \
+           -c "assert h.get_block_size(nbd.SIZE_PREFERRED) == 64 *
1024" \
+           -c "assert h.get_block_size(nbd.SIZE_MAXIMUM) == 32 * 1024 *
1024" \
+      '
+nbdkit -U - eval \
+       block_size="echo 64K 128K 32M" \
+       get_size="echo 0" \
+       --filter=blocksize-policy \
+       blocksize-maximum=1M \
+       --run 'nbdsh \
+           -u "$uri" \
+           -c "assert h.get_block_size(nbd.SIZE_MINIMUM) == 64 *
1024" \
+           -c "assert h.get_block_size(nbd.SIZE_PREFERRED) == 128 *
1024" \
+           -c "assert h.get_block_size(nbd.SIZE_MAXIMUM) == 1 * 1024 *
1024" \
+      '
+
+# Adjust all values for a plugin which is advertising.
+nbdkit -U - eval \
+       block_size="echo 64K 128K 32M" \
+       get_size="echo 0" \
+       --filter=blocksize-policy \
+       blocksize-minimum=1 \
+       blocksize-preferred=4K \
+       blocksize-maximum=1M \
+       --run 'nbdsh \
+           -u "$uri" \
+           -c "assert h.get_block_size(nbd.SIZE_MINIMUM) == 1" \
+           -c "assert h.get_block_size(nbd.SIZE_PREFERRED) == 4 *
1024" \
+           -c "assert h.get_block_size(nbd.SIZE_MAXIMUM) == 1 * 1024 *
1024" \
+      '
+
+# Set all values for a plugin which is not advertising.
+nbdkit -U - eval \
+       get_size="echo 0" \
+       --filter=blocksize-policy \
+       blocksize-minimum=1 \
+       blocksize-preferred=4K \
+       blocksize-maximum=1M \
+       --run 'nbdsh \
+           -u "$uri" \
+           -c "assert h.get_block_size(nbd.SIZE_MINIMUM) == 1" \
+           -c "assert h.get_block_size(nbd.SIZE_PREFERRED) == 4 *
1024" \
+           -c "assert h.get_block_size(nbd.SIZE_MAXIMUM) == 1 * 1024 *
1024" \
+      '
-- 
2.35.1

On Thu, Feb 17, 2022 at 02:36:48PM +0000, Richard W.M. Jones
wrote:
> +static int
> +policy_pread (nbdkit_next *next,
> +              void *handle, void *buf, uint32_t count, uint64_t offset,
> +              uint32_t flags, int *err)
> +{
> +  if (check_policy (next, "pread", count, offset, err) == -1)
> +    return -1;
> +
> +  return next->pread (next, buf, count, offset, flags, err);
> +}
> +
> +static int
> +policy_pwrite (nbdkit_next *next,
> +               void *handle, const void *buf, uint32_t count, uint64_t
offset,
> +               uint32_t flags, int *err)
> +{
> +  if (check_policy (next, "write", count, offset, err) == -1)
Typo, should be "pwrite" here.  Fixed in my copy.

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

On Thu, Feb 17, 2022 at 02:36:48PM +0000, Richard W.M. Jones
wrote:
> This filter can add block size constraints to plugins which don't
> already support them.  It can also enforce an error policy for badly
> behaved clients which do not obey the block size constraints.
> ---
> + nbdkit --filter=blocksize-policy PLUGIN
> +        [blocksize-error-policy=allow|error]
> +        [blocksize-minimum=N]
> +        [blocksize-preferred=N]
> +        [blocksize-maximum=N]
> +
> +=head1 DESCRIPTION
> +
> +C<nbdkit-blocksize-policy-filter> is an L<nbdkit(1)> filter
that
> +can add block size constraints to plugins which don't already support
> +then.  It can also enforce an error policy for badly behaved clients
s/then/them/
> +which do not obey the block size constraints.
> +
> +For more information about block size constraints, see section
> +"Block size constraints" in
>
+L<https://github.com/NetworkBlockDevice/nbd/blob/master/doc/proto.md>.
> +
> +The simplest usage is to place this filter on top of any plugin which
> +does not advertise block size constraints, and set the
> +C<blocksize-minimum>, C<blocksize-preferred> and
C<blocksize-maximum>
> +parameters with the desired constraints.  For example:
> +
> + nbdkit --filter=blocksize-policy memory 1G \
> +        blocksize-preferred=32K
> +
> +would adjust L<nbdkit-memory-plugin(1)> so that clients should
> +prefer 32K requests.  You can query the NBD server advertised constraints
> +using L<nbdinfo(1)>:
> +
> + $ nbdinfo nbd://localhost
> + [...]
> +     block_size_minimum: 1
> +     block_size_preferred: 32768
> +     block_size_maximum: 4294967295
> +
> +The second part of this filter is adjusting the error policy when
> +badly behaved clients do not obey the minimum or maximum request size.
> +Normally nbdkit permits these requests, leaving it up to the plugin
> +whether it rejects the request with an error or tries to process the
> +request (eg. trying to split an over-large request).  With this filter
Maybe also mention 'or doing a read-modify-write for an unaligned
write'?
> +you can use C<blocksize-error-policy=error> to reject these requests
> +in the filter with EIO error.  The plugin will not see them.
> +
> +=head2 Combining with L<nbdkit-blocksize-filter(1)>
> +
> +A related filter is L<nbdkit-blocksize-filter(1)>.  That filter can
> +split and combine requests for plugins that cannot handle requests
> +under or over a particular size.
> +
> +Both filters may be used together like this (note that the order of
> +the filters is important):
> +
> +  nbdkit --filter=blocksize-policy \
> +         --filter=blocksize \
> +         PLUGIN ... \
> +         blocksize-error-policy=allow \
> +         blocksize-minimum=64K minblock=64K
> +
> +This says to advertise a minimum block size of 64K.  Well-behaved
> +clients will obey this.  Badly behaved clients will send requests
> +S<E<lt> 64K> which will be converted to slow 64K
read-modify-write
> +cycles to the underlying plugin.  In either case the plugin will only
> +see requests on 64K (or multiples of 64K) boundaries.
Should the blocksize filter implement .block_size?  But that's a
separate patch compared to this one adding a new filter.  The example
looks good.
> +++ b/filters/blocksize-policy/policy.c
> +
> +static int
> +policy_config (nbdkit_next_config *next, nbdkit_backend *nxdata,
> +               const char *key, const char *value)
> +{
> +  int64_t r;
> +
> +  if (strcmp (key, "blocksize-error-policy") == 0) {
> +    if (strcmp (value, "allow") == 0)
> +      error_policy = ALLOW;
> +    else if (strcmp (value, "error") == 0)
> +      error_policy = ERROR;
> +    else {
> +      nbdkit_error ("unknown %s: %s", key, value);
> +      return -1;
> +    }
> +    return 0;
> +  }
> +  else if (strcmp (key, "blocksize-minimum") == 0) {
> +    r = nbdkit_parse_size (value);
> +    if (r == -1 || r > UINT32_MAX) {
Should this be 'r <= -1' (or 'r < 0') to flag all negative
values as invalid?

/me goes back and re-reads the docs for nbdkit_parse_size

Nope, correct as is, because nbdkit_parse_size already rejects
negatives, and reserves just -1 to indicate any parse error.
> +
> +static int
> +policy_block_size (nbdkit_next *next, void *handle,
> +                   uint32_t *minimum, uint32_t *preferred, uint32_t
*maximum)
> +{
> +  /* If the user has set all of the block size parameters then we
> +   * don't need to ask the plugin, we can go ahead and advertise them.
> +   */
> +  if (config_minimum && config_preferred &&
config_maximum) {
> +    *minimum = config_minimum;
> +    *preferred = config_preferred;
> +    *maximum = config_maximum;
> +    return 0;
> +  }
> +
> +  /* Otherwise, ask the plugin. */
> +  if (next->block_size (next, minimum, preferred, maximum) == -1)
> +    return -1;
> +
> +  /* If the user of this filter didn't configure anything, then return
> +   * the plugin values (even if unset).
> +   */
> +  if (!config_minimum && !config_preferred &&
!config_maximum)
> +    return 0;
> +
> +  /* Now we get to the awkward case where the user configured some
> +   * values but not others.  There's all kinds of room for things to
> +   * go wrong here, so try to check for obvious user errors as best we
> +   * can.
> +   */
> +  if (*minimum == 0) {           /* Plugin didn't set anything. */
> +    if (config_minimum)
> +      *minimum = config_minimum;
> +    else
> +      *minimum = 1;
> +
> +    if (config_preferred)
> +      *preferred = config_preferred;
> +    else
> +      *preferred = 4096;
> +
> +    if (config_maximum)
> +      *maximum = config_maximum;
> +    else
> +      *maximum = 0xffffffff;
I'd be inclined to advertise 64*1024*1024 here instead of 0xffffffff,
to match the actual buffer size that we know nbdkit is constrained by.

Then again, doing that may cause clients to self-limit calls to .zero
with at most 64M in a chunk, instead of calling it with 4G in a chunk.
It's a toss-up.  (The NBD protocol really does need a fourth value, to
distinguish between maximum buffer and maximum length)
> +  }
> +  else {                        /* Plugin set some values. */
> +    if (config_minimum)
> +      *minimum = config_minimum;
> +
> +    if (config_preferred)
> +      *preferred = config_preferred;
> +
> +    if (config_maximum)
> +      *maximum = config_maximum;
> +  }
> +
> +  if (*minimum > *preferred || *preferred > *maximum) {
> +    nbdkit_error ("computed block size values are invalid, minimum
%" PRIu32
> +                  " > preferred %" PRIu32
> +                  " or preferred > maximum %" PRIu32,
> +                  *minimum, *preferred, *maximum);
Good enough for a first cut.  We may find ourselves having to refine
it over time (my biggest worry is a plugin that clamps maximum to the
advertised .get_length size, even if that is smaller than a hardcoded
preferred size - even though right now we are rejecting that in
server/plugins.c), but that is doable.
> +/* This function checks the error policy for all request functions below.
*/
> +static int
> +check_policy (nbdkit_next *next, const char *type,
> +              uint32_t count, uint64_t offset, int *err)
Ah, new code compared to the last draft ;)
> +{
> +  uint32_t minimum, preferred, maximum;
> +
> +  if (error_policy == ALLOW)
> +    return 0;
> +
> +  /* Get the current block size constraints.  Note these are cached in
> +   * the backend so if they've already been computed then this simply
> +   * returns the cached values.  The plugin is only asked once per
> +   * connection.
> +   */
> +  errno = 0;
> +  if (next->block_size (next, &minimum, &preferred,
&maximum) == -1) {
> +    *err = errno ? : EIO;
> +    return -1;
> +  }
Correct.
> +
> +  /* If there are no constraints, allow. */
> +  if (minimum == 0)
> +    return 0;
> +
> +  /* Check constraints. */
> +  if (count < minimum) {
> +    *err = EIO;
> +    nbdkit_error ("client %s request rejected: "
> +                  "count %" PRIu32 " is smaller than
minimum size %" PRIu32,
> +                  type, count, minimum);
> +    return -1;
> +  }
> +  if (count > maximum) {
> +    *err = EIO;
> +    nbdkit_error ("client %s request rejected: "
> +                  "count %" PRIu32 " is larger than maximum
size %" PRIu32,
> +                  type, count, maximum);
> +    return -1;
> +  }
This is where I worry about size constraints on data (pread/pwrite)
vs. length (zero/trim/cache).  Again, I'm happy to propose a patch on
top of this, so we can use this as our starting point.
> +  if ((count % minimum) != 0) {
> +    *err = EIO;
> +    nbdkit_error ("client %s request rejected: "
> +                  "count %" PRIu32 " is not a multiple
"
> +                  "of minimum size %" PRIu32,
> +                  type, count, minimum);
> +    return -1;
> +  }
> +  if ((offset % minimum) != 0) {
> +    *err = EIO;
> +    nbdkit_error ("client %s request rejected: "
> +                  "offset %" PRIu64 " is not aligned to a
multiple "
> +                  "of minimum size %" PRIu32,
> +                  type, offset, minimum);
> +    return -1;
> +  }
Do we also want to override .get_size, to flag (or round down) the
case where the size reported by the plugin is not a multiple of the
plugin's minsize, at which point those trailing bytes are inaccessbile
to the client?  Could be done on top.

Overall the series looks good, I'll leave it up to you if you think it
is ready to push now or if you want to adjust for one more round of
reviews, and meanwhile I can start working on my proposed tweaks to
compare on top to see how they look.

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

On Thu, Feb 17, 2022 at 02:36:48PM +0000, Richard W.M. Jones
wrote:
> This filter can add block size constraints to plugins which don't
> already support them.  It can also enforce an error policy for badly
> behaved clients which do not obey the block size constraints.
> ---
> +=item B<blocksize-error-policy=allow>
> +
> +=item B<blocksize-error-policy=error>
> +
> +If a client sends a request which is smaller than the permitted
> +minimum size or larger than the permitted maximum size, or not aligned
> +to the minimum size, C<blocksize-error-policy> chooses what the
filter
> +will do.  The default (and also nbdkit's default) is C<allow>
which
> +means pass the request through to the plugin.
> +
> +Use C<error> to return an EIO error back to the client.  The plugin
> +will not see the badly formed request in this case.
> +
> +
> +/* This function checks the error policy for all request functions below.
*/
> +static int
> +check_policy (nbdkit_next *next, const char *type,
> +              uint32_t count, uint64_t offset, int *err)
> +{
> +  uint32_t minimum, preferred, maximum;
> +
> +  if (error_policy == ALLOW)
> +    return 0;
> +
> +  /* Get the current block size constraints.  Note these are cached in
> +   * the backend so if they've already been computed then this simply
> +   * returns the cached values.  The plugin is only asked once per
> +   * connection.
> +   */
> +  errno = 0;
> +  if (next->block_size (next, &minimum, &preferred,
&maximum) == -1) {
> +    *err = errno ? : EIO;
> +    return -1;
> +  }
After re-reading libnbd's nbd_get_block_size(), I noticed something:

The NBD protocol spec recommends EINVAL (rather than EIO) for requests
that violate block size constraints.  Do we want to hardcode EINVAL,
or else tweak the error policy to allow the user to specify which
errno to provide?

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