Libguestfs - Feb 2022 - [PATCH nbdkit 1/6] server: Add new plugin/filter .block_size callback

On Wed, Feb 16, 2022 at 04:20:36PM +0000, Richard W.M. Jones
wrote:
> This commit implements backend_block_size() and wires it up to a new
> .block_size callback from the plugin through any filters on top.
> 
> This callback will allow plugins to pass their minimum, preferred and
> maximum block size through the NBD protocol NBD_INFO_BLOCK_SIZE info
> option.
> 
> The new function is not called from anywhere yet, so for the moment
> this commit does nothing.
> ---
>  include/nbdkit-filter.h |  4 ++++
>  include/nbdkit-plugin.h |  3 +++
>  server/internal.h       |  9 ++++++++
>  server/backend.c        | 29 +++++++++++++++++++++++
>  server/filters.c        | 17 ++++++++++++++
>  server/plugins.c        | 51 +++++++++++++++++++++++++++++++++++++++++
>  6 files changed, 113 insertions(+)
> 
>  
> +int
> +backend_block_size (struct context *c,
> +                    uint32_t *minimum, uint32_t *preferred, uint32_t
*maximum)
> +{
> +  PUSH_CONTEXT_FOR_SCOPE (c);
> +  struct backend *b = c->b;
> +  int r;
> +
> +  assert (c->handle && (c->state & HANDLE_CONNECTED));
> +  if (c->minimum_block_size != -1) {
> +    *minimum = c->minimum_block_size;
> +    *preferred = c->preferred_block_size;
> +    *maximum = c->maximum_block_size;
> +    return 0;
> +  }
> +  else {
> +    controlpath_debug ("%s: block_size", b->name);
> +    r = b->block_size (c, minimum, preferred, maximum);
> +    if (r == 0) {
> +      c->minimum_block_size = *minimum;
> +      c->preferred_block_size = *preferred;
> +      c->maximum_block_size = *maximum;
> +    }
We should probably ensure that NBD protocol constraints are met rather
than just assuming the plugin gave us sane values: minimum must be
power of 2 between 1 and 64k; preferred must be power of 2 between
max(minsize,512) and 32M; maximum must be either -1 or a multiple of
minsize (but not necessarily a power of 2).

/me reads on...
> +++ b/server/plugins.c
>  
> +static int
> +plugin_block_size (struct context *c,
> +                   uint32_t *minimum, uint32_t *preferred, uint32_t
*maximum)
> +{
> +  struct backend *b = c->b;
> +  struct backend_plugin *p = container_of (b, struct backend_plugin,
backend);
> +  int r;
> +
> +  if (p->plugin.block_size) {
> +    r = p->plugin.block_size (c->handle, minimum, preferred,
maximum);
> +    if (r == 0) {
> +      /* To make scripting easier, it's permitted to set
> +       * minimum = preferred = maximum = 0 and return 0.
> +       * That means "no information", and works the same
> +       * way as the else clause below.
> +       */
> +      if (*minimum == 0 && *preferred == 0 && *maximum ==
0)
> +        return 0;
> +
> +      if (*minimum < 1) {
> +        nbdkit_error ("plugin must set minimum block size >=
1");
> +        r = -1;
> +      }
In other words, either all three values are 0 (no info), or all three
values are non-zero, ruling out partial info.  Makes sense.  We could
instead decide to provide defaults to let plugins provide partial info
(such as if minsize is nonzero but preferred is 0, then set preferred
to min(minsize, 4k), but I don't know if it would be worth the extra
complication.
> +      if (! is_power_of_2 (*minimum)) {
> +        nbdkit_error ("plugin must set minimum block size to a power
of 2");
> +        r = -1;
> +      }
> +      if (*minimum > *preferred || *preferred > *maximum) {
> +        nbdkit_error ("plugin must set minimum block size "
> +                      "<= preferred <= maximum");
> +        r = -1;
The NBD protocol allows the maximum to match the block size (and
potentially be smaller than preferred size) if the overall export is
small (for example, a 512-byte file can advertise a preferred size of
4k but a max size matching the actual size of 512).  Yeah, it's a
corner case, so I'm also okay if we insist that if a plugin give us
information, the info be strictly ranked rather than allowing all
corner cases possible in the NBD protocol.
> +      }
> +      /* XXX Other checks should be performed here:
> +       * preferred and maximum should be multiples of the minimum.
> +       */
More specifically, preferred must also be a power of 2.
> +    }
> +    return r;
> +  }
> +  else {
> +    /* If there is no .block_size call then return minimum = preferred
> +     * = maximum = 0, which is a sentinel meaning don't send the
> +     * NBD_INFO_BLOCK_SIZE message.
> +     */
> +    *minimum = *preferred = *maximum = 0;
Yes, supporting a sentinel as the ability to turn off the message
makes sense.
> +    return 0;
> +  }
> +}
> +
>  static int
>  normalize_bool (int value)
>  {
> @@ -824,6 +874,7 @@ static struct backend plugin_functions = {
>    .close = plugin_close,
>    .export_description = plugin_export_description,
>    .get_size = plugin_get_size,
> +  .block_size = plugin_block_size,
>    .can_write = plugin_can_write,
>    .can_flush = plugin_can_flush,
>    .is_rotational = plugin_is_rotational,
> -- 
> 2.35.1
Looks like a good start.

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

On Wed, Feb 16, 2022 at 11:16:49AM -0600, Eric Blake wrote:
> > +int
> > +backend_block_size (struct context *c,
> > +                    uint32_t *minimum, uint32_t *preferred, uint32_t
*maximum)
> > +{
> > +    r = b->block_size (c, minimum, preferred, maximum);
> > +    if (r == 0) {
> > +      c->minimum_block_size = *minimum;
> > +      c->preferred_block_size = *preferred;
> > +      c->maximum_block_size = *maximum;
> > +    }
> 
> We should probably ensure that NBD protocol constraints are met rather
> than just assuming the plugin gave us sane values: minimum must be
> power of 2 between 1 and 64k; preferred must be power of 2 between
> max(minsize,512) and 32M; maximum must be either -1 or a multiple of
> minsize (but not necessarily a power of 2).
> 
> /me reads on...
> 
> > +++ b/server/plugins.c
> >  
> > +static int
> > +plugin_block_size (struct context *c,
> > +                   uint32_t *minimum, uint32_t *preferred, uint32_t
*maximum)
> > +{
> > +  struct backend *b = c->b;
> > +  struct backend_plugin *p = container_of (b, struct backend_plugin,
backend);
> > +  int r;
> > +
> > +  if (p->plugin.block_size) {
> > +    r = p->plugin.block_size (c->handle, minimum, preferred,
maximum);
> > +    if (r == 0) {
> > +      /* To make scripting easier, it's permitted to set
> > +       * minimum = preferred = maximum = 0 and return 0.
> > +       * That means "no information", and works the same
> > +       * way as the else clause below.
> > +       */
> > +      if (*minimum == 0 && *preferred == 0 &&
*maximum == 0)
> > +        return 0;
> > +
> > +      if (*minimum < 1) {
> > +        nbdkit_error ("plugin must set minimum block size >=
1");
> > +        r = -1;
> > +      }
> 
> In other words, either all three values are 0 (no info), or all three
> values are non-zero, ruling out partial info.  Makes sense.  We could
> instead decide to provide defaults to let plugins provide partial info
> (such as if minsize is nonzero but preferred is 0, then set preferred
> to min(minsize, 4k), but I don't know if it would be worth the extra
> complication.
...and then failed to complete my thought.  Okay, so instead of
validating that parameters are sane at the backend level, you only
enforce them to be sane at the plugin level (since all filters are
in-tree, we have a bit more control there).  Seems like a reasonable
tradeoff, although I'm still a bit worried that not checking in the
backend exposes us to a little more risk of writing a bad in-tree
filter.

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