[Libguestfs] [PATCH nbdkit v2 2/4] filters: Implement next_ops .reopen call.

[Eric Blake]

On 9/19/19 9:33 AM, Richard W.M. Jones wrote:
> This is intended for use by the forthcoming retry filter to close and
> reopen the backend chain.  It is handled entirely by server/backend.c
> as no cooperation is needed with the plugin.
> 
> Note the explicit readonly parameter: An alternative would be to store
> the previous readonly setting in the b_conn_handle struct.  However
> passing it explicitly allows the retry filter to retry as readonly,
> which might be useful.  This design does however require any filter
> which might call .reopen to save the original readonly parameter from
> the .open call.
> ---
>  include/nbdkit-filter.h | 6 ++++++
>  server/backend.c        | 9 +++++++++
>  server/filters.c        | 8 ++++++++
>  server/internal.h       | 4 ++++
>  4 files changed, 27 insertions(+)
> 

> +++ b/include/nbdkit-filter.h
> @@ -68,6 +68,12 @@ typedef int nbdkit_next_open (void *nxdata,
>                                int readonly);
>  
>  struct nbdkit_next_ops {
> +  /* Performs close + open on the underlying chain.
> +   * Used by the retry filter.
> +   */
> +  int (*reopen) (void *nxdata, int readonly);
> +

ABI change - but we've solved that with our version match check. No
further header version bumps needed :)


> +++ b/server/backend.c
> @@ -233,6 +233,15 @@ backend_valid_range (struct backend *b, struct connection *conn,
>  
>  /* Wrappers for all callbacks in a filter's struct nbdkit_next_ops. */
>  
> +int
> +backend_reopen (struct backend *b, struct connection *conn, int readonly)
> +{
> +  debug ("%s: reopen", b->name);

I'd also debug the value of readonly=%d here.

> +
> +  backend_close (b, conn);
> +  return backend_open (b, conn, readonly);
> +}

Your followup patch about only calling backend_close if h->handle is
non-NULL is correct.

Also, this gets us into weird territory.  Previously, we could claim
that if handles[0]->handle is non-NULL, then when the connection closes,
we must call backend_close().  But now, if backend_reopen() fails, then
handls[0]->handle is left NULL but we still have an open handle (namely,
the retry filter and anything above it) that still need cleanup, or else
we have a memory leak.  We'll have to figure out how to ensure that no
matter what state we end up in, we properly call .close on all currently
open backends.

As it is, I'm also starting to worry that we may need to track whether
.prepare succeeded, and only call .finalize on success.  We may want to
extend struct b_conn_handle to track an enum stating the handle's
current lifecycle status, to ensure that .close is never called except
after successful .open, and .finalize is never called except after
successful .prepare.

> 
> +++ b/server/internal.h
> @@ -307,6 +307,7 @@ struct backend {
>    int (*prepare) (struct backend *, struct connection *conn, int readonly);
>    int (*finalize) (struct backend *, struct connection *conn);
>    void (*close) (struct backend *, struct connection *conn);
> +  int (*reopen) (struct backend *, struct connection *conn);

Why is this needed?  We don't provide a .reopen callback for either
plugins or filters (filters use next_ops->reopen, but don't override it
themselves).

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20190919/558f36ed/attachment.sig>