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

[Eric Blake]

On Thu, Feb 17, 2022 at 05:51:59PM +0000, Richard W.M. Jones wrote:
> > In fact, maybe we should plan for that right away, by having four
> > parameters, ending with 'uint32_t *maxdata, uint64_t *maxlen'.  This
> > would match the fact that right now, the blocksize filter allows
> > maxdata=32M maxlen=3G to break up large pwrite requests differently
> > than large zero requests.  For the existing NBD protocol, only the
> > first three will be advertised to the client (and we ignore the fourth
> > for now), but down the road with 64-bit extensions, it will make it
> > easier to advertise a 64-bit non-data max length alongside the
> > existing 3 32-bit constraints, as the NBD protocol gains another
> > extension.
> 
> Presuming that we're going to need a new NBDKIT_API_VERSION for this
> anyway, we can in fact change the signature of .block_size by adding a
> new parameter.  This doesn't break backwards compatiblity.

Indeed, and part of my work on 64-bit extensions is figuring out how
to implement NBDKIT_API_VERSION 3 for plugins.  Maybe it's worth an
addition in TODO to remind us of yet another thing worth visiting as
part of v3 api.

> 
> [...]
> > > +++ b/server/backend.c
> > > @@ -214,6 +214,7 @@ static struct nbdkit_next_ops next_ops = {
> > >    .finalize = backend_finalize,
> > >    .export_description = backend_export_description,
> > >    .get_size = backend_get_size,
> > > +  .block_size = backend_block_size,
> > >    .can_write = backend_can_write,
> > >    .can_flush = backend_can_flush,
> > >    .is_rotational = backend_is_rotational,
> > > @@ -265,6 +266,7 @@ backend_open (struct backend *b, int readonly, const char *exportname,
> > >    c->conn = shared ? NULL : conn;
> > >    c->state = 0;
> > >    c->exportsize = -1;
> > > +  c->minimum_block_size = c->preferred_block_size = c->maximum_block_size = -1;
> > 
> > So we actually implement two sentinels - all -1 (we haven't
> > initialized yet, and a plugin cannot explicitly set things to this
> > value, because it violates minimum <= 64k), and all 0 (initialized but
> > nothing to advertise), in addition to all other values (initialized,
> > and passed our constraint validator so worth advertising).
> 
> Hopefully the -1 sentinel is never visible outside the server, and
> could in fact be changed (eg. adding an extra "initialized" boolean).
> 
> If I've got it right, it should neither be seen by plugins, filters or
> the client(!), nor should the plugin or filter be allowed to set any
> of these to -1, except maximum.

It also looked that way to me - other than maximum of -1 (which the
NBD protocol permits), I could not spot anywhere that you would leak -1 unintended.

> > 
> > Did we want to stick all of the constraint validation code in a common
> > helper function, rather than open-coding it at each place that wants
> > to perform validation?
> 
> The code is a bit different unfortunately ..  In the filter code we
> have to consider the case where some of the config_* variables are
> unset.

Yeah, having re-read blocksize-policy code, that is indeed the case
(we don't know the full picture at .config_complete).  So living with
the near-duplication is probably the best we can do for now.

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