[Libguestfs] [PATCH nbdkit 2/3] Refactor plugin_* functions into a backend struct.

[Eric Blake]

On 01/16/2018 08:11 AM, Richard W.M. Jones wrote:
> Introduce the concept of a backend.  Currently the only type of
> backend is a plugin, and there can only be one of them.  Instead of
> calling functions like ‘plugin_pwrite’ you call the backend method
> ‘backend->pwrite (backend, ...)’.
> 
> The change is largely mechanical.  I was able to remove ‘assert (dl)’
> statements throughout since we can now prove they will never be
> called.
> 
> Note this does not lift the restriction of one plugin per server, and
> it can *never* do that because plugins can use global variables.
> ---
>  src/connections.c |  40 +++--
>  src/internal.h    |  49 ++---
>  src/locks.c       |   8 +-
>  src/main.c        |  31 ++--
>  src/plugins.c     | 523 ++++++++++++++++++++++++++++++------------------------
>  5 files changed, 360 insertions(+), 291 deletions(-)
> 

> +++ b/src/internal.h
> @@ -108,6 +108,8 @@ extern int threads;
>  extern volatile int quit;
>  extern int quit_fd;
>  
> +struct backend *backend;

Missing an 'extern', which means...


> +++ b/src/main.c
> @@ -64,7 +64,7 @@
>  
>  static int is_short_name (const char *);
>  static char *make_random_fifo (void);
> -static void open_plugin_so (const char *filename, int short_name);
> +static struct backend *open_plugin_so (const char *filename, int short_name);
>  static void start_serving (void);
>  static void set_up_signals (void);
>  static void run_command (void);
> @@ -103,6 +103,9 @@ volatile int quit;
>  int quit_fd;
>  static int write_quit_fd;
>  
> +/* The currently loaded plugin. */
> +struct backend *backend;

...you are declaring more than one instance, and the linker may either
balk or do unexpected things.

> +++ b/src/plugins.c
> @@ -49,192 +49,102 @@
>  /* Maximum read or write request that we will handle. */
>  #define MAX_REQUEST_SIZE (64 * 1024 * 1024)
>  
> -/* Currently the server can only load one plugin (see TODO).  Hence we
> - * can just use globals to store these.
> +/* We extend the generic backend struct with extra fields relating
> + * to this plugin.
>   */
> -static char *filename;
> -static void *dl;
> -static struct nbdkit_plugin plugin;
> -
> -void
> -plugin_register (const char *_filename,
> -                 void *_dl, struct nbdkit_plugin *(*plugin_init) (void))
> +struct backend_plugin {
> +  struct backend backend;
> +  char *filename;
> +  void *dl;
> +  struct nbdkit_plugin plugin;
> +};

So we still have a global variable 'backend', and I agree with your
assessment that since plugins can use global variables we can't safely
load the same plugin multiple times in one server.  But this refactoring
does mean more state is now passed as explicit context rather than at a
distance through globals, so I'm liking it.

> +
> +static void
> +plugin_free (struct backend *b)
>  {

> +  struct backend_plugin *p = (struct backend_plugin *) b;

qemu has a macro container_of() which makes casts like this be a bit
more declarative and utilize a bit more type-safety from the compiler
(it also lets you build derived type wrappers and upcast back to the
parent base class without requiring the base class to be the first
member of the derived class, although that's a side-effect we don't need
to exploit here):

#define container_of(ptr, type, member) ({                      \
        const typeof(((type *) 0)->member) *__mptr = (ptr);     \
        (type *) ((char *) __mptr - offsetof(type, member));})

which would be used here as:

struct backend_plugin *p = container_of (b, backend_plugin, backend);


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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20180116/f6540e4d/attachment.sig>