5:31 p.m.
The reopen operation (key to the retry filter) is enough of an oddball
that it does not belong as a function pointer in next_ops; in
particular, while most functions in next_ops operate on an existing
context, reopen can operate even after a previous failure has left no
context, but passing NULL for nxdata to represent no context does not
give us the information we need to associate the reopened context back
to the right connection. Wrap backend_reopen with a new exported
function nbdkit_backend_reopen, and drop the next_ops->reopen function
pointer. Note that the function is only declared for the retry filter
at this time, and that while at it, I made the retry filter use a bit
more type safety. Later patches will perform more mechanical
conversion of other filters to be type-safe.
The new function depends on guaranteeing that the 'nxdata' passed to
.open is stable for usage as the 'backend' parameter to pass to
nbdkit_backend_reopen, so I renamed that parameter in the docs. Our
testsuite coverage of the retry filter ensures we don't break this.
And it gets us one step closer to allowing plugins to open a context
into the backend without needing it to be tied to the current
connection from the client. In this patch, 'backend' of .open and
'nxdata' of .pread happen to be the same pointer and reopen doesn't
affect it, but in the upcoming patches, the two will be distinct
pointers, where nxdata can sometimes be NULL on entry to .pread and
set back to non-NULL by reopen.
Note that the new function is intended to be temporary while I
continue to make other refactorings. Ultimately, I want to get to the
point where any filter can open up a new context without regards to
the current connection, and then optionally associate any context into
the current connection (that is, the next_open callback passed to
.open will become shorthand for the paired action of
nbdkit_backend_open/nbdkit_backend_associate); once that is in place,
we don't need nbdkit_backend_reopen because the retry filter can just
directly open new connections as needed.
---
docs/nbdkit-filter.pod | 51 +++++++++++++++++++++++++++++------------
include/nbdkit-filter.h | 24 +++++++++++--------
server/backend.c | 6 +++--
server/filters.c | 12 ++++++++--
server/nbdkit.syms | 3 ++-
filters/retry/retry.c | 47 ++++++++++++++++++-------------------
6 files changed, 91 insertions(+), 52 deletions(-)
diff --git a/docs/nbdkit-filter.pod b/docs/nbdkit-filter.pod
index 65e56fe5..e801965d 100644
--- a/docs/nbdkit-filter.pod
+++ b/docs/nbdkit-filter.pod
@@ -132,16 +132,18 @@ C<nbdkit_next_after_fork>, C<nbdkit_next_preconnect>,
C<nbdkit_next_list_exports>, C<nbdkit_next_default_export>,
C<nbdkit_next_open>) and a structure called C<struct nbdkit_next_ops>.
These abstract the next plugin or filter in the chain. There is also
-an opaque pointer C<nxdata> which must be passed along when calling
-these functions. The value of C<nxdata> passed to C<.prepare> has a
-stable lifetime that lasts to the corresponding C<.finalize>, with all
-intermediate functions (such as C<.pread>) receiving the same value
-for convenience. Functions where C<nxdata> is not reused are
-C<.config>, C<.config_complete>, C<.get_ready>, and
C<.after_fork>,
-which are called during initialization outside any connections, and
-C<.preconnect>, C<.list_exports>, C<.default_export>, and
C<.open>
-which are called based on client connections but prior to the stable
-lifetime of C<.prepare>.
+an opaque pointer C<backend> or C<nxdata> which must be passed along
+when calling these functions. The value of C<nxdata> passed to
+C<.prepare> has a stable lifetime that lasts to the corresponding
+C<.finalize>, with all intermediate functions (such as C<.pread>)
+receiving the same value for convenience. Functions where C<nxdata>
+is not reused are C<.config>, C<.config_complete>, C<.get_ready>, and
+C<.after_fork>, which are called during initialization outside any
+connections, and C<.preconnect>, C<.list_exports>, C<.default_export>,
+and C<.open> which are called based on client connections but prior to
+the stable lifetime of C<.prepare>. The value of C<backend> passed to
+C<.open> has a lifetime that lasts until the matching C<.close> for
+use by C<nbdkit_backend_reopen>.
=head2 Next config, open and close
@@ -175,10 +177,6 @@ C<nbdkit_set_error> or setting C<errno>), whereas
C<next_ops-E<gt>pread> exposes this via an explicit parameter,
allowing a filter to learn or modify this error if desired.
-There is also a C<next_ops-E<gt>reopen> function which is used by
-L<nbdkit-retry-filter(1)> to close and reopen the underlying plugin.
-It should be used with caution because it is difficult to use safely.
-
=head2 Other considerations
You can modify parameters when you call the C<next> function. However
@@ -431,7 +429,7 @@ C<next> because it cannot be altered.
=head2 C<.open>
- void * (*open) (nbdkit_next_open *next, void *nxdata,
+ void * (*open) (nbdkit_next_open *next, nbdkit_backend *backend,
int readonly, const char *exportname, int is_tls);
This is called when a new client connection is opened and can be used
@@ -473,6 +471,29 @@ outer filter to the plugin will be in reverse. Skipping a call to
C<next> is acceptable if the filter will not access C<next_ops>
during any of the remaining callbacks reached on the same connection.
+The value of C<backend> in this call has a lifetime that lasts until
+the counterpart C<.close>, and it is this value (and not the distinct
+C<nxdata> of C<.pread> and friends) that must be passed as the first
+parameter to C<nbdkit_backend_reopen> by a filter attempting to retry
+a connection into the backend.
+
+=head3 Reopening the backend
+
+Filters have access to a function for reopening the backend:
+
+ int (nbdkit_backend *backend, int readonly, const char *exportname,
+ void **nxdata);
+
+This function is used by L<nbdkit-retry-filter(1)> to close and reopen
+the underlying plugin, with possible changes to the C<readonly> and
+C<exportname> parameters in relation to the original opening. It
+should be used with caution because it is difficult to use safely.
+The C<backend> parameter to this function should be the C<backend>
+parameter originally passed in to C<.open>; while the C<nxdata>
+pointer should be the address of C<nxdata> from any function with a
+C<next_ops> parameter (such as C<.pread>) that wants to call into the
+plugin after the reopen.
+
=head2 C<.close>
void (*close) (void *handle);
diff --git a/include/nbdkit-filter.h b/include/nbdkit-filter.h
index 0964c6e7..0c172936 100644
--- a/include/nbdkit-filter.h
+++ b/include/nbdkit-filter.h
@@ -1,5 +1,5 @@
/* nbdkit
- * Copyright (C) 2013-2020 Red Hat Inc.
+ * Copyright (C) 2013-2021 Red Hat Inc.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are
@@ -54,6 +54,8 @@ extern "C" {
* the next filter or plugin.
*/
typedef struct backend nbdkit_backend;
+#elif defined NBDKIT_RETRY_FILTER /* Hack to expose reopen to retry filter */
+typedef struct nbdkit_backend nbdkit_backend;
#else
typedef void nbdkit_backend;
#endif
@@ -69,16 +71,11 @@ typedef int nbdkit_next_list_exports (nbdkit_backend *nxdata, int
readonly,
struct nbdkit_exports *exports);
typedef const char *nbdkit_next_default_export (nbdkit_backend *nxdata,
int readonly);
-typedef int nbdkit_next_open (nbdkit_backend *nxdata,
+typedef int nbdkit_next_open (nbdkit_backend *backend,
int readonly, const char *exportname);
struct nbdkit_next_ops {
- /* Performs close + open on the underlying chain.
- * Used by the retry filter.
- */
- int (*reopen) (nbdkit_backend *nxdata, int readonly, const char *exportname);
-
- /* The rest of the next ops are the same as normal plugin operations. */
+ /* These callbacks are the same as normal plugin operations. */
int64_t (*get_size) (nbdkit_backend *nxdata);
const char * (*export_description) (nbdkit_backend *nxdata);
@@ -150,6 +147,15 @@ NBDKIT_EXTERN_DECL (size_t, nbdkit_exports_count,
NBDKIT_EXTERN_DECL (const struct nbdkit_export, nbdkit_get_export,
(const struct nbdkit_exports *, size_t));
+#ifdef NBDKIT_RETRY_FILTER
+/* Performs close + open on the underlying chain.
+ * Used by the retry filter.
+ */
+NBDKIT_EXTERN_DECL (int, nbdkit_backend_reopen,
+ (nbdkit_backend *backend, int readonly,
+ const char *exportname, nbdkit_backend **nxdata));
+#endif
+
/* Filter struct. */
struct nbdkit_filter {
/* Do not set these fields directly; use NBDKIT_REGISTER_FILTER.
@@ -190,7 +196,7 @@ struct nbdkit_filter {
nbdkit_backend *nxdata,
int readonly, int is_tls);
- void * (*open) (nbdkit_next_open *next, nbdkit_backend *nxdata,
+ void * (*open) (nbdkit_next_open *next, nbdkit_backend *backend,
int readonly, const char *exportname, int is_tls);
void (*close) (void *handle);
diff --git a/server/backend.c b/server/backend.c
index 3630163b..e9fcd696 100644
--- a/server/backend.c
+++ b/server/backend.c
@@ -1,5 +1,5 @@
/* nbdkit
- * Copyright (C) 2013-2020 Red Hat Inc.
+ * Copyright (C) 2013-2021 Red Hat Inc.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are
@@ -337,7 +337,7 @@ backend_valid_range (struct backend *b, uint64_t offset, uint32_t
count)
offset + count <= h->exportsize;
}
-/* Wrappers for all callbacks in a filter's struct nbdkit_next_ops. */
+/* Core functionality of nbdkit_backend_reopen for retry filter */
int
backend_reopen (struct backend *b, int readonly, const char *exportname)
@@ -360,6 +360,8 @@ backend_reopen (struct backend *b, int readonly, const char
*exportname)
return 0;
}
+/* Wrappers for all callbacks in a filter's struct nbdkit_next_ops. */
+
const char *
backend_export_description (struct backend *b)
{
diff --git a/server/filters.c b/server/filters.c
index 54abf9a4..31be9742 100644
--- a/server/filters.c
+++ b/server/filters.c
@@ -1,5 +1,5 @@
/* nbdkit
- * Copyright (C) 2013-2020 Red Hat Inc.
+ * Copyright (C) 2013-2021 Red Hat Inc.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are
@@ -290,7 +290,6 @@ filter_close (struct backend *b, void *handle)
}
static struct nbdkit_next_ops next_ops = {
- .reopen = backend_reopen,
.export_description = backend_export_description,
.get_size = backend_get_size,
.can_write = backend_can_write,
@@ -659,3 +658,12 @@ filter_register (struct backend *next, size_t index, const char
*filename,
return (struct backend *) f;
}
+
+int
+nbdkit_backend_reopen (struct backend *b, int readonly,
+ const char *exportname, struct backend **nxdata)
+{
+ /* For now, we don't need to update nxdata. */
+ assert (b == *nxdata);
+ return backend_reopen (b, readonly, exportname);
+}
diff --git a/server/nbdkit.syms b/server/nbdkit.syms
index 20ee27f3..50411d27 100644
--- a/server/nbdkit.syms
+++ b/server/nbdkit.syms
@@ -1,5 +1,5 @@
# nbdkit
-# Copyright (C) 2018-2020 Red Hat Inc.
+# Copyright (C) 2018-2021 Red Hat Inc.
#
# Redistribution and use in source and binary forms, with or without
# modification, are permitted provided that the following conditions are
@@ -41,6 +41,7 @@
nbdkit_absolute_path;
nbdkit_add_export;
nbdkit_add_extent;
+ nbdkit_backend_reopen;
nbdkit_debug;
nbdkit_error;
nbdkit_export_name;
diff --git a/filters/retry/retry.c b/filters/retry/retry.c
index 7377fdf4..abd49eb8 100644
--- a/filters/retry/retry.c
+++ b/filters/retry/retry.c
@@ -40,6 +40,7 @@
#include <string.h>
#include <sys/time.h>
+#define NBDKIT_RETRY_FILTER /* Hack to expose reopen */
#include <nbdkit-filter.h>
#include "cleanup.h"
@@ -50,7 +51,7 @@ static unsigned initial_delay = 2;
static bool exponential_backoff = true;
static bool force_readonly = false;
-/* Currently next_ops->reopen is not safe if another thread makes a
+/* Currently nbdkit_backend_reopen is not safe if another thread makes a
* request on the same connection (but on other connections it's OK).
* To work around this for now we limit the thread model here, but
* this is something we could improve in server/backend.c in future.
@@ -62,7 +63,7 @@ retry_thread_model (void)
}
static int
-retry_config (nbdkit_next_config *next, void *nxdata,
+retry_config (nbdkit_next_config *next, nbdkit_backend *nxdata,
const char *key, const char *value)
{
int r;
@@ -108,12 +109,13 @@ retry_config (nbdkit_next_config *next, void *nxdata,
struct retry_handle {
int readonly; /* Save original readonly setting. */
char *exportname; /* Client exportname. */
+ nbdkit_backend *backend; /* Backend learned during .open. */
unsigned reopens;
bool open;
};
static void *
-retry_open (nbdkit_next_open *next, void *nxdata,
+retry_open (nbdkit_next_open *next, nbdkit_backend *nxdata,
int readonly, const char *exportname, int is_tls)
{
struct retry_handle *h;
@@ -129,6 +131,7 @@ retry_open (nbdkit_next_open *next, void *nxdata,
h->readonly = readonly;
h->exportname = strdup (exportname);
+ h->backend = nxdata;
if (h->exportname == NULL) {
nbdkit_error ("strdup: %m");
free (h);
@@ -172,10 +175,8 @@ valid_range (struct nbdkit_next_ops *next_ops, void *nxdata,
}
static bool
-do_retry (struct retry_handle *h,
- struct retry_data *data,
- struct nbdkit_next_ops *next_ops, void *nxdata,
- const char *method, int *err)
+do_retry (struct retry_handle *h, struct retry_data *data,
+ nbdkit_backend **nxdata, const char *method, int *err)
{
/* If it's the first retry, initialize the other fields in *data. */
if (data->retry == 0)
@@ -208,8 +209,8 @@ do_retry (struct retry_handle *h,
/* Reopen the connection. */
h->reopens++;
- if (next_ops->reopen (nxdata,
- h->readonly || force_readonly, h->exportname) == -1) {
+ if (nbdkit_backend_reopen (h->backend, h->readonly || force_readonly,
+ h->exportname, nxdata) == -1) {
/* If the reopen fails we treat it the same way as a command
* failing.
*/
@@ -224,7 +225,7 @@ do_retry (struct retry_handle *h,
}
static int
-retry_pread (struct nbdkit_next_ops *next_ops, void *nxdata,
+retry_pread (struct nbdkit_next_ops *next_ops, nbdkit_backend *nxdata,
void *handle, void *buf, uint32_t count, uint64_t offset,
uint32_t flags, int *err)
{
@@ -237,7 +238,7 @@ retry_pread (struct nbdkit_next_ops *next_ops, void *nxdata,
r = -1;
else
r = next_ops->pread (nxdata, buf, count, offset, flags, err);
- if (r == -1 && do_retry (h, &data, next_ops, nxdata, "pread",
err))
+ if (r == -1 && do_retry (h, &data, &nxdata, "pread", err))
goto again;
return r;
@@ -245,7 +246,7 @@ retry_pread (struct nbdkit_next_ops *next_ops, void *nxdata,
/* Write. */
static int
-retry_pwrite (struct nbdkit_next_ops *next_ops, void *nxdata,
+retry_pwrite (struct nbdkit_next_ops *next_ops, nbdkit_backend *nxdata,
void *handle,
const void *buf, uint32_t count, uint64_t offset,
uint32_t flags, int *err)
@@ -272,7 +273,7 @@ retry_pwrite (struct nbdkit_next_ops *next_ops, void *nxdata,
}
else
r = next_ops->pwrite (nxdata, buf, count, offset, flags, err);
- if (r == -1 && do_retry (h, &data, next_ops, nxdata, "pwrite",
err))
+ if (r == -1 && do_retry (h, &data, &nxdata, "pwrite", err))
goto again;
return r;
@@ -280,7 +281,7 @@ retry_pwrite (struct nbdkit_next_ops *next_ops, void *nxdata,
/* Trim. */
static int
-retry_trim (struct nbdkit_next_ops *next_ops, void *nxdata,
+retry_trim (struct nbdkit_next_ops *next_ops, nbdkit_backend *nxdata,
void *handle,
uint32_t count, uint64_t offset, uint32_t flags,
int *err)
@@ -307,7 +308,7 @@ retry_trim (struct nbdkit_next_ops *next_ops, void *nxdata,
}
else
r = next_ops->trim (nxdata, count, offset, flags, err);
- if (r == -1 && do_retry (h, &data, next_ops, nxdata, "trim",
err))
+ if (r == -1 && do_retry (h, &data, &nxdata, "trim", err))
goto again;
return r;
@@ -315,7 +316,7 @@ retry_trim (struct nbdkit_next_ops *next_ops, void *nxdata,
/* Flush. */
static int
-retry_flush (struct nbdkit_next_ops *next_ops, void *nxdata,
+retry_flush (struct nbdkit_next_ops *next_ops, nbdkit_backend *nxdata,
void *handle, uint32_t flags,
int *err)
{
@@ -332,7 +333,7 @@ retry_flush (struct nbdkit_next_ops *next_ops, void *nxdata,
}
else
r = next_ops->flush (nxdata, flags, err);
- if (r == -1 && do_retry (h, &data, next_ops, nxdata, "flush",
err))
+ if (r == -1 && do_retry (h, &data, &nxdata, "flush", err))
goto again;
return r;
@@ -340,7 +341,7 @@ retry_flush (struct nbdkit_next_ops *next_ops, void *nxdata,
/* Zero. */
static int
-retry_zero (struct nbdkit_next_ops *next_ops, void *nxdata,
+retry_zero (struct nbdkit_next_ops *next_ops, nbdkit_backend *nxdata,
void *handle,
uint32_t count, uint64_t offset, uint32_t flags,
int *err)
@@ -372,7 +373,7 @@ retry_zero (struct nbdkit_next_ops *next_ops, void *nxdata,
}
else
r = next_ops->zero (nxdata, count, offset, flags, err);
- if (r == -1 && do_retry (h, &data, next_ops, nxdata, "zero",
err))
+ if (r == -1 && do_retry (h, &data, &nxdata, "zero", err))
goto again;
return r;
@@ -380,7 +381,7 @@ retry_zero (struct nbdkit_next_ops *next_ops, void *nxdata,
/* Extents. */
static int
-retry_extents (struct nbdkit_next_ops *next_ops, void *nxdata,
+retry_extents (struct nbdkit_next_ops *next_ops, nbdkit_backend *nxdata,
void *handle,
uint32_t count, uint64_t offset, uint32_t flags,
struct nbdkit_extents *extents, int *err)
@@ -408,7 +409,7 @@ retry_extents (struct nbdkit_next_ops *next_ops, void *nxdata,
}
r = next_ops->extents (nxdata, count, offset, flags, extents2, err);
}
- if (r == -1 && do_retry (h, &data, next_ops, nxdata, "extents",
err))
+ if (r == -1 && do_retry (h, &data, &nxdata, "extents", err))
goto again;
if (r == 0) {
@@ -428,7 +429,7 @@ retry_extents (struct nbdkit_next_ops *next_ops, void *nxdata,
/* Cache. */
static int
-retry_cache (struct nbdkit_next_ops *next_ops, void *nxdata,
+retry_cache (struct nbdkit_next_ops *next_ops, nbdkit_backend *nxdata,
void *handle,
uint32_t count, uint64_t offset, uint32_t flags,
int *err)
@@ -446,7 +447,7 @@ retry_cache (struct nbdkit_next_ops *next_ops, void *nxdata,
}
else
r = next_ops->cache (nxdata, count, offset, flags, err);
- if (r == -1 && do_retry (h, &data, next_ops, nxdata, "cache",
err))
+ if (r == -1 && do_retry (h, &data, &nxdata, "cache", err))
goto again;
return r;
--
2.30.1