Libguestfs - Aug 2019 - [nbdkit PATCH v2] filters: Stronger version match requirements

We documented our intent of only allowing a filter to run with the
same version of nbdkit it was compiled against, but up to now, were
not actually enforcing that - we had only been insisting on the looser
notion of a matching ._api_version, which doesn't help when we've
forgotten to bump that macro when making incompatible API/ABI changes
(see commit 6934d4c1).  However, we can't use .version (it was
documented as optional, so even though all our in-tree filters set it,
a theoretical out-of-tree filter could use NULL; and it has changed
offsets in the ABI during previous API changes, such as commit
ee61d232).  Thus, we have to bump the API version one final time; but
now, our API guaarantees enough stable ABI to check the version string
as part of our API sanity checking, so we are set to avoid future
maintenance snafus.

All in-tree filters are affected now that .version is no longer
available as an optional member (although it so happens that we are
setting the new mandatory ._version to the same value that they all
used).  And thanks to the previous patch adding nbdkit-version.h, even
a theoretical out-of-tree filter will get the correct version string
without having to maintain it by hand.

Note that the offset of ._version differs between 32- and 64-bit
platform ABIs; but on that front, we are safe: Rich and I tested (at
least on Linux where ELF executables encode ABI) that attempts to load
a 32-bit .so from 64-bit nbdkit, or vice versa, gracefully fail
dlopen() for being an incompatible ABI even before we get far enough
to worry about inspecting ._api_version or ._version.

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

This is what I ended up committing for filter API/ABI sanity checking
(a lot nicer than playing ABI games with the unused bytes in
._api_version).

 docs/nbdkit-filter.pod              |  7 -------
 server/filters.c                    | 20 +++++++++++++++-----
 include/nbdkit-filter.h             | 18 ++++++++++--------
 filters/blocksize/blocksize.c       |  1 -
 filters/cache/cache.c               |  1 -
 filters/cacheextents/cacheextents.c |  1 -
 filters/cow/cow.c                   |  1 -
 filters/delay/delay.c               |  1 -
 filters/error/error.c               |  1 -
 filters/fua/fua.c                   |  1 -
 filters/log/log.c                   |  1 -
 filters/nocache/nocache.c           |  1 -
 filters/noextents/noextents.c       |  1 -
 filters/noparallel/noparallel.c     |  1 -
 filters/nozero/nozero.c             |  1 -
 filters/offset/offset.c             |  1 -
 filters/partition/partition.c       |  1 -
 filters/rate/rate.c                 |  1 -
 filters/readahead/readahead.c       |  1 -
 filters/stats/stats.c               |  1 -
 filters/truncate/truncate.c         |  1 -
 filters/xz/xz.c                     |  1 -
 tests/test-layers-filter.c          |  1 -
 tests/test-cxx-filter.cpp           |  1 -
 24 files changed, 25 insertions(+), 41 deletions(-)

diff --git a/docs/nbdkit-filter.pod b/docs/nbdkit-filter.pod
index 0cd3e26a..4bf87295 100644
--- a/docs/nbdkit-filter.pod
+++ b/docs/nbdkit-filter.pod
@@ -178,13 +178,6 @@ methods.
 This field (a string) is required, and B<must> contain only ASCII
 alphanumeric characters and be unique amongst all filters.

-=head2 C<.version>
-
- const char *version;
-
-Filters may optionally set a version string which is displayed in help
-and debugging output.
-
 =head2 C<.longname>

  const char *longname;
diff --git a/server/filters.c b/server/filters.c
index c67676a3..0e10816f 100644
--- a/server/filters.c
+++ b/server/filters.c
@@ -102,7 +102,7 @@ filter_version (struct backend *b)
 {
   struct backend_filter *f = container_of (b, struct backend_filter, backend);

-  return f->filter.version;
+  return f->filter._version;
 }

 static void
@@ -730,14 +730,24 @@ filter_register (struct backend *next, size_t index, const
char *filename,
   }

   /* We do not provide API or ABI guarantees for filters, other than
-   * the ABI position of _api_version that will let us diagnose
-   * mismatch when the API changes.
+   * the ABI position and API contents of _api_version and _version to
+   * diagnose mismatch from the current nbdkit version.
    */
   if (filter->_api_version != NBDKIT_FILTER_API_VERSION) {
     fprintf (stderr,
              "%s: %s: filter is incompatible with this version of nbdkit
"
-             "(_api_version = %d)\n",
-             program_name, filename, filter->_api_version);
+             "(_api_version = %d, need %d)\n",
+             program_name, filename, filter->_api_version,
+             NBDKIT_FILTER_API_VERSION);
+    exit (EXIT_FAILURE);
+  }
+  if (filter->_version == NULL ||
+      strcmp (filter->_version, PACKAGE_VERSION) != 0) {
+    fprintf (stderr,
+             "%s: %s: filter is incompatible with this version of nbdkit
"
+             "(_version = %s, need %s)\n",
+             program_name, filename, filter->_version ?:
"<null>",
+             PACKAGE_VERSION);
     exit (EXIT_FAILURE);
   }

diff --git a/include/nbdkit-filter.h b/include/nbdkit-filter.h
index 4f8f19ab..8423644e 100644
--- a/include/nbdkit-filter.h
+++ b/include/nbdkit-filter.h
@@ -43,7 +43,7 @@
 extern "C" {
 #endif

-#define NBDKIT_FILTER_API_VERSION 5 /* Corresponding to v1.14 */
+#define NBDKIT_FILTER_API_VERSION 6 /* Corresponding to v1.16+ */

 struct nbdkit_extent {
   uint64_t offset;
@@ -93,20 +93,21 @@ struct nbdkit_next_ops {
 };

 struct nbdkit_filter {
-  /* Do not set this field directly; use NBDKIT_REGISTER_FILTER.
-   * It exists so that we can diagnose filters compiled against
-   * one version of the header with a runtime compiled against a
-   * different version.
+  /* Do not set these fields directly; use NBDKIT_REGISTER_FILTER.
+   * They exist so that we can diagnose filters compiled against one
+   * version of the header with a runtime compiled against a different
+   * version.  As of API version 6, _version is also part of the
+   * guaranteed ABI, so we no longer have to remember to bump API
+   * versions regardless of other API/ABI changes later in the struct.
    */
   int _api_version;
+  const char *_version;

   /* Because there is no ABI guarantee, new fields may be added where
-   * logically appropriate, as long as we correctly bump
-   * NBDKIT_FILTER_API_VERSION once per stable release.
+   * logically appropriate.
    */
   const char *name;
   const char *longname;
-  const char *version;
   const char *description;

   void (*load) (void);
@@ -179,6 +180,7 @@ struct nbdkit_filter {
   filter_init (void)                                                    \
   {                                                                     \
     (filter)._api_version = NBDKIT_FILTER_API_VERSION;                  \
+    (filter)._version = NBDKIT_VERSION_STRING;                          \
     return &(filter);                                                   \
   }

diff --git a/filters/blocksize/blocksize.c b/filters/blocksize/blocksize.c
index 0978887f..058c236b 100644
--- a/filters/blocksize/blocksize.c
+++ b/filters/blocksize/blocksize.c
@@ -399,7 +399,6 @@ blocksize_cache (struct nbdkit_next_ops *next_ops, void
*nxdata,
 static struct nbdkit_filter filter = {
   .name              = "blocksize",
   .longname          = "nbdkit blocksize filter",
-  .version           = PACKAGE_VERSION,
   .config            = blocksize_config,
   .config_complete   = blocksize_config_complete,
   .config_help       = blocksize_config_help,
diff --git a/filters/cache/cache.c b/filters/cache/cache.c
index b5dbccd2..fb852df9 100644
--- a/filters/cache/cache.c
+++ b/filters/cache/cache.c
@@ -615,7 +615,6 @@ cache_cache (struct nbdkit_next_ops *next_ops, void *nxdata,
 static struct nbdkit_filter filter = {
   .name              = "cache",
   .longname          = "nbdkit caching filter",
-  .version           = PACKAGE_VERSION,
   .load              = cache_load,
   .unload            = cache_unload,
   .config            = cache_config,
diff --git a/filters/cacheextents/cacheextents.c
b/filters/cacheextents/cacheextents.c
index b3828f41..e89fe580 100644
--- a/filters/cacheextents/cacheextents.c
+++ b/filters/cacheextents/cacheextents.c
@@ -192,7 +192,6 @@ cacheextents_zero (struct nbdkit_next_ops *next_ops, void
*nxdata,
 static struct nbdkit_filter filter = {
   .name              = "cacheextents",
   .longname          = "nbdkit cacheextents filter",
-  .version           = PACKAGE_VERSION,
   .unload            = cacheextents_unload,
   .pwrite            = cacheextents_pwrite,
   .trim              = cacheextents_trim,
diff --git a/filters/cow/cow.c b/filters/cow/cow.c
index 9d91d432..491d0009 100644
--- a/filters/cow/cow.c
+++ b/filters/cow/cow.c
@@ -482,7 +482,6 @@ cow_cache (struct nbdkit_next_ops *next_ops, void *nxdata,
 static struct nbdkit_filter filter = {
   .name              = "cow",
   .longname          = "nbdkit copy-on-write (COW) filter",
-  .version           = PACKAGE_VERSION,
   .load              = cow_load,
   .unload            = cow_unload,
   .open              = cow_open,
diff --git a/filters/delay/delay.c b/filters/delay/delay.c
index c92a12d7..6b13d459 100644
--- a/filters/delay/delay.c
+++ b/filters/delay/delay.c
@@ -266,7 +266,6 @@ delay_cache (struct nbdkit_next_ops *next_ops, void *nxdata,
 static struct nbdkit_filter filter = {
   .name              = "delay",
   .longname          = "nbdkit delay filter",
-  .version           = PACKAGE_VERSION,
   .config            = delay_config,
   .config_help       = delay_config_help,
   .pread             = delay_pread,
diff --git a/filters/error/error.c b/filters/error/error.c
index 968f2837..2d39b251 100644
--- a/filters/error/error.c
+++ b/filters/error/error.c
@@ -375,7 +375,6 @@ error_cache (struct nbdkit_next_ops *next_ops, void *nxdata,
 static struct nbdkit_filter filter = {
   .name              = "error",
   .longname          = "nbdkit error filter",
-  .version           = PACKAGE_VERSION,
   .load              = error_load,
   .unload            = error_unload,
   .config            = error_config,
diff --git a/filters/fua/fua.c b/filters/fua/fua.c
index 9d0e561e..83f7a1a7 100644
--- a/filters/fua/fua.c
+++ b/filters/fua/fua.c
@@ -233,7 +233,6 @@ fua_zero (struct nbdkit_next_ops *next_ops, void *nxdata,
 static struct nbdkit_filter filter = {
   .name              = "fua",
   .longname          = "nbdkit fua filter",
-  .version           = PACKAGE_VERSION,
   .config            = fua_config,
   .config_help       = fua_config_help,
   .prepare           = fua_prepare,
diff --git a/filters/log/log.c b/filters/log/log.c
index 7cf741e1..6d37d583 100644
--- a/filters/log/log.c
+++ b/filters/log/log.c
@@ -435,7 +435,6 @@ log_cache (struct nbdkit_next_ops *next_ops, void *nxdata,
 static struct nbdkit_filter filter = {
   .name              = "log",
   .longname          = "nbdkit log filter",
-  .version           = PACKAGE_VERSION,
   .config            = log_config,
   .config_complete   = log_config_complete,
   .config_help       = log_config_help,
diff --git a/filters/nocache/nocache.c b/filters/nocache/nocache.c
index a3f11984..7ddb84bc 100644
--- a/filters/nocache/nocache.c
+++ b/filters/nocache/nocache.c
@@ -101,7 +101,6 @@ nocache_cache (struct nbdkit_next_ops *next_ops, void
*nxdata,
 static struct nbdkit_filter filter = {
   .name              = "nocache",
   .longname          = "nbdkit nocache filter",
-  .version           = PACKAGE_VERSION,
   .config            = nocache_config,
   .config_help       = nocache_config_help,
   .can_cache         = nocache_can_cache,
diff --git a/filters/noextents/noextents.c b/filters/noextents/noextents.c
index e39723cd..e6ac33b2 100644
--- a/filters/noextents/noextents.c
+++ b/filters/noextents/noextents.c
@@ -44,7 +44,6 @@ noextents_can_extents (struct nbdkit_next_ops *next_ops, void
*nxdata,
 static struct nbdkit_filter filter = {
   .name              = "noextents",
   .longname          = "nbdkit noextents filter",
-  .version           = PACKAGE_VERSION,
   .can_extents       = noextents_can_extents,
 };

diff --git a/filters/noparallel/noparallel.c b/filters/noparallel/noparallel.c
index 057485fa..f9006c06 100644
--- a/filters/noparallel/noparallel.c
+++ b/filters/noparallel/noparallel.c
@@ -76,7 +76,6 @@ noparallel_thread_model (void)
 static struct nbdkit_filter filter = {
   .name              = "noparallel",
   .longname          = "nbdkit noparallel filter",
-  .version           = PACKAGE_VERSION,
   .config            = noparallel_config,
   .config_help       = noparallel_config_help,
   .thread_model      = noparallel_thread_model,
diff --git a/filters/nozero/nozero.c b/filters/nozero/nozero.c
index 964cce9f..9187ec33 100644
--- a/filters/nozero/nozero.c
+++ b/filters/nozero/nozero.c
@@ -139,7 +139,6 @@ nozero_zero (struct nbdkit_next_ops *next_ops, void *nxdata,
 static struct nbdkit_filter filter = {
   .name              = "nozero",
   .longname          = "nbdkit nozero filter",
-  .version           = PACKAGE_VERSION,
   .config            = nozero_config,
   .config_help       = nozero_config_help,
   .prepare           = nozero_prepare,
diff --git a/filters/offset/offset.c b/filters/offset/offset.c
index faae0328..7dc94b1d 100644
--- a/filters/offset/offset.c
+++ b/filters/offset/offset.c
@@ -175,7 +175,6 @@ offset_cache (struct nbdkit_next_ops *next_ops, void
*nxdata,
 static struct nbdkit_filter filter = {
   .name              = "offset",
   .longname          = "nbdkit offset filter",
-  .version           = PACKAGE_VERSION,
   .config            = offset_config,
   .config_help       = offset_config_help,
   .get_size          = offset_get_size,
diff --git a/filters/partition/partition.c b/filters/partition/partition.c
index b1b1945d..39132ce8 100644
--- a/filters/partition/partition.c
+++ b/filters/partition/partition.c
@@ -265,7 +265,6 @@ partition_cache (struct nbdkit_next_ops *next_ops, void
*nxdata,
 static struct nbdkit_filter filter = {
   .name              = "partition",
   .longname          = "nbdkit partition filter",
-  .version           = PACKAGE_VERSION,
   .config            = partition_config,
   .config_complete   = partition_config_complete,
   .config_help       = partition_config_help,
diff --git a/filters/rate/rate.c b/filters/rate/rate.c
index dca5e9fc..2b105f91 100644
--- a/filters/rate/rate.c
+++ b/filters/rate/rate.c
@@ -310,7 +310,6 @@ rate_pwrite (struct nbdkit_next_ops *next_ops, void *nxdata,
 static struct nbdkit_filter filter = {
   .name              = "rate",
   .longname          = "nbdkit rate filter",
-  .version           = PACKAGE_VERSION,
   .unload            = rate_unload,
   .config            = rate_config,
   .config_complete   = rate_config_complete,
diff --git a/filters/readahead/readahead.c b/filters/readahead/readahead.c
index b6c18096..3ec73e45 100644
--- a/filters/readahead/readahead.c
+++ b/filters/readahead/readahead.c
@@ -247,7 +247,6 @@ readahead_zero (struct nbdkit_next_ops *next_ops, void
*nxdata,
 static struct nbdkit_filter filter = {
   .name              = "readahead",
   .longname          = "nbdkit readahead filter",
-  .version           = PACKAGE_VERSION,
   .unload            = readahead_unload,
   .prepare           = readahead_prepare,
   .get_size          = readahead_get_size,
diff --git a/filters/stats/stats.c b/filters/stats/stats.c
index d8ff02bc..98282e26 100644
--- a/filters/stats/stats.c
+++ b/filters/stats/stats.c
@@ -290,7 +290,6 @@ stats_cache (struct nbdkit_next_ops *next_ops, void *nxdata,
 static struct nbdkit_filter filter = {
   .name              = "stats",
   .longname          = "nbdkit stats filter",
-  .version           = PACKAGE_VERSION,
   .unload            = stats_unload,
   .config            = stats_config,
   .config_complete   = stats_config_complete,
diff --git a/filters/truncate/truncate.c b/filters/truncate/truncate.c
index 93d8f074..ae1f8624 100644
--- a/filters/truncate/truncate.c
+++ b/filters/truncate/truncate.c
@@ -385,7 +385,6 @@ truncate_cache (struct nbdkit_next_ops *next_ops, void
*nxdata,
 static struct nbdkit_filter filter = {
   .name              = "truncate",
   .longname          = "nbdkit truncate filter",
-  .version           = PACKAGE_VERSION,
   .config            = truncate_config,
   .config_help       = truncate_config_help,
   .open              = truncate_open,
diff --git a/filters/xz/xz.c b/filters/xz/xz.c
index 51ac919f..78351859 100644
--- a/filters/xz/xz.c
+++ b/filters/xz/xz.c
@@ -254,7 +254,6 @@ static int xz_thread_model (void)
 static struct nbdkit_filter filter = {
   .name              = "xz",
   .longname          = "nbdkit XZ filter",
-  .version           = PACKAGE_VERSION,
   .config            = xz_config,
   .config_help       = xz_config_help,
   .thread_model      = xz_thread_model,
diff --git a/tests/test-layers-filter.c b/tests/test-layers-filter.c
index bd063bdf..465454c0 100644
--- a/tests/test-layers-filter.c
+++ b/tests/test-layers-filter.c
@@ -261,7 +261,6 @@ test_layers_filter_cache (struct nbdkit_next_ops *next_ops,
void *nxdata,

 static struct nbdkit_filter filter = {
   .name              = "testlayers" layer,
-  .version           = PACKAGE_VERSION,
   .load              = test_layers_filter_load,
   .unload            = test_layers_filter_unload,
   .config            = test_layers_filter_config,
diff --git a/tests/test-cxx-filter.cpp b/tests/test-cxx-filter.cpp
index ba5ff29e..57299a15 100644
--- a/tests/test-cxx-filter.cpp
+++ b/tests/test-cxx-filter.cpp
@@ -49,7 +49,6 @@ namespace {
   nbdkit_filter create_filter() {
     nbdkit_filter filter = nbdkit_filter ();
     filter.name = "cxxfilter";
-    filter.version = PACKAGE_VERSION;
     return filter;
   }
 }
-- 
2.21.0

On Fri, Aug 30, 2019 at 11:12:07AM -0500, Eric Blake
wrote:
> We documented our intent of only allowing a filter to run with the
> same version of nbdkit it was compiled against, but up to now, were
> not actually enforcing that - we had only been insisting on the looser
> notion of a matching ._api_version, which doesn't help when we've
> forgotten to bump that macro when making incompatible API/ABI changes
> (see commit 6934d4c1).  However, we can't use .version (it was
> documented as optional, so even though all our in-tree filters set it,
> a theoretical out-of-tree filter could use NULL; and it has changed
> offsets in the ABI during previous API changes, such as commit
> ee61d232).  Thus, we have to bump the API version one final time; but
> now, our API guaarantees enough stable ABI to check the version string
> as part of our API sanity checking, so we are set to avoid future
> maintenance snafus.
> 
> All in-tree filters are affected now that .version is no longer
> available as an optional member (although it so happens that we are
> setting the new mandatory ._version to the same value that they all
> used).  And thanks to the previous patch adding nbdkit-version.h, even
> a theoretical out-of-tree filter will get the correct version string
> without having to maintain it by hand.
> 
> Note that the offset of ._version differs between 32- and 64-bit
> platform ABIs; but on that front, we are safe: Rich and I tested (at
> least on Linux where ELF executables encode ABI) that attempts to load
> a 32-bit .so from 64-bit nbdkit, or vice versa, gracefully fail
> dlopen() for being an incompatible ABI even before we get far enough
> to worry about inspecting ._api_version or ._version.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
This all looks fine, ACK.

Thanks,

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