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
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
