[Libguestfs] [nbdkit PATCH v2 4/6] filters: Track next handle alongside next backend

[Eric Blake]

On 3/1/21 9:53 AM, Richard W.M. Jones wrote:
> So ... this is all complicated ...  And reverts some changes which I
> found simplified things.
> 
> Stepping back here, is it the case that next-ops is really a hack, and
> what filters should actually get is some kind of connection object
> (let's call it that, even if it doesn't exactly correspond to "struct
> connection"), on which they can make general calls to the layer
> beneath.

I'm almost thinking of calling it a 'context object', but yes, that's
the idea.

> 
> Then we allow filters to open and close new connection objects.

Yes.

> 
> How do filters get these connection objects?  I would say that unlike
> next-ops, filters should not get a connection object with each
> callback.  Instead in filter_open the filter should explicitly open a
> new connection, and in filter_close explicitly close it.  Then
> filter_pread would make a pread() call on the connection object which
> the filter has saved in the filter's handle.
> 
> My questions would be, assuming a design like this magically existed:
> 
> (1) Does it solve the problem for multi-conn filter?
> 
> (2) Does it solve the problem for background threads?

Right now, I'm still experimenting with design on top of this series
which will get a working background thread setup (with a
proof-of-concept for that using the ext2 filter), but here's my thoughts
so far:

A filter needs to know which backend it is (or which backend is next);
right now, we use 'void *nxdata' to encapsulate struct backend*, but
could make 'void*' more typesafe at the expense of a lot of mechanical
churn in the filters.

At any point in .after_fork(), a filter can call:

nbdkit_context *nbdkit_context_open (void *nxdata, int readonly, const
char *exportname);

to open up a new context visiting the appropriate backend stored in
nxdata (but leaving nxdata unchanged), along with a matching:

void nbdkit_context_close (nbdkit_context *context);

to close it.  Most filters want 1:1 context per connection, which means
we want a way to associate a just-opened context into the usual handle
so that next_ops->FOO(nxdata) works as before.  I'm thinking:

int nbdkit_context_associate (void *nxdata, nbdkit_context *context);

which returns 0 on success, -1 on failure, and updates nxdata so that
future calls to next_ops->FOO(nxdata) now operate on the context.  The
existing:

filter_open (nbdkit_next_open *next_open, void *nxdata, ...)

where a filter calls next_open (nxdata, ...) remains as shorthand for
calling both nbdkit_context_open() and nbdkit_context_associate().
Filters like ext2 that want to share a single context among all
connections never call next_open() or nbdkit_context_associate(), but
instead need a way to operate on the single context opened during
.after_fork(); this could look like:

int nbdkit_context_next_ops (nbdkit_context *context, struct
nbdkit_next_ops **next_ops, void **nxdata);

which populates a filter-global next_ops and nxdata pointer so the
filter can later call shared_next_ops->FOO(shared_nxdata, ...) as
needed.  Filters that do not call nbdkit_context_associate() (including
implicitly via .open's next_open) will be passed next_ops==NULL in their
other various .foo functions (to force them to call through their
shared_next_ops object obtained earlier).

The reopen filter currently calls next_ops->reopen() to first close out
the existing context and then attempt to reopen a new one in its place.
But it seems a bit more logical that we would get rid of
next_ops->reopen(), and instead have .reopen first attempt to use
nbdkit_context_open() to open a new context, and if successful, then
call nbdkit_context_close() on the old context, and
nbdkit_context_associate (nxdata) to associate the new context into place.

The multi-conn filter still needs to track a list of all open contexts,
in order to call list[n]->next_ops->flush(list[n]->nxdata) across each
context.

I also note that at present, the 'struct nbdkit_next_ops * next_ops' we
pass to all filters is constant (just a list of function pointers, no
per-connection state); all the per-connection details are shared between
a 'struct backend *' (which backend to call into, part of nxdata both
before and after this patch) and a 'struct handle *' (state of whether
we are safe, and what pointer to pass to that backend - stored in TLS
prior to this series, but explicitly in nxdata afterwards).  The
combination of struct backend and struct handle is what forms an
nbdkit_context.  And since the function pointers in next_ops are always
the same, we could change the signature for ALL filters to have just one
pointer instead of 2 as the parameter to all .foo callbacks, changing:

int filter_pread (struct nbdkit_next_ops *next_ops, void *nxdata,
                  void *handle, ...)
into:
int filter_pread (nbdkit_context *nxdata, void *handle, ...)

where nxdata is NULL if nothing was associated during .open, and when
non-NULL, the filter uses nxdata->pread(nxdata, ...) to call into the
next context.  But that extra type-safety and change in signatures is
more churn across all the filters.  It would also mean we don't need
nbdkit_context_next_ops() proposed above.

I hope to post some patches later this week (maybe today?) that
demonstrate where I'm headed.


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