[Libguestfs] [nbdkit PATCH 2/3] filter: Add .can_zero/.can_fua overrides

[Richard W.M. Jones]

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