[prev in list] [next in list] [prev in thread] [next in thread] 

List:       libguestfs
Subject:    Re: [Libguestfs] [nbdkit PATCH 2/3] filter: Add .can_zero/.can_fua overrides
From:       "Richard W.M. Jones" <rjones () redhat ! com>
Date:       2018-01-24 14:26:20
Message-ID: 20180124142620.GN19514 () redhat ! com
[Download RAW message or body]

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

_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs


[prev in list] [next in list] [prev in thread] [next in thread]