[Libguestfs] [PATCH nbdkit v2 1/3] file: Move file operators to a new common/fileops mini-library.

[Eric Blake]

On 4/9/20 3:36 AM, Richard W.M. Jones wrote:
> Writing "file-like" plugins is hard because you have to implement your
> own .zero, .trim, .extents, etc, and that is very complicated.
> However implementations of these functions already exist in the file
> plugin.  By factoring out the file plugin into a separate "fileops"
> mini-library we can reuse these implementations in other plugins.
> 
> This refactoring commit creates a new mini-library called fileops, and
> uses it to implement the file plugin.
> 
> Note that the name or prefix "file" leaks into fileops in a few
> places: the debug option is still called ‘-D file.zero=1’ and the
> nbdkit --dump-plugin output will still contain ‘file_blksszget’ etc.
> However I think that is fine as it should make usage more consistent
> across future file-like plugins.

Agreed.

> ---
>   configure.ac               |   1 +
>   Makefile.am                |   1 +
>   common/fileops/Makefile.am |  45 +++
>   plugins/file/Makefile.am   |   2 +
>   common/fileops/fileops.h   | 158 +++++++++++
>   common/fileops/fileops.c   | 547 +++++++++++++++++++++++++++++++++++++
>   plugins/file/file.c        | 541 ++----------------------------------
>   7 files changed, 773 insertions(+), 522 deletions(-)
> 

> +++ b/common/fileops/fileops.h

> +#ifndef NBDKIT_FILEOPS_H
> +#define NBDKIT_FILEOPS_H
> +
> +#include <config.h>

Unnecessary.  Since all of our .h files will be included by another .c 
file that has <config.h> as its first include, we don't need .h files to 
try the include a second time (even if it is idempotent).

I'll clean up the existing places where we don't need it, as a separate 
cleanup patch.

> +/* This mini-library helps when writing plugins which are like
> + * nbdkit-file-plugin.  It is also used to implement
> + * nbdkit-file-plugin itself.  What this means is if your plugin
> + * (after perhaps some custom setup) serves a single, whole, local
> + * file or block device then you can implement most of the data
> + * serving part of the plugin using these generic fileops.  The
> + * advantage is that this mini-library deals with the complexity of
> + * implementing callbacks such as .zero and .trim efficiently.

Even if your plugin serves the concatenation of several fds as if one 
single image, fileops may still prove useful; in those situations, 
you'll have to write your own callbacks that then call into specific 
fileops helper functions, rather than being able to use the convenience 
macro that directly sets callbacks to fileops functions.  But that can 
be a later patch; for now, this wording is fine.

> + *
> + * To use it:
> + *
> + * - your plugin must define and use NBDKIT_API_VERSION == 2
> + *
> + * - your plugin per-connection handle must either have type ‘struct
> + *   fileops *’, or include ‘struct fileops’ as the first member
> + *
> + * - add FILEOPS_READ_WRITE_CALLBACKS or
> + *   FILEOPS_READ_ONLY_CALLBACKS to your ‘struct nbdkit_plugin’

This is the step that gets dropped when using fileops for concatenated fds.

> + *
> + * - call init_fileops from your .open callback, and close_fileops
> + *   from your .close callback
> + *
> + * - optionally call fileops_dump_plugin from your .dump_plugin
> + *   callback if you have one
> + *
> + * - the fileops mini-library is linked statically into the plugin
> + *
> + * This mini-library is safe to use from any thread model, including
> + * NBDKIT_THREAD_MODEL_PARALLEL (in fact, parallel is the recommended
> + * thread model).
> + *
> + * See plugins/file/file.c for an example.
> + */
> +

> +
> +/* Initialize the fileops struct.  ‘fd’ is a file descriptor opened on
> + * the local file or block device that you want to serve.  Call this
> + * from your .open callback after allocating the handle and setting up
> + * the file descriptor.
> + */
> +extern int init_fileops (int fd, struct fileops *fops);

Mention that you must not access fd after this call succeeds.  Also, 
decide if that also applies to error situations (does this function 
guarantee to call close(fd) even when returning -1, or does it only take 
over fd on success?).

> +
> +/* Close the file descriptor and perform any other cleanup (but it
> + * doesn’t free the struct or handle).  Use this in your .close
> + * callback.
> + */
> +extern void close_fileops (struct fileops *fops);
> +
> +/* You may optionally define a .dump_plugin callback which calls this. */
> +extern void fileops_dump_plugin (void);
> +
> +/* Use the fileops callbacks by adding either
> + * FILEOPS_READ_WRITE_CALLBACKS or FILEOPS_READ_ONLY_CALLBACKS to your
> + * ‘struct nbdkit_plugin’.
> + */
> +#define FILEOPS_READ_WRITE_CALLBACKS            \
> +  .can_trim           = fileops_can_trim,       \
> +  .can_fua            = fileops_can_fua,        \
> +  .pwrite             = fileops_pwrite,         \
> +  .flush              = fileops_flush,          \
> +  .trim               = fileops_trim,           \
> +  .zero               = fileops_zero,           \
> +  FILEOPS_READ_ONLY_CALLBACKS

Someday, I hope to add .can_fast_zero support; but for now, this matches 
what the file plugin can do.

> +
> +#define FILEOPS_READ_ONLY_CALLBACKS             \
> +  .get_size           = fileops_get_size,       \
> +  .can_cache          = fileops_can_cache,      \
> +  .pread              = fileops_pread,          \
> +  .errno_is_preserved = 1,                      \
> +  FILEOPS_MAYBE_EXTENTS                         \
> +  FILEOPS_MAYBE_CACHE
> +
> +#ifdef SEEK_HOLE
> +#define FILEOPS_MAYBE_EXTENTS                   \
> +  .can_extents        = fileops_can_extents,    \
> +  .extents            = fileops_extents,
> +#else
> +#define FILEOPS_MAYBE_EXTENTS /* nothing */
> +#endif
> +
> +#ifdef HAVE_POSIX_FADVISE
> +#define FILEOPS_MAYBE_CACHE                     \
> +  .cache             = fileops_cache,
> +#else
> +#define FILEOPS_MAYBE_CACHE /* nothing */
> +#endif
> +
> +/* Don’t call these directly. */

Well, don't call them directly if you wrapped a single fd and used the 
macros above.  But for multi-fd clients, calling these after deciding 
which fd to forward to makes sense.  Or maybe we rename these slightly, 
and the versions we export for multi-fd plugins are type-safe with 
struct fileops* instead of void* for the first parameter.

> +extern int64_t fileops_get_size (void *handle);
> +extern int fileops_can_trim (void *handle);

> +++ b/common/fileops/fileops.c

> +
> +int
> +init_fileops (int fd, struct fileops *fops)
> +{
> +  struct stat statbuf;
> +
> +  if (fstat (fd, &statbuf) == -1) {
> +    nbdkit_error ("fstat: %m");
> +    return -1;
> +  }

This does not close fd on failure, but decide if that's the semantics 
you want when tweaking the documentation above.

> +void
> +close_fileops (struct fileops *fops)
> +{
> +  close (fops->fd);
> +}

Do you want to make sure this is safe to call even if init_fileops() 
failed?  If so, I'd recommend that init_fileops set 'fops->fd = -1' 
before returning failure, and that this check for -1 before trying close.

> +/* Get the file size. */
> +int64_t
> +fileops_get_size (void *handle)
> +{
> +  struct fileops *fops = handle;
> +
> +  if (fops->is_block_device) {
> +    ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lseek_lock);
> +    return block_device_size (fops->fd);
> +  } else {
> +    /* Regular file. */
> +    struct stat statbuf;
> +
> +    if (fstat (fops->fd, &statbuf) == -1) {

This matches what file already did, but should we cache the file size we 
read during init_fileops, rather than having to do a second fstat()?

> +/* Write data to the file. */
> +int
> +fileops_pwrite (void *handle, const void *buf, uint32_t count, uint64_t offset,
> +                uint32_t flags)
> +{
> +  struct fileops *fops = handle;
> +
> +  while (count > 0) {
> +    ssize_t r = pwrite (fops->fd, buf, count, offset);
> +    if (r == -1) {
> +      nbdkit_error ("pwrite: %m");
> +      return -1;
> +    }
> +    buf += r;
> +    count -= r;
> +    offset += r;
> +  }
> +
> +  if ((flags & NBDKIT_FLAG_FUA) && fileops_flush (handle, 0) == -1)
> +    return -1;
> +

Yeah, reading ahead to patch 3, I can see parameterizing things (perhaps 
an additional bool parameter to init_fileops) to completely skip 
flushing, in cases where we know that flushing is pointless due to the 
temporary nature of the plugin.


> +
> +#ifdef SEEK_HOLE
> +/* Extents. */
> +
> +int
> +fileops_can_extents (void *handle)

Hmm - this declaration is conditional, and the macros only install the 
callback when the function is defined.  But is that safe enough, or 
should we always compile the entry point even the macros do not install 
it as a callback (where the #if moves inside the function body, with a 
sane fallback for the case where we don't support SEEK_HOLE).


> +#if HAVE_POSIX_FADVISE
> +/* Caching. */
> +int
> +fileops_cache (void *handle, uint32_t count, uint64_t offset, uint32_t flags)
> +{

And similar.

> +++ b/plugins/file/file.c
> @@ -34,51 +34,18 @@
>   
>   #include <stdio.h>
>   #include <stdlib.h>
> -#include <stdbool.h>
> +#include <stdint.h>

Why the addition?

>   /* Create the per-connection handle. */
>   static void *
>   file_open (int readonly)
>   {
> -  struct handle *h;
> -  struct stat statbuf;
> -  int flags;
> +  struct fileops *fops;
> +  int fd, flags;
>   
> -  h = malloc (sizeof *h);
> -  if (h == NULL) {
> +  fops = malloc (sizeof *fops);

Uninitialized memory...

> +  if (fops == NULL) {
>       nbdkit_error ("malloc: %m");
>       return NULL;
>     }
> @@ -176,98 +120,33 @@ file_open (int readonly)
>     else
>       flags |= O_RDWR;
>   
> -  h->fd = open (filename, flags);
> -  if (h->fd == -1) {
> +  fd = open (filename, flags);
> +  if (fd == -1) {
>       nbdkit_error ("open: %s: %m", filename);
> -    free (h);
> +    free (fops);
>       return NULL;
>     }
>   
> -  if (fstat (h->fd, &statbuf) == -1) {
> -    nbdkit_error ("fstat: %s: %m", filename);
> -    free (h);
> +  if (init_fileops (fd, fops) == -1) {
> +    free (fops);
>       return NULL;

...good. init_fileops() initialized the memory on success, and we don't 
leak the memory on failure.  Bad - we leak fd if init_fileops() fails 
(where you fix it, there or here, determines what you document for the 
contract).

>   /* Allow multiple parallel connections from a single client. */
>   static int
>   file_can_multi_conn (void *handle)

The fileops docs probably ought to mention that .can_multi_conn is the 
responsibility of the plugin, as there is no sane answer that fileops 
can generalize.


>   static struct nbdkit_plugin plugin = {
>     .name              = "file",
>     .longname          = "nbdkit file plugin",
> @@ -655,24 +166,10 @@ static struct nbdkit_plugin plugin = {
>     .dump_plugin       = file_dump_plugin,
>     .open              = file_open,
>     .close             = file_close,
> -  .get_size          = file_get_size,
>     .can_multi_conn    = file_can_multi_conn,
> -  .can_trim          = file_can_trim,
> -  .can_fua           = file_can_fua,
> -  .can_cache         = file_can_cache,
> -  .pread             = file_pread,
> -  .pwrite            = file_pwrite,
> -  .flush             = file_flush,
> -  .trim              = file_trim,
> -  .zero              = file_zero,
> -#ifdef SEEK_HOLE
> -  .can_extents       = file_can_extents,
> -  .extents           = file_extents,
> -#endif
> -#if HAVE_POSIX_FADVISE
> -  .cache             = file_cache,
> -#endif
> -  .errno_is_preserved = 1,
> +
> +  /* This bulk of this plugin is implemented in common/fileops/fileops.c */
> +  FILEOPS_READ_WRITE_CALLBACKS
>   };

Overall, I like how this is heading.

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