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

[Richard W.M. Jones]

On Wed, Feb 16, 2022 at 11:27:05AM -0600, Eric Blake wrote:
> 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.

This meant I also had to add essentially duplicate checks in the new
filter (patch 6).

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