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