Libguestfs - Feb 2020 - Re: [nbdkit PATCH] filters: Remove most next_* wrappers

With our recent cleanups to nxdata, the only remaining difference
between functions like backend_open() and next_open() was the
signature (one used void*, the other struct backend *); the API is
compatible.  All of our filters are in-tree, and we don't promise
API/ABI stability, but it is still a lot of files to touch, so the
simplest solution to avoid the redundant hop through wrapper functions
is to change our header to compile with a typedef, which is either
'struct backend *' for internal code or 'void *' for filters. 
With
this in place, we can now delete most of the next_* wrappers, by
pointing to corresponding backend_* functions instead; the few
exceptions are .config and .config_complete (the public API returns an
integer, but the backend code exits on failure and thus returns void),
and .preconnect (we don't have a backend_preconnect function).

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

The alternative to the #ifdef'd typedef is to touch all of the
filters/*.c files to change 'void *nxdata' to 'struct backend
*nxdata'.

 include/nbdkit-filter.h | 119 ++++++++++++-----------
 server/filters.c        | 208 +++++-----------------------------------
 server/internal.h       |   3 +-
 3 files changed, 89 insertions(+), 241 deletions(-)

diff --git a/include/nbdkit-filter.h b/include/nbdkit-filter.h
index f7705d67..50b3d55a 100644
--- a/include/nbdkit-filter.h
+++ b/include/nbdkit-filter.h
@@ -1,5 +1,5 @@
 /* nbdkit
- * Copyright (C) 2013-2019 Red Hat Inc.
+ * Copyright (C) 2013-2020 Red Hat Inc.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
@@ -49,47 +49,56 @@ extern "C" {
 #define NBDKIT_ZERO_EMULATE  1
 #define NBDKIT_ZERO_NATIVE   2

+#ifdef NBDKIT_INTERNAL
+/* Opaque type encapsulating all information needed for calling into
+ * the next filter or plugin.
+ */
+typedef struct backend backend;
+#else
+typedef void backend;
+#endif
+
 /* Next ops. */
-typedef int nbdkit_next_config (void *nxdata,
+typedef int nbdkit_next_config (backend *nxdata,
                                 const char *key, const char *value);
-typedef int nbdkit_next_config_complete (void *nxdata);
-typedef int nbdkit_next_preconnect (void *nxdata, int readonly);
-typedef int nbdkit_next_open (void *nxdata, int readonly);
+typedef int nbdkit_next_config_complete (backend *nxdata);
+typedef int nbdkit_next_preconnect (backend *nxdata, int readonly);
+typedef int nbdkit_next_open (backend *nxdata, int readonly);

 struct nbdkit_next_ops {
   /* Performs close + open on the underlying chain.
    * Used by the retry filter.
    */
-  int (*reopen) (void *nxdata, int readonly);
+  int (*reopen) (backend *nxdata, int readonly);

   /* The rest of the next ops are the same as normal plugin operations. */
-  int64_t (*get_size) (void *nxdata);
-
-  int (*can_write) (void *nxdata);
-  int (*can_flush) (void *nxdata);
-  int (*is_rotational) (void *nxdata);
-  int (*can_trim) (void *nxdata);
-  int (*can_zero) (void *nxdata);
-  int (*can_fast_zero) (void *nxdata);
-  int (*can_extents) (void *nxdata);
-  int (*can_fua) (void *nxdata);
-  int (*can_multi_conn) (void *nxdata);
-  int (*can_cache) (void *nxdata);
-
-  int (*pread) (void *nxdata, void *buf, uint32_t count, uint64_t offset,
+  int64_t (*get_size) (backend *nxdata);
+
+  int (*can_write) (backend *nxdata);
+  int (*can_flush) (backend *nxdata);
+  int (*is_rotational) (backend *nxdata);
+  int (*can_trim) (backend *nxdata);
+  int (*can_zero) (backend *nxdata);
+  int (*can_fast_zero) (backend *nxdata);
+  int (*can_extents) (backend *nxdata);
+  int (*can_fua) (backend *nxdata);
+  int (*can_multi_conn) (backend *nxdata);
+  int (*can_cache) (backend *nxdata);
+
+  int (*pread) (backend *nxdata, void *buf, uint32_t count, uint64_t offset,
                 uint32_t flags, int *err);
-  int (*pwrite) (void *nxdata,
+  int (*pwrite) (backend *nxdata,
                  const void *buf, uint32_t count, uint64_t offset,
                  uint32_t flags, int *err);
-  int (*flush) (void *nxdata, uint32_t flags, int *err);
-  int (*trim) (void *nxdata, uint32_t count, uint64_t offset, uint32_t flags,
-               int *err);
-  int (*zero) (void *nxdata, uint32_t count, uint64_t offset, uint32_t flags,
-               int *err);
-  int (*extents) (void *nxdata, uint32_t count, uint64_t offset, uint32_t
flags,
-                  struct nbdkit_extents *extents, int *err);
-  int (*cache) (void *nxdata, uint32_t count, uint64_t offset, uint32_t flags,
-                int *err);
+  int (*flush) (backend *nxdata, uint32_t flags, int *err);
+  int (*trim) (backend *nxdata, uint32_t count, uint64_t offset,
+               uint32_t flags, int *err);
+  int (*zero) (backend *nxdata, uint32_t count, uint64_t offset,
+               uint32_t flags, int *err);
+  int (*extents) (backend *nxdata, uint32_t count, uint64_t offset,
+                  uint32_t flags, struct nbdkit_extents *extents, int *err);
+  int (*cache) (backend *nxdata, uint32_t count, uint64_t offset,
+                uint32_t flags, int *err);
 };

 /* Extent functions. */
@@ -127,66 +136,66 @@ struct nbdkit_filter {
   void (*load) (void);
   void (*unload) (void);

-  int (*config) (nbdkit_next_config *next, void *nxdata,
+  int (*config) (nbdkit_next_config *next, backend *nxdata,
                  const char *key, const char *value);
-  int (*config_complete) (nbdkit_next_config_complete *next, void *nxdata);
+  int (*config_complete) (nbdkit_next_config_complete *next, backend *nxdata);
   const char *config_help;
   int (*thread_model) (void);
-  int (*preconnect) (nbdkit_next_preconnect *next, void *nxdata, int readonly);
+  int (*preconnect) (nbdkit_next_preconnect *next, backend *nxdata,
+                     int readonly);

-  void * (*open) (nbdkit_next_open *next, void *nxdata,
+  void * (*open) (nbdkit_next_open *next, backend *nxdata,
                   int readonly);
   void (*close) (void *handle);

-  int (*prepare) (struct nbdkit_next_ops *next_ops, void *nxdata,
+  int (*prepare) (struct nbdkit_next_ops *next_ops, backend *nxdata,
                   void *handle, int readonly);
-  int (*finalize) (struct nbdkit_next_ops *next_ops, void *nxdata,
+  int (*finalize) (struct nbdkit_next_ops *next_ops, backend *nxdata,
                    void *handle);

-  int64_t (*get_size) (struct nbdkit_next_ops *next_ops, void *nxdata,
+  int64_t (*get_size) (struct nbdkit_next_ops *next_ops, backend *nxdata,
                        void *handle);

-  int (*can_write) (struct nbdkit_next_ops *next_ops, void *nxdata,
+  int (*can_write) (struct nbdkit_next_ops *next_ops, backend *nxdata,
                     void *handle);
-  int (*can_flush) (struct nbdkit_next_ops *next_ops, void *nxdata,
+  int (*can_flush) (struct nbdkit_next_ops *next_ops, backend *nxdata,
                     void *handle);
   int (*is_rotational) (struct nbdkit_next_ops *next_ops,
-                        void *nxdata,
-                        void *handle);
-  int (*can_trim) (struct nbdkit_next_ops *next_ops, void *nxdata,
+                        backend *nxdata, void *handle);
+  int (*can_trim) (struct nbdkit_next_ops *next_ops, backend *nxdata,
                    void *handle);
-  int (*can_zero) (struct nbdkit_next_ops *next_ops, void *nxdata,
+  int (*can_zero) (struct nbdkit_next_ops *next_ops, backend *nxdata,
                    void *handle);
-  int (*can_fast_zero) (struct nbdkit_next_ops *next_ops, void *nxdata,
+  int (*can_fast_zero) (struct nbdkit_next_ops *next_ops, backend *nxdata,
                         void *handle);
-  int (*can_extents) (struct nbdkit_next_ops *next_ops, void *nxdata,
+  int (*can_extents) (struct nbdkit_next_ops *next_ops, backend *nxdata,
                       void *handle);
-  int (*can_fua) (struct nbdkit_next_ops *next_ops, void *nxdata,
+  int (*can_fua) (struct nbdkit_next_ops *next_ops, backend *nxdata,
                   void *handle);
-  int (*can_multi_conn) (struct nbdkit_next_ops *next_ops, void *nxdata,
+  int (*can_multi_conn) (struct nbdkit_next_ops *next_ops, backend *nxdata,
                          void *handle);
-  int (*can_cache) (struct nbdkit_next_ops *next_ops, void *nxdata,
+  int (*can_cache) (struct nbdkit_next_ops *next_ops, backend *nxdata,
                     void *handle);

-  int (*pread) (struct nbdkit_next_ops *next_ops, void *nxdata,
+  int (*pread) (struct nbdkit_next_ops *next_ops, backend *nxdata,
                 void *handle, void *buf, uint32_t count, uint64_t offset,
                 uint32_t flags, int *err);
-  int (*pwrite) (struct nbdkit_next_ops *next_ops, void *nxdata,
+  int (*pwrite) (struct nbdkit_next_ops *next_ops, backend *nxdata,
                  void *handle,
                  const void *buf, uint32_t count, uint64_t offset,
                  uint32_t flags, int *err);
-  int (*flush) (struct nbdkit_next_ops *next_ops, void *nxdata,
+  int (*flush) (struct nbdkit_next_ops *next_ops, backend *nxdata,
                 void *handle, uint32_t flags, int *err);
-  int (*trim) (struct nbdkit_next_ops *next_ops, void *nxdata,
+  int (*trim) (struct nbdkit_next_ops *next_ops, backend *nxdata,
                void *handle, uint32_t count, uint64_t offset, uint32_t flags,
                int *err);
-  int (*zero) (struct nbdkit_next_ops *next_ops, void *nxdata,
+  int (*zero) (struct nbdkit_next_ops *next_ops, backend *nxdata,
                void *handle, uint32_t count, uint64_t offset, uint32_t flags,
                int *err);
-  int (*extents) (struct nbdkit_next_ops *next_ops, void *nxdata,
+  int (*extents) (struct nbdkit_next_ops *next_ops, backend *nxdata,
                   void *handle, uint32_t count, uint64_t offset, uint32_t
flags,
                   struct nbdkit_extents *extents, int *err);
-  int (*cache) (struct nbdkit_next_ops *next_ops, void *nxdata,
+  int (*cache) (struct nbdkit_next_ops *next_ops, backend *nxdata,
                 void *handle, uint32_t count, uint64_t offset, uint32_t flags,
                 int *err);
 };
diff --git a/server/filters.c b/server/filters.c
index 92b0ceb3..8985ebeb 100644
--- a/server/filters.c
+++ b/server/filters.c
@@ -127,9 +127,8 @@ filter_dump_fields (struct backend *b)
 }

 static int
-next_config (void *nxdata, const char *key, const char *value)
+next_config (struct backend *b, const char *key, const char *value)
 {
-  struct backend *b = nxdata;
   b->config (b, key, value);
   return 0;
 }
@@ -151,9 +150,8 @@ filter_config (struct backend *b, const char *key, const
char *value)
 }

 static int
-next_config_complete (void *nxdata)
+next_config_complete (struct backend *b)
 {
-  struct backend *b = nxdata;
   b->config_complete (b);
   return 0;
 }
@@ -173,13 +171,6 @@ filter_config_complete (struct backend *b)
     b->next->config_complete (b->next);
 }

-static int
-next_preconnect (void *nxdata, int readonly)
-{
-  struct backend *b_next = nxdata;
-  return b_next->preconnect (b_next, readonly);
-}
-
 static int
 filter_preconnect (struct backend *b, int readonly)
 {
@@ -188,7 +179,7 @@ filter_preconnect (struct backend *b, int readonly)
   debug ("%s: preconnect", b->name);

   if (f->filter.preconnect)
-    return f->filter.preconnect (next_preconnect, b->next, readonly);
+    return f->filter.preconnect (b->next->preconnect, b->next,
readonly);
   else
     return b->next->preconnect (b->next, readonly);
 }
@@ -202,14 +193,6 @@ plugin_magic_config_key (struct backend *b)
   return b->next->magic_config_key (b->next);
 }

-static int
-next_open (void *nxdata, int readonly)
-{
-  struct backend *b_next = nxdata;
-
-  return backend_open (b_next, readonly);
-}
-
 static void *
 filter_open (struct backend *b, int readonly)
 {
@@ -220,7 +203,7 @@ filter_open (struct backend *b, int readonly)
    * inner-to-outer ordering.
    */
   if (f->filter.open)
-    handle = f->filter.open (next_open, b->next, readonly);
+    handle = f->filter.open (backend_open, b->next, readonly);
   else if (backend_open (b->next, readonly) == -1)
     handle = NULL;
   else
@@ -237,171 +220,26 @@ filter_close (struct backend *b, void *handle)
     f->filter.close (handle);
 }

-/* The next_functions structure contains pointers to backend
- * functions.  These are only needed for type safety (nxdata is void
- * pointer, backend_* functions expect a struct backend * parameter).
- * nxdata is a pointer to the next backend in the linked list.
- */
-
-static int
-next_reopen (void *nxdata, int readonly)
-{
-  struct backend *b_next = nxdata;
-  return backend_reopen (b_next, readonly);
-}
-
-static int64_t
-next_get_size (void *nxdata)
-{
-  struct backend *b_next = nxdata;
-  return backend_get_size (b_next);
-}
-
-static int
-next_can_write (void *nxdata)
-{
-  struct backend *b_next = nxdata;
-  return backend_can_write (b_next);
-}
-
-static int
-next_can_flush (void *nxdata)
-{
-  struct backend *b_next = nxdata;
-  return backend_can_flush (b_next);
-}
-
-static int
-next_is_rotational (void *nxdata)
-{
-  struct backend *b_next = nxdata;
-  return backend_is_rotational (b_next);
-}
-
-static int
-next_can_trim (void *nxdata)
-{
-  struct backend *b_next = nxdata;
-  return backend_can_trim (b_next);
-}
-
-static int
-next_can_zero (void *nxdata)
-{
-  struct backend *b_next = nxdata;
-  return backend_can_zero (b_next);
-}
-
-static int
-next_can_fast_zero (void *nxdata)
-{
-  struct backend *b_next = nxdata;
-  return backend_can_fast_zero (b_next);
-}
-
-static int
-next_can_extents (void *nxdata)
-{
-  struct backend *b_next = nxdata;
-  return backend_can_extents (b_next);
-}
-
-static int
-next_can_fua (void *nxdata)
-{
-  struct backend *b_next = nxdata;
-  return backend_can_fua (b_next);
-}
-
-static int
-next_can_multi_conn (void *nxdata)
-{
-  struct backend *b_next = nxdata;
-  return backend_can_multi_conn (b_next);
-}
-
-static int
-next_can_cache (void *nxdata)
-{
-  struct backend *b_next = nxdata;
-  return backend_can_cache (b_next);
-}
-
-static int
-next_pread (void *nxdata, void *buf, uint32_t count, uint64_t offset,
-            uint32_t flags, int *err)
-{
-  struct backend *b_next = nxdata;
-  return backend_pread (b_next, buf, count, offset, flags, err);
-}
-
-static int
-next_pwrite (void *nxdata, const void *buf, uint32_t count, uint64_t offset,
-             uint32_t flags, int *err)
-{
-  struct backend *b_next = nxdata;
-  return backend_pwrite (b_next, buf, count, offset, flags, err);
-}
-
-static int
-next_flush (void *nxdata, uint32_t flags, int *err)
-{
-  struct backend *b_next = nxdata;
-  return backend_flush (b_next, flags, err);
-}
-
-static int
-next_trim (void *nxdata, uint32_t count, uint64_t offset, uint32_t flags,
-           int *err)
-{
-  struct backend *b_next = nxdata;
-  return backend_trim (b_next, count, offset, flags, err);
-}
-
-static int
-next_zero (void *nxdata, uint32_t count, uint64_t offset, uint32_t flags,
-           int *err)
-{
-  struct backend *b_next = nxdata;
-  return backend_zero (b_next, count, offset, flags, err);
-}
-
-static int
-next_extents (void *nxdata, uint32_t count, uint64_t offset, uint32_t flags,
-              struct nbdkit_extents *extents, int *err)
-{
-  struct backend *b_next = nxdata;
-  return backend_extents (b_next, count, offset, flags, extents, err);
-}
-
-static int
-next_cache (void *nxdata, uint32_t count, uint64_t offset,
-            uint32_t flags, int *err)
-{
-  struct backend *b_next = nxdata;
-  return backend_cache (b_next, count, offset, flags, err);
-}
-
 static struct nbdkit_next_ops next_ops = {
-  .reopen = next_reopen,
-  .get_size = next_get_size,
-  .can_write = next_can_write,
-  .can_flush = next_can_flush,
-  .is_rotational = next_is_rotational,
-  .can_trim = next_can_trim,
-  .can_zero = next_can_zero,
-  .can_fast_zero = next_can_fast_zero,
-  .can_extents = next_can_extents,
-  .can_fua = next_can_fua,
-  .can_multi_conn = next_can_multi_conn,
-  .can_cache = next_can_cache,
-  .pread = next_pread,
-  .pwrite = next_pwrite,
-  .flush = next_flush,
-  .trim = next_trim,
-  .zero = next_zero,
-  .extents = next_extents,
-  .cache = next_cache,
+  .reopen = backend_reopen,
+  .get_size = backend_get_size,
+  .can_write = backend_can_write,
+  .can_flush = backend_can_flush,
+  .is_rotational = backend_is_rotational,
+  .can_trim = backend_can_trim,
+  .can_zero = backend_can_zero,
+  .can_fast_zero = backend_can_fast_zero,
+  .can_extents = backend_can_extents,
+  .can_fua = backend_can_fua,
+  .can_multi_conn = backend_can_multi_conn,
+  .can_cache = backend_can_cache,
+  .pread = backend_pread,
+  .pwrite = backend_pwrite,
+  .flush = backend_flush,
+  .trim = backend_trim,
+  .zero = backend_zero,
+  .extents = backend_extents,
+  .cache = backend_cache,
 };

 static int
diff --git a/server/internal.h b/server/internal.h
index 9d314bf8..eaec31ba 100644
--- a/server/internal.h
+++ b/server/internal.h
@@ -1,5 +1,5 @@
 /* nbdkit
- * Copyright (C) 2013-2019 Red Hat Inc.
+ * Copyright (C) 2013-2020 Red Hat Inc.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
@@ -40,6 +40,7 @@
 #include <pthread.h>

 #define NBDKIT_API_VERSION 2
+#define NBDKIT_INTERNAL
 #include "nbdkit-plugin.h"
 #include "nbdkit-filter.h"
 #include "cleanup.h"
-- 
2.24.1

On Wed, Feb 12, 2020 at 09:38:29AM -0600, Eric Blake
wrote:
> +#ifdef NBDKIT_INTERNAL
> +/* Opaque type encapsulating all information needed for calling into
> + * the next filter or plugin.
> + */
> +typedef struct backend backend;
> +#else
> +typedef void backend;
> +#endif
Is it better to call it something with an nbdkit_* prefix?  Although
not advisable, there can be out of tree filters and we do publish
<nbdkit-filter.h> as a public header file.

Anyway, you decide - ACK patch.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

On 2/12/20 10:07 AM, Richard W.M. Jones wrote:
> On Wed, Feb 12, 2020 at 09:38:29AM -0600, Eric Blake wrote:
>> +#ifdef NBDKIT_INTERNAL
>> +/* Opaque type encapsulating all information needed for calling into
>> + * the next filter or plugin.
>> + */
>> +typedef struct backend backend;
>> +#else
>> +typedef void backend;
>> +#endif
> 
> Is it better to call it something with an nbdkit_* prefix?  Although
> not advisable, there can be out of tree filters and we do publish
> <nbdkit-filter.h> as a public header file.
Good idea.  Will fix, and then push.
> 
> Anyway, you decide - ACK patch.
> 
> Rich.
> 
-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org