Libguestfs - Jan 2018 - [nbdkit PATCH 0/3] Add nozero filter

I still need to add testsuite coverage. Perhaps it might be easier
if I create a new '--filter=log logfile=foo' filter that produces
a log of which commands a client sent, then compare the log using
a known client that uses write_zeroes (qemu-io works well) both
with and without --filter=nozero to prove that the change in
advertisement changes the commands sent over the wire (that would
also test the log filter at the same time...).

I'll wait for a review on this series, particularly since patch 2
intentionally breaks filter ABI (more ABI/API breaks on the way
when I resubmit my FUA work; it would be nice to make all the API
breaks before v1.1.29, rather than releasing versions while things
are still in flux).

Note that even if we try hard to let newer nbdkit load an older
filter (which I did not do here), we absolutely cannot let older
nbdkit load a newer filter - while we have sizing limits to
guarantee we don't call beyond the end of an older filter's
smaller struct, there are no sizing limits in the other
direction to prevent a newer filter to know that it cannot call
beyond the end of our smaller nbdkit_next_ops.

Eric Blake (3):
  connections: Don't advertise TRIM on readonly connection
  filter: Add .can_zero/.can_fua overrides
  filters: Add nozero filter

 TODO                                    |  2 +-
 configure.ac                            |  3 +-
 docs/nbdkit-filter.pod                  | 30 +++++++++++-
 docs/nbdkit.pod                         |  1 +
 filters/Makefile.am                     |  1 +
 filters/nozero/Makefile.am              | 62 ++++++++++++++++++++++++
 filters/nozero/nbdkit-nozero-filter.pod | 84 +++++++++++++++++++++++++++++++++
 filters/nozero/nozero.c                 | 68 ++++++++++++++++++++++++++
 include/nbdkit-filter.h                 |  8 +++-
 src/connections.c                       | 45 +++++++++++++-----
 src/filters.c                           | 70 ++++++++++++++++++++++++---
 src/internal.h                          |  2 +
 src/plugins.c                           | 22 +++++++++
 13 files changed, 376 insertions(+), 22 deletions(-)
 create mode 100644 filters/nozero/Makefile.am
 create mode 100644 filters/nozero/nbdkit-nozero-filter.pod
 create mode 100644 filters/nozero/nozero.c

-- 
2.14.3

A client cannot attempt TRIM on a read-only connection (we already
filter it in connections.c with EROFS); as such, it makes no sense
to query the plugin's .can_trim nor to advertise TRIM support to
the client on such a connection, similar to how we do not
advertise WRITE_ZEROES.

FUA only has defined meanings on write operations, but the NBD
protocol allows us to advertise it on reads and to ignore clients
that send it on read (older qemu did so), so we can continue to
advertise it regardless of readonly state.  Likewise, while flush
is unlikely to make a difference on a read-only connection, there
is no reason why it shouldn't be advertised.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 src/connections.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/src/connections.c b/src/connections.c
index 8e110a5..75c2c2d 100644
--- a/src/connections.c
+++ b/src/connections.c
@@ -427,6 +427,14 @@ compute_eflags (struct connection *conn, uint16_t *flags)
   }
   if (!conn->readonly) {
     eflags |= NBD_FLAG_SEND_WRITE_ZEROES;
+
+    fl = backend->can_trim (backend, conn);
+    if (fl == -1)
+      return -1;
+    if (fl) {
+      eflags |= NBD_FLAG_SEND_TRIM;
+      conn->can_trim = 1;
+    }
   }

   fl = backend->can_flush (backend, conn);
@@ -445,14 +453,6 @@ compute_eflags (struct connection *conn, uint16_t *flags)
     conn->is_rotational = 1;
   }

-  fl = backend->can_trim (backend, conn);
-  if (fl == -1)
-    return -1;
-  if (fl) {
-    eflags |= NBD_FLAG_SEND_TRIM;
-    conn->can_trim = 1;
-  }
-
   *flags = eflags;
   return 0;
 }
-- 
2.14.3

While our plugin code always advertises WRITE_ZEROES on writable
connections (because we can emulate .zero by calling .pwrite),
and FUA support when .flush exists (because we can emulate the
persistence by calling .flush), it is conceivable that a filter
may want to explicitly avoid advertising particular bits.  (More
to the point, I plan on writing a 'nozero' filter that hides
WRITE_ZEROES support, at least for the purposes of timing
comparisons between server emulation and the client having to
send zeroes over the wire).  Furthermore, since we only have
to honor FUA support on write requests, we do not need to
advertise it on a readonly connection.

Wire up two new backend callbacks, as well as exposing them to the
filter interface (for now, we intentionally do not expose .can_zero
to the plugin interface, and later patches will worry about exposing
.can_fua as part of updating the plugin interface to support flags).

This change breaks filter API; a filter compiled against an earlier
nbdkit will not run against this version of nbdkit.  We haven't
promised ABI stability yet, in part because a filter compiled
against an older nbdkit needs to be aware of any new entry points
(and other upcoming changes, such as adding FUA flag support);
conversely, a filter compiled against a newer nbdkit is liable to
try to use new pointers in nbdkit_next_ops that are not part of
the current nbdkit, and while we have _struct_size to protect what
we call in the filter, we do not have a similar mechanism to
protect what the filter can call back in our code.

As such, this patch can keep the public interface logically grouped
by inserting members in the middle of the struct, rather than
worrying about having to stick additions at the end.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 docs/nbdkit-filter.pod  | 21 ++++++++++++++-
 include/nbdkit-filter.h |  8 +++++-
 src/connections.c       | 29 +++++++++++++++++---
 src/filters.c           | 70 ++++++++++++++++++++++++++++++++++++++++++++-----
 src/internal.h          |  2 ++
 src/plugins.c           | 22 ++++++++++++++++
 6 files changed, 140 insertions(+), 12 deletions(-)

diff --git a/docs/nbdkit-filter.pod b/docs/nbdkit-filter.pod
index be3c7a0..dbd4385 100644
--- a/docs/nbdkit-filter.pod
+++ b/docs/nbdkit-filter.pod
@@ -338,7 +338,26 @@ should call C<nbdkit_error> with an error message and
return C<-1>.
  int (*can_trim) (struct nbdkit_next_ops *next_ops, void *nxdata,
                   void *handle);

-These intercept the corresponding plugin methods.
+These intercept the corresponding plugin methods, and control feature
+bits advertised to the client.
+
+If there is an error, the callback should call C<nbdkit_error> with an
+error message and return C<-1>.
+
+=head2 C<.can_zero>
+
+=head2 C<.can_fua>
+
+ int (*can_zero) (struct nbdkit_next_ops *next_ops, void *nxdata,
+                  void *handle);
+ int (*can_fua) (struct nbdkit_next_ops *next_ops, void *nxdata,
+                 void *handle);
+
+These control feature bits that are advertised to the client.  These
+functions have no counterpart in plugins, because nbdkit can always
+emulate zero by using pwrite and emulate Forced Unit Access (FUA) by
+using flush; but a filter can implement these to force a bypass of the
+nbdkit fallbacks.

 If there is an error, the callback should call C<nbdkit_error> with an
 error message and return C<-1>.
diff --git a/include/nbdkit-filter.h b/include/nbdkit-filter.h
index 8ea9919..2ff2902 100644
--- a/include/nbdkit-filter.h
+++ b/include/nbdkit-filter.h
@@ -1,5 +1,5 @@
 /* nbdkit
- * Copyright (C) 2013-2017 Red Hat Inc.
+ * Copyright (C) 2013-2018 Red Hat Inc.
  * All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -57,6 +57,8 @@ struct nbdkit_next_ops {
   int (*can_flush) (void *nxdata);
   int (*is_rotational) (void *nxdata);
   int (*can_trim) (void *nxdata);
+  int (*can_zero) (void *nxdata);
+  int (*can_fua) (void *nxdata);

   int (*pread) (void *nxdata, void *buf, uint32_t count, uint64_t offset);
   int (*pwrite) (void *nxdata,
@@ -111,6 +113,10 @@ struct nbdkit_filter {
                         void *handle);
   int (*can_trim) (struct nbdkit_next_ops *next_ops, void *nxdata,
                    void *handle);
+  int (*can_zero) (struct nbdkit_next_ops *next_ops, void *nxdata,
+                   void *handle);
+  int (*can_fua) (struct nbdkit_next_ops *next_ops, void *nxdata,
+                  void *handle);

   int (*pread) (struct nbdkit_next_ops *next_ops, void *nxdata,
                 void *handle, void *buf, uint32_t count, uint64_t offset);
diff --git a/src/connections.c b/src/connections.c
index 75c2c2d..5a7b2d8 100644
--- a/src/connections.c
+++ b/src/connections.c
@@ -80,6 +80,8 @@ struct connection {
   int can_flush;
   int is_rotational;
   int can_trim;
+  int can_zero;
+  int can_fua;
   int using_tls;

   int sockin, sockout;
@@ -426,7 +428,13 @@ compute_eflags (struct connection *conn, uint16_t *flags)
     conn->readonly = 1;
   }
   if (!conn->readonly) {
-    eflags |= NBD_FLAG_SEND_WRITE_ZEROES;
+    fl = backend->can_zero (backend, conn);
+    if (fl == -1)
+      return -1;
+    if (fl) {
+      eflags |= NBD_FLAG_SEND_WRITE_ZEROES;
+      conn->can_zero = 1;
+    }

     fl = backend->can_trim (backend, conn);
     if (fl == -1)
@@ -435,13 +443,21 @@ compute_eflags (struct connection *conn, uint16_t *flags)
       eflags |= NBD_FLAG_SEND_TRIM;
       conn->can_trim = 1;
     }
+
+    fl = backend->can_fua (backend, conn);
+    if (fl == -1)
+      return -1;
+    if (fl) {
+      eflags |= NBD_FLAG_SEND_FUA;
+      conn->can_fua = 1;
+    }
   }

   fl = backend->can_flush (backend, conn);
   if (fl == -1)
     return -1;
   if (fl) {
-    eflags |= NBD_FLAG_SEND_FLUSH | NBD_FLAG_SEND_FUA;
+    eflags |= NBD_FLAG_SEND_FLUSH;
     conn->can_flush = 1;
   }

@@ -880,7 +896,7 @@ validate_request (struct connection *conn,
     *error = EINVAL;
     return false;
   }
-  if (!conn->can_flush && (flags & NBD_CMD_FLAG_FUA)) {
+  if (!conn->can_fua && (flags & NBD_CMD_FLAG_FUA)) {
     nbdkit_error ("invalid request: FUA flag not supported");
     *error = EINVAL;
     return false;
@@ -909,6 +925,13 @@ validate_request (struct connection *conn,
     return false;
   }

+  /* Zero allowed? */
+  if (!conn->can_zero && cmd == NBD_CMD_WRITE_ZEROES) {
+    nbdkit_error ("invalid request: write zeroes operation not
supported");
+    *error = EINVAL;
+    return false;
+  }
+
   return true;                     /* Command validates. */
 }

diff --git a/src/filters.c b/src/filters.c
index 2804cba..87c6e97 100644
--- a/src/filters.c
+++ b/src/filters.c
@@ -297,6 +297,20 @@ next_can_trim (void *nxdata)
   return b_conn->b->can_trim (b_conn->b, b_conn->conn);
 }

+static int
+next_can_zero (void *nxdata)
+{
+  struct b_conn *b_conn = nxdata;
+  return b_conn->b->can_zero (b_conn->b, b_conn->conn);
+}
+
+static int
+next_can_fua (void *nxdata)
+{
+  struct b_conn *b_conn = nxdata;
+  return b_conn->b->can_fua (b_conn->b, b_conn->conn);
+}
+
 static int
 next_pread (void *nxdata, void *buf, uint32_t count, uint64_t offset)
 {
@@ -348,6 +362,8 @@ static struct nbdkit_next_ops next_ops = {
   .flush = next_flush,
   .trim = next_trim,
   .zero = next_zero,
+  .can_zero = next_can_zero,
+  .can_fua = next_can_fua,
 };

 static int
@@ -455,6 +471,36 @@ filter_can_trim (struct backend *b, struct connection
*conn)
     return f->backend.next->can_trim (f->backend.next, conn);
 }

+static int
+filter_can_zero (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 ("can_zero");
+
+  if (f->filter.can_zero)
+    return f->filter.can_zero (&next_ops, &nxdata, handle);
+  else
+    return f->backend.next->can_zero (f->backend.next, conn);
+}
+
+static int
+filter_can_fua (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 ("can_fua");
+
+  if (f->filter.can_fua)
+    return f->filter.can_fua (&next_ops, &nxdata, handle);
+  else
+    return f->backend.next->can_fua (f->backend.next, conn);
+}
+
 static int
 filter_pread (struct backend *b, struct connection *conn,
               void *buf, uint32_t count, uint64_t offset,
@@ -582,6 +628,8 @@ static struct backend filter_functions = {
   .flush = filter_flush,
   .trim = filter_trim,
   .zero = filter_zero,
+  .can_zero = filter_can_zero,
+  .can_fua = filter_can_fua,
 };

 /* Register and load a filter. */
@@ -626,15 +674,23 @@ filter_register (struct backend *next, size_t index, const
char *filename,
     exit (EXIT_FAILURE);
   }

-  /* Since the filter might be much older than the current version of
-   * nbdkit, only copy up to the self-declared _struct_size of the
-   * filter and zero out the rest.  If the filter is much newer then
-   * we'll only call the "old" fields.
+  /* Providing a stable filter ABI is much harder than a stable plugin
+   * ABI.  Any newer filter compiled against a larger struct
+   * nbdkit_next_ops could potentially dereference fields we did not
+   * provide; and any older filter compiled against a smaller struct
+   * nbdkit_filter may miss intercepting functions that are vital to
+   * avoid corrupting the client-visible data.  So for now, we insist
+   * on an exact match in _struct_size, and promise that
+   * nbdkit_next_ops won't change size without also changing
+   * _struct_size.
    */
   size = sizeof f->filter;      /* our struct */
-  memset (&f->filter, 0, size);
-  if (size > filter->_struct_size)
-    size = filter->_struct_size;
+  if (filter->_struct_size != size) {
+    fprintf (stderr,
+             "%s: %s: filter compiled against incompatible nbdkit
version\n",
+             program_name, f->filename);
+    exit (EXIT_FAILURE);
+  }
   memcpy (&f->filter, filter, size);

   /* Only filter.name is required. */
diff --git a/src/internal.h b/src/internal.h
index 3cbfde5..2f03279 100644
--- a/src/internal.h
+++ b/src/internal.h
@@ -191,6 +191,8 @@ struct backend {
   int (*flush) (struct backend *, struct connection *conn, uint32_t flags);
   int (*trim) (struct backend *, struct connection *conn, uint32_t count,
uint64_t offset, uint32_t flags);
   int (*zero) (struct backend *, struct connection *conn, uint32_t count,
uint64_t offset, uint32_t flags);
+  int (*can_zero) (struct backend *, struct connection *conn);
+  int (*can_fua) (struct backend *, struct connection *conn);
 };

 /* plugins.c */
diff --git a/src/plugins.c b/src/plugins.c
index dba3e24..3468b6d 100644
--- a/src/plugins.c
+++ b/src/plugins.c
@@ -358,6 +358,26 @@ plugin_can_trim (struct backend *b, struct connection
*conn)
     return p->plugin.trim != NULL;
 }

+static int
+plugin_can_zero (struct backend *b, struct connection *conn)
+{
+  debug ("can_zero");
+
+  // We always allow .zero to fall back to .write, so plugins don't
+  // need to override this
+  return plugin_can_write (b, conn);
+}
+
+static int
+plugin_can_fua (struct backend *b, struct connection *conn)
+{
+  debug ("can_fua");
+
+  // TODO - wire FUA flag support into plugins. Until then, this copies
+  // can_flush, since that's how we emulate FUA
+  return plugin_can_flush (b, conn);
+}
+
 static int
 plugin_pread (struct backend *b, struct connection *conn,
               void *buf, uint32_t count, uint64_t offset, uint32_t flags)
@@ -535,6 +555,8 @@ static struct backend plugin_functions = {
   .flush = plugin_flush,
   .trim = plugin_trim,
   .zero = plugin_zero,
+  .can_zero = plugin_can_zero,
+  .can_fua = plugin_can_fua,
 };

 /* Register and load a plugin. */
-- 
2.14.3

Sometimes, it's nice to see what a difference it makes in timing
or in destination file size when sparse handling is enabled or
disabled.  Add a new filter that makes it trivial to disable
write zero support.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 TODO                                    |  2 +-
 configure.ac                            |  3 +-
 docs/nbdkit-filter.pod                  |  9 ++++
 docs/nbdkit.pod                         |  1 +
 filters/Makefile.am                     |  1 +
 filters/nozero/Makefile.am              | 62 ++++++++++++++++++++++++
 filters/nozero/nbdkit-nozero-filter.pod | 84 +++++++++++++++++++++++++++++++++
 filters/nozero/nozero.c                 | 68 ++++++++++++++++++++++++++
 8 files changed, 228 insertions(+), 2 deletions(-)
 create mode 100644 filters/nozero/Makefile.am
 create mode 100644 filters/nozero/nbdkit-nozero-filter.pod
 create mode 100644 filters/nozero/nozero.c

diff --git a/TODO b/TODO
index f6ab20c..eb11fc0 100644
--- a/TODO
+++ b/TODO
@@ -65,7 +65,7 @@ Suggestions for filters
 -----------------------

 * injecting artificial errors or otherwise masking plugin features
-  (such as hiding zero support) for testing clients
+  for testing clients (see 'nozero' filter for example)

 * logging all client commands

diff --git a/configure.ac b/configure.ac
index 6025ce0..dc7fc76 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1,5 +1,5 @@
 # nbdkit
-# Copyright (C) 2013-2017 Red Hat Inc.
+# Copyright (C) 2013-2018 Red Hat Inc.
 # All rights reserved.
 #
 # Redistribution and use in source and binary forms, with or without
@@ -516,6 +516,7 @@ AC_CONFIG_FILES([Makefile
                  filters/cache/Makefile
                  filters/cow/Makefile
                  filters/delay/Makefile
+		 filters/nozero/Makefile
                  filters/offset/Makefile
                  filters/partition/Makefile
                  src/Makefile
diff --git a/docs/nbdkit-filter.pod b/docs/nbdkit-filter.pod
index dbd4385..d0d9042 100644
--- a/docs/nbdkit-filter.pod
+++ b/docs/nbdkit-filter.pod
@@ -487,6 +487,15 @@ You can also run pkg-config/pkgconf directly, for example:
 L<nbdkit(1)>,
 L<nbdkit-plugin(1)>.

+Filters:
+
+L<nbdkit-cache-filter(1)>,
+L<nbdkit-cow-filter(1)>,
+L<nbdkit-delay-filter(1)>,
+L<nbdkit-nozero-filter(1)>,
+L<nbdkit-offset-filter(1)>,
+L<nbdkit-partition-filter(1)>.
+
 =head1 AUTHORS

 Richard W.M. Jones
diff --git a/docs/nbdkit.pod b/docs/nbdkit.pod
index 1167245..d66bf7c 100644
--- a/docs/nbdkit.pod
+++ b/docs/nbdkit.pod
@@ -920,6 +920,7 @@ Filters:
 L<nbdkit-cache-filter(1)>,
 L<nbdkit-cow-filter(1)>,
 L<nbdkit-delay-filter(1)>,
+L<nbdkit-nozero-filter(1)>,
 L<nbdkit-offset-filter(1)>,
 L<nbdkit-partition-filter(1)>.

diff --git a/filters/Makefile.am b/filters/Makefile.am
index 9996d77..0a3ec0e 100644
--- a/filters/Makefile.am
+++ b/filters/Makefile.am
@@ -34,5 +34,6 @@ SUBDIRS = \
 	cache \
 	cow \
 	delay \
+	nozero \
 	offset \
 	partition
diff --git a/filters/nozero/Makefile.am b/filters/nozero/Makefile.am
new file mode 100644
index 0000000..1c66ed8
--- /dev/null
+++ b/filters/nozero/Makefile.am
@@ -0,0 +1,62 @@
+# nbdkit
+# Copyright (C) 2018 Red Hat Inc.
+# All rights reserved.
+#
+# 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.
+
+EXTRA_DIST = nbdkit-nozero-filter.pod
+
+CLEANFILES = *~
+
+filterdir = $(libdir)/nbdkit/filters
+
+filter_LTLIBRARIES = nbdkit-nozero-filter.la
+
+nbdkit_nozero_filter_la_SOURCES = \
+	nozero.c \
+	$(top_srcdir)/include/nbdkit-filter.h
+
+nbdkit_nozero_filter_la_CPPFLAGS = \
+	-I$(top_srcdir)/include
+nbdkit_nozero_filter_la_CFLAGS = \
+	$(WARNINGS_CFLAGS)
+nbdkit_nozero_filter_la_LDFLAGS = \
+	-module -avoid-version -shared
+
+if HAVE_POD2MAN
+
+man_MANS = nbdkit-nozero-filter.1
+CLEANFILES += $(man_MANS)
+
+nbdkit-nozero-filter.1: nbdkit-nozero-filter.pod
+	$(POD2MAN) $(POD2MAN_ARGS) --section=1 --name=`basename $@ .1` $< $@.t
&& \
+	if grep 'POD ERROR' $@.t; then rm $@.t; exit 1; fi && \
+	mv $@.t $@
+
+endif
diff --git a/filters/nozero/nbdkit-nozero-filter.pod
b/filters/nozero/nbdkit-nozero-filter.pod
new file mode 100644
index 0000000..dd89767
--- /dev/null
+++ b/filters/nozero/nbdkit-nozero-filter.pod
@@ -0,0 +1,84 @@
+=encoding utf8
+
+=head1 NAME
+
+nbdkit-nozero-filter - nbdkit nozero filter
+
+=head1 SYNOPSIS
+
+ nbdkit --filter=nozero plugin [plugin-args...]
+
+=head1 DESCRIPTION
+
+C<nbdkit-nozero-filter> is a filter that intentionally disables
+efficient handling of sparse file holes (ranges of all-zero bytes)
+across the NBD protocol.  It is mainly useful for evaluating timing
+differences between naive vs. sparse-aware connections, and for
+testing client or server fallbacks.
+
+=head1 PARAMETERS
+
+There are no parameters specific to nbdkit-nozero-filter.  Any
+parameters are passed through to and processed by the underlying
+plugin in the normal way.
+
+=head1 EXAMPLES
+
+Serve the file F<disk.img>, but force the client to write zeroes
+explicitly rather than with C<NBD_CMD_WRITE_ZEROES>:
+
+ nbdkit --filter=nozero file file=disk.img
+
+=head1 SEE ALSO
+
+L<nbdkit(1)>,
+L<nbdkit-file-plugin(1)>,
+L<nbdkit-filter(3)>.
+
+=head1 AUTHORS
+
+Eric Blake
+
+=head1 COPYRIGHT
+
+Copyright (C) 2018 Red Hat Inc.
+
+=head1 LICENSE
+
+Redistribution and use in source and binary forms, with or without
+modification, are permitted provided that the following conditions are
+met:
+
+=over 4
+
+=item *
+
+Redistributions of source code must retain the above copyright
+notice, this list of conditions and the following disclaimer.
+
+=item *
+
+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.
+
+=item *
+
+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.
+
+=back
+
+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.
diff --git a/filters/nozero/nozero.c b/filters/nozero/nozero.c
new file mode 100644
index 0000000..676164e
--- /dev/null
+++ b/filters/nozero/nozero.c
@@ -0,0 +1,68 @@
+/* nbdkit
+ * Copyright (C) 2018 Red Hat Inc.
+ * All rights reserved.
+ *
+ * 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 <string.h>
+
+#include <nbdkit-filter.h>
+
+#define THREAD_MODEL NBDKIT_THREAD_MODEL_PARALLEL
+
+/* Disable WRITE_ZEROES. */
+static int
+nozero_can_zero (struct nbdkit_next_ops *next_ops, void *nxdata, void *handle)
+{
+  return 0;
+}
+
+/* Should never be reached (nbdkit should have already flagged a bad client).
*/
+static int
+nozero_zero (struct nbdkit_next_ops *next_ops, void *nxdata,
+             void *handle, uint32_t count, uint64_t offs, int may_trim)
+{
+  abort ();
+}
+
+static struct nbdkit_filter filter = {
+  .name              = "nozero",
+  .longname          = "nbdkit nozero filter",
+  .version           = PACKAGE_VERSION,
+  .can_zero          = nozero_can_zero,
+  .zero              = nozero_zero,
+};
+
+NBDKIT_REGISTER_FILTER(filter)
-- 
2.14.3

On Tue, Jan 23, 2018 at 10:10:12PM -0600, Eric Blake
wrote:
> I still need to add testsuite coverage. Perhaps it might be easier
> if I create a new '--filter=log logfile=foo' filter that produces
> a log of which commands a client sent, then compare the log using
> a known client that uses write_zeroes (qemu-io works well) both
> with and without --filter=nozero to prove that the change in
> advertisement changes the commands sent over the wire (that would
> also test the log filter at the same time...).
> 
> I'll wait for a review on this series, particularly since patch 2
> intentionally breaks filter ABI (more ABI/API breaks on the way
> when I resubmit my FUA work; it would be nice to make all the API
> breaks before v1.1.29, rather than releasing versions while things
> are still in flux).
> 
> Note that even if we try hard to let newer nbdkit load an older
> filter (which I did not do here), we absolutely cannot let older
> nbdkit load a newer filter - while we have sizing limits to
> guarantee we don't call beyond the end of an older filter's
> smaller struct, there are no sizing limits in the other
> direction to prevent a newer filter to know that it cannot call
> beyond the end of our smaller nbdkit_next_ops.
Hmm, yes, this is a good point that I didn't think about.

Probably we're going to have to forget about the filter._api_version
and filter._struct_size fields and instead encode the version of
nbdkit it was compiled against and check that rigorously.  It seems
like maintaining a filter ABI is hopeless.

Rich.
> Eric Blake (3):
>   connections: Don't advertise TRIM on readonly connection
>   filter: Add .can_zero/.can_fua overrides
>   filters: Add nozero filter
> 
>  TODO                                    |  2 +-
>  configure.ac                            |  3 +-
>  docs/nbdkit-filter.pod                  | 30 +++++++++++-
>  docs/nbdkit.pod                         |  1 +
>  filters/Makefile.am                     |  1 +
>  filters/nozero/Makefile.am              | 62 ++++++++++++++++++++++++
>  filters/nozero/nbdkit-nozero-filter.pod | 84
+++++++++++++++++++++++++++++++++
>  filters/nozero/nozero.c                 | 68 ++++++++++++++++++++++++++
>  include/nbdkit-filter.h                 |  8 +++-
>  src/connections.c                       | 45 +++++++++++++-----
>  src/filters.c                           | 70 ++++++++++++++++++++++++---
>  src/internal.h                          |  2 +
>  src/plugins.c                           | 22 +++++++++
>  13 files changed, 376 insertions(+), 22 deletions(-)
>  create mode 100644 filters/nozero/Makefile.am
>  create mode 100644 filters/nozero/nbdkit-nozero-filter.pod
>  create mode 100644 filters/nozero/nozero.c
> 
> -- 
> 2.14.3
> 
> _______________________________________________
> Libguestfs mailing list
> Libguestfs@redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs
-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

On Tue, Jan 23, 2018 at 10:10:13PM -0600, Eric Blake
wrote:
> A client cannot attempt TRIM on a read-only connection (we already
> filter it in connections.c with EROFS); as such, it makes no sense
> to query the plugin's .can_trim nor to advertise TRIM support to
> the client on such a connection, similar to how we do not
> advertise WRITE_ZEROES.
> 
> FUA only has defined meanings on write operations, but the NBD
> protocol allows us to advertise it on reads and to ignore clients
> that send it on read (older qemu did so), so we can continue to
> advertise it regardless of readonly state.  Likewise, while flush
> is unlikely to make a difference on a read-only connection, there
> is no reason why it shouldn't be advertised.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  src/connections.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/src/connections.c b/src/connections.c
> index 8e110a5..75c2c2d 100644
> --- a/src/connections.c
> +++ b/src/connections.c
> @@ -427,6 +427,14 @@ compute_eflags (struct connection *conn, uint16_t
*flags)
>    }
>    if (!conn->readonly) {
>      eflags |= NBD_FLAG_SEND_WRITE_ZEROES;
> +
> +    fl = backend->can_trim (backend, conn);
> +    if (fl == -1)
> +      return -1;
> +    if (fl) {
> +      eflags |= NBD_FLAG_SEND_TRIM;
> +      conn->can_trim = 1;
> +    }
>    }
> 
>    fl = backend->can_flush (backend, conn);
> @@ -445,14 +453,6 @@ compute_eflags (struct connection *conn, uint16_t
*flags)
>      conn->is_rotational = 1;
>    }
> 
> -  fl = backend->can_trim (backend, conn);
> -  if (fl == -1)
> -    return -1;
> -  if (fl) {
> -    eflags |= NBD_FLAG_SEND_TRIM;
> -    conn->can_trim = 1;
> -  }
> -
>    *flags = eflags;
>    return 0;
>  }
> -- 
Simple & obvious, ACK.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

On Tue, Jan 23, 2018 at 10:10:14PM -0600, Eric Blake
wrote:
> @@ -348,6 +362,8 @@ static struct nbdkit_next_ops next_ops = {
>    .flush = next_flush,
>    .trim = next_trim,
>    .zero = next_zero,
> +  .can_zero = next_can_zero,
> +  .can_fua = next_can_fua,
Not that it makes a functional difference but for ease of reading the
code maybe we should initialize these in the same order as they appear
in the struct definition?
> @@ -582,6 +628,8 @@ static struct backend filter_functions = {
>    .flush = filter_flush,
>    .trim = filter_trim,
>    .zero = filter_zero,
> +  .can_zero = filter_can_zero,
> +  .can_fua = filter_can_fua,
Since the backend struct is internal, we can put the can_* functions
next to the others.  (Also same comments about the change to
src/internal.h)
> 
>  /* Register and load a filter. */
> @@ -626,15 +674,23 @@ filter_register (struct backend *next, size_t index,
const char *filename,
>      exit (EXIT_FAILURE);
>    }
> 
> -  /* Since the filter might be much older than the current version of
> -   * nbdkit, only copy up to the self-declared _struct_size of the
> -   * filter and zero out the rest.  If the filter is much newer then
> -   * we'll only call the "old" fields.
> +  /* Providing a stable filter ABI is much harder than a stable plugin
> +   * ABI.  Any newer filter compiled against a larger struct
> +   * nbdkit_next_ops could potentially dereference fields we did not
> +   * provide; and any older filter compiled against a smaller struct
> +   * nbdkit_filter may miss intercepting functions that are vital to
> +   * avoid corrupting the client-visible data.  So for now, we insist
> +   * on an exact match in _struct_size, and promise that
> +   * nbdkit_next_ops won't change size without also changing
> +   * _struct_size.
>     */
>    size = sizeof f->filter;      /* our struct */
> -  memset (&f->filter, 0, size);
> -  if (size > filter->_struct_size)
> -    size = filter->_struct_size;
> +  if (filter->_struct_size != size) {
> +    fprintf (stderr,
> +             "%s: %s: filter compiled against incompatible nbdkit
version\n",
> +             program_name, f->filename);
> +    exit (EXIT_FAILURE);
> +  }
>    memcpy (&f->filter, filter, size);
Can we put this into a separate commit?
> diff --git a/src/internal.h b/src/internal.h
> index 3cbfde5..2f03279 100644
> --- a/src/internal.h
> +++ b/src/internal.h
> @@ -191,6 +191,8 @@ struct backend {
>    int (*flush) (struct backend *, struct connection *conn, uint32_t
flags);
>    int (*trim) (struct backend *, struct connection *conn, uint32_t count,
uint64_t offset, uint32_t flags);
>    int (*zero) (struct backend *, struct connection *conn, uint32_t count,
uint64_t offset, uint32_t flags);
> +  int (*can_zero) (struct backend *, struct connection *conn);
> +  int (*can_fua) (struct backend *, struct connection *conn);
See above.
> 
>  /* plugins.c */
> diff --git a/src/plugins.c b/src/plugins.c
> index dba3e24..3468b6d 100644
> --- a/src/plugins.c
> +++ b/src/plugins.c
> @@ -358,6 +358,26 @@ plugin_can_trim (struct backend *b, struct connection
*conn)
>      return p->plugin.trim != NULL;
>  }
> 
> +static int
> +plugin_can_zero (struct backend *b, struct connection *conn)
> +{
> +  debug ("can_zero");
> +
> +  // We always allow .zero to fall back to .write, so plugins don't
> +  // need to override this
> +  return plugin_can_write (b, conn);
> +}
> +
> +static int
> +plugin_can_fua (struct backend *b, struct connection *conn)
> +{
> +  debug ("can_fua");
> +
> +  // TODO - wire FUA flag support into plugins. Until then, this copies
> +  // can_flush, since that's how we emulate FUA
> +  return plugin_can_flush (b, conn);
> +}
> +
>  static int
>  plugin_pread (struct backend *b, struct connection *conn,
>                void *buf, uint32_t count, uint64_t offset, uint32_t flags)
> @@ -535,6 +555,8 @@ static struct backend plugin_functions = {
>    .flush = plugin_flush,
>    .trim = plugin_trim,
>    .zero = plugin_zero,
> +  .can_zero = plugin_can_zero,
> +  .can_fua = plugin_can_fua,
See above.

ACK with changes suggested.

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 Tue, Jan 23, 2018 at 10:10:15PM -0600, Eric Blake
wrote:
> Sometimes, it's nice to see what a difference it makes in timing
> or in destination file size when sparse handling is enabled or
> disabled.  Add a new filter that makes it trivial to disable
> write zero support.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
This one looks fine, ACK.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

On 01/24/2018 08:18 AM, Richard W.M. Jones wrote:
>> Note that even if we try hard to let newer nbdkit load an older
>> filter (which I did not do here), we absolutely cannot let older
>> nbdkit load a newer filter - while we have sizing limits to
>> guarantee we don't call beyond the end of an older filter's
>> smaller struct, there are no sizing limits in the other
>> direction to prevent a newer filter to know that it cannot call
>> beyond the end of our smaller nbdkit_next_ops.
> 
> Hmm, yes, this is a good point that I didn't think about.
> 
> Probably we're going to have to forget about the filter._api_version
> and filter._struct_size fields and instead encode the version of
> nbdkit it was compiled against and check that rigorously.  It seems
> like maintaining a filter ABI is hopeless.
About the only thing I could think of that MIGHT let us preserve ABI
from future filter to older nbdkit is if instead of using
next_ops.foo(), we add functions named next_foo().  nbdkit-filter.h
would then define weak-linkage functions that can only error out, but
which get overridden when available by strong linkage functions in
nbdkit proper.  Then a newer filter run against an older nbdkit will
call the weak-linkage function and see a proper failure in trying to
call the next function, while a filter run against the current nbdkit
will call the strong-linkage variant that does the intended passthrough
to the next layer.  But that's a lot of effort (and a one-time API
break) to set up something that will allow a stable ABI from that point
on, even if we didn't have the issue that running a newer plugin against
an older nbdkit may not be a reasonable goal (my FUA patches already
demonstrated that having new nbdkit run an old plugin is easy, but
having an old nbdkit run a new plugin may not work if the new plugin
only provides the new API callbacks and nbdkit-plugin.h doesn't provide
fallback shims in the old API callbacks).

As requested on 2/3, I'll factor out the ABI check into a separate
patch, and go with your suggestion that a simple version check is easier
than using a struct size check - which does indeed cement us into a
filter must be re-compiled in lockstep with nbdkit (we may still choose
to relax that later, but it's always better to start conservative).

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