This is a revision of my .default_export work, plus new work on .export_descriptions and a new exportname filter. I think it is now ready to check in. Things I'd still like in 1.22: - the file plugin should implement .list_exports (patch already posted, but it needs rebasing on this series) - the ext2 filter should override .list_exports when in exportname mode - the nbd plugin should be able to use libnbd 1.4 features for .list_exports - more language bindings for new functions (perl, python, ocaml, ...) nice to have, but can be post-release: - the file plugin in dir= mode should have default=none|largest (default behavior none says "" is an error because it is not a file, setting it to largest picks the largest file in the directory for "") - the tar filter should consider an exportname mode - figure out how to let filters open the plugin without a current connection into the filter Eric Blake (8): api: Add .default_export api: Add nbdkit_add_default_export server: Respond to NBD_INFO_NAME request api: Add nbdkit_str[n]dup_intern helper sh, eval: Implement .default_export api: Alter .list_exports api: Add .export_description exportname: New filter docs/nbdkit-filter.pod | 56 ++- docs/nbdkit-plugin.pod | 123 ++++++- docs/nbdkit-protocol.pod | 14 + .../exportname/nbdkit-exportname-filter.pod | 154 ++++++++ filters/ext2/nbdkit-ext2-filter.pod | 5 + filters/log/nbdkit-log-filter.pod | 2 +- plugins/eval/nbdkit-eval-plugin.pod | 6 + plugins/file/nbdkit-file-plugin.pod | 9 +- plugins/sh/nbdkit-sh-plugin.pod | 39 +- include/nbdkit-common.h | 6 + include/nbdkit-filter.h | 13 +- include/nbdkit-plugin.h | 4 +- configure.ac | 2 + filters/exportname/Makefile.am | 67 ++++ tests/Makefile.am | 8 +- server/internal.h | 22 +- server/backend.c | 93 +++-- server/connections.c | 2 +- server/exports.c | 32 +- server/filters.c | 31 +- server/main.c | 26 +- server/nbdkit.syms | 3 + server/plugins.c | 44 ++- server/protocol-handshake-newstyle.c | 87 ++++- server/public.c | 48 +++ server/test-public.c | 9 +- plugins/sh/methods.h | 2 + plugins/cc/cc.c | 73 ++-- plugins/eval/eval.c | 70 ++-- plugins/ondemand/ondemand.c | 11 +- plugins/sh/methods.c | 84 ++++- plugins/sh/sh.c | 70 ++-- plugins/sh/example.sh | 2 +- filters/exportname/exportname.c | 342 ++++++++++++++++++ filters/log/log.c | 18 +- filters/tls-fallback/tls-fallback.c | 60 ++- tests/test-eval-exports.sh | 34 +- tests/test-export-info.sh | 111 ++++++ tests/test-exportname.sh | 184 ++++++++++ tests/test-tls-fallback.sh | 53 ++- tests/test-layers-filter.c | 13 +- tests/test-layers-plugin.c | 8 + tests/test-layers.c | 24 +- TODO | 20 +- 44 files changed, 1790 insertions(+), 294 deletions(-) create mode 100644 filters/exportname/nbdkit-exportname-filter.pod create mode 100644 filters/exportname/Makefile.am create mode 100644 filters/exportname/exportname.c create mode 100755 tests/test-export-info.sh create mode 100755 tests/test-exportname.sh -- 2.28.0
Eric Blake
2020-Aug-27 02:16 UTC
[Libguestfs] [nbdkit PATCH v2 1/8] api: Add .default_export
I'm about to add an 'exportname' filter, and in the process, I
noticed
a few shortcomings in our API. Time to fix those before the 1.22
release locks our API in stone. Overloading .list_exports in order to
determine a canonical export name at .open time is awkward; the two
uses (answering NBD_OPT_LIST for a full list, vs. remapping a client's
"" into a canonical name during .open) are orthogonal enough to
warrant separate plugin callbacks. This will also make it easier to
express the notion of no default export (connecting to "" is an error)
at the same time as listing other exports. Another consideration is
that when tls=1, the choice of export to expose pre-TLS vs. post-TLS
may differ, but without a call to .open yet, our just-added
nbdkit_is_tls() does not fit our preferred lifecycle, so this has to
be a parameter to the new .default_export. We will alter the
signature of .list_exports soon; in the meantime, the bool
default_only parameter is now ignored.
Adding .default_export support to sh/eval is big enough for a separate
patch.
The ondemand plugin continues to advertise "" (and not
"default") in
its list of exports, but now reports a canonical name of "default"
when connecting to it.
The cc plugin gets .default_export now; it will get .list_exports when
we fix that signature.
The tls-fallback filter can now avoid leaking a default export name
through an insecure connection.
The test-layers.c changes demonstrate how fragile it is to maintain
our own naive client for probing which aspects of the plugin have been
reached; the test could probably be improved by using libnbd 1.4 APIs.
---
docs/nbdkit-filter.pod | 28 ++++++++++-----
docs/nbdkit-plugin.pod | 36 ++++++++++++++++++++
plugins/eval/nbdkit-eval-plugin.pod | 2 ++
include/nbdkit-filter.h | 9 +++--
include/nbdkit-plugin.h | 1 +
server/internal.h | 7 ++--
server/backend.c | 51 ++++++++++++++++++++--------
server/exports.c | 8 +----
server/filters.c | 12 +++++++
server/plugins.c | 14 +++++++-
server/protocol-handshake-newstyle.c | 9 ++++-
plugins/cc/cc.c | 9 +++++
plugins/ondemand/ondemand.c | 11 ++++--
filters/tls-fallback/tls-fallback.c | 12 ++++++-
tests/test-eval-exports.sh | 3 +-
tests/test-layers-filter.c | 9 +++++
tests/test-layers-plugin.c | 8 +++++
tests/test-layers.c | 16 ++-------
18 files changed, 192 insertions(+), 53 deletions(-)
diff --git a/docs/nbdkit-filter.pod b/docs/nbdkit-filter.pod
index 94cebccd..dd667c0c 100644
--- a/docs/nbdkit-filter.pod
+++ b/docs/nbdkit-filter.pod
@@ -137,9 +137,9 @@ C<nxdata> passed to C<.open> has a stable
lifetime that lasts to the
corresponding C<.close>, with all intermediate functions (such as
C<.pread>) receiving the same value for convenience; the only
exceptions where C<nxdata> is not reused are C<.config>,
-C<.config_complete>, C<.get_ready>, C<.after_fork>,
C<.preconnect> and
-C<.list_exports>, which are called outside the lifetime of a
-connection.
+C<.config_complete>, C<.get_ready>, C<.after_fork>,
C<.preconnect>,
+C<.list_exports>, and C<.default_export>, which are called outside
+the lifetime of a connection.
=head2 Next config, open and close
@@ -355,7 +355,7 @@ from the layer below. Without error checking it would look
like this:
struct nbdkit_export e;
char *name, *desc;
- exports2 = nbdkit_exports_new (default_only);
+ exports2 = nbdkit_exports_new ();
next_list_exports (nxdata, readonly, default_only, exports);
for (i = 0; i < nbdkit_exports_count (exports2); ++i) {
e = nbdkit_get_export (exports2, i);
@@ -376,11 +376,9 @@ an error message and return C<-1>.
Two functions are provided to filters only for allocating and freeing
the list:
- struct nbdkit_exports *nbdkit_exports_new (int default_only);
+ struct nbdkit_exports *nbdkit_exports_new (void);
-Allocates and returns a new, empty exports list. The C<default_only>
-parameter should match whether the list is intended to grab the
-canonical name of the default export, or all exports.
+Allocates and returns a new, empty exports list.
On error this function can return C<NULL>. In this case it calls
C<nbdkit_error> as required. C<errno> will be set to a suitable
@@ -408,6 +406,20 @@ Returns the number of exports in the list.
Returns a copy of the C<i>'th export.
+=head2 C<.default_export>
+
+ const char *default_export (nbdkit_next_default_export *next, void *nxdata,
+ int readonly, int is_tls)
+
+This intercepts the plugin C<.default_export> method and can be used to
+alter the canonical export name used in place of the default
C<"">.
+
+The C<readonly> parameter matches what is passed to <.preconnect>
and
+C<.open>, and may be changed by the filter when calling into the
+plugin. The C<is_tls> parameter informs the filter whether TLS
+negotiation has been completed by the client, but is not passed on to
+C<next> because it cannot be altered.
+
=head2 C<.open>
void * (*open) (nbdkit_next_open *next, void *nxdata,
diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod
index 5c641e83..1f208b27 100644
--- a/docs/nbdkit-plugin.pod
+++ b/docs/nbdkit-plugin.pod
@@ -245,6 +245,12 @@ Early in option negotiation the client may try to list the
exports
served by the plugin, and plugins can optionally implement this
callback to answer the client. See L</EXPORT NAME> below.
+=item C<.default_export>
+
+During option negotiation, if the client requests the default export
+name (C<"">), this optional callback provides a canonical name
to
+use in its place prior to calling C<.open>.
+
=item C<.open>
A new client has connected and finished the NBD handshake. TLS
@@ -709,6 +715,36 @@ Returning C<0> will send the list of exports back to
the client. If
there is an error, C<.list_exports> should call C<nbdkit_error>
with
an error message and return C<-1>.
+=head2 C<.default_export>
+
+ const char *default_export (int readonly, int is_tls);
+
+This optional callback is called if the client tries to connect to the
+default export C<"">, where the plugin provides a UTF-8 string
between
+0 and 4096 bytes. If the plugin does not supply this callback, the
+connection continues with the empty name; if the plugin returns a
+valid string, nbdkit returns that name to clients that support it (see
+the C<NBD_INFO_NAME> response to C<NBD_OPT_GO>), and behaves as if
the
+client had passed that string instead of an empty name.
+
+The C<readonly> flag informs the plugin that the server was started
+with the I<-r> flag on the command line, which is the same value
+passed to C<.preconnect> and C<.open>. However, the NBD protocol
does
+not yet have a way to let the client advertise an intent to be
+read-only even when the server allows writes, so this parameter may
+not be as useful as it appears.
+
+The C<is_tls> flag informs the plugin whether the canonical name for
+the default export is being requested after the client has completed
+TLS negotiation. When running the server in a mode that permits but
+does not require TLS, be careful that a default export name does not
+leak unintended information.
+
+If the plugin returns C<NULL> or an invalid string (such as longer
+than 4096 bytes), the client is not permitted to connect to the
+default export. However, this is not an error in the protocol, so it
+is not necessary to call C<nbdkit_error>.
+
=head2 C<.open>
void *open (int readonly);
diff --git a/plugins/eval/nbdkit-eval-plugin.pod
b/plugins/eval/nbdkit-eval-plugin.pod
index 7126c6de..baf79adf 100644
--- a/plugins/eval/nbdkit-eval-plugin.pod
+++ b/plugins/eval/nbdkit-eval-plugin.pod
@@ -96,6 +96,8 @@ features):
=item B<config_complete=>SCRIPT
+=item B<default_export=>SCRIPT
+
=item B<dump_plugin=>SCRIPT
=item B<extents=>SCRIPT
diff --git a/include/nbdkit-filter.h b/include/nbdkit-filter.h
index b4024ae5..2c5b36be 100644
--- a/include/nbdkit-filter.h
+++ b/include/nbdkit-filter.h
@@ -66,8 +66,10 @@ typedef int nbdkit_next_get_ready (nbdkit_backend *nxdata);
typedef int nbdkit_next_after_fork (nbdkit_backend *nxdata);
typedef int nbdkit_next_preconnect (nbdkit_backend *nxdata, int readonly);
typedef int nbdkit_next_list_exports (nbdkit_backend *nxdata, int readonly,
- int default_only,
+ int ignored,
struct nbdkit_exports *exports);
+typedef const char *nbdkit_next_default_export (nbdkit_backend *nxdata,
+ int readonly);
typedef int nbdkit_next_open (nbdkit_backend *nxdata,
int readonly, const char *exportname);
@@ -136,7 +138,7 @@ struct nbdkit_export {
};
NBDKIT_EXTERN_DECL (struct nbdkit_exports *, nbdkit_exports_new,
- (int default_only));
+ (void));
NBDKIT_EXTERN_DECL (void, nbdkit_exports_free, (struct nbdkit_exports *));
NBDKIT_EXTERN_DECL (size_t, nbdkit_exports_count,
(const struct nbdkit_exports *));
@@ -179,6 +181,9 @@ struct nbdkit_filter {
int (*list_exports) (nbdkit_next_list_exports *next, nbdkit_backend *nxdata,
int readonly, int default_only,
struct nbdkit_exports *exports);
+ const char * (*default_export) (nbdkit_next_default_export *next,
+ nbdkit_backend *nxdata,
+ int readonly, int is_tls);
void * (*open) (nbdkit_next_open *next, nbdkit_backend *nxdata,
int readonly, const char *exportname, int is_tls);
diff --git a/include/nbdkit-plugin.h b/include/nbdkit-plugin.h
index a5d85411..28e83757 100644
--- a/include/nbdkit-plugin.h
+++ b/include/nbdkit-plugin.h
@@ -142,6 +142,7 @@ struct nbdkit_plugin {
int (*list_exports) (int readonly, int default_only,
struct nbdkit_exports *exports);
+ const char * (*default_export) (int readonly, int is_tls);
};
NBDKIT_EXTERN_DECL (void, nbdkit_set_error, (int err));
diff --git a/server/internal.h b/server/internal.h
index d043225a..8c8448e6 100644
--- a/server/internal.h
+++ b/server/internal.h
@@ -365,8 +365,9 @@ struct backend {
void (*get_ready) (struct backend *);
void (*after_fork) (struct backend *);
int (*preconnect) (struct backend *, int readonly);
- int (*list_exports) (struct backend *, int readonly, int default_only,
+ int (*list_exports) (struct backend *, int readonly, int ignored,
struct nbdkit_exports *exports);
+ const char *(*default_export) (struct backend *, int readonly, int is_tls);
void *(*open) (struct backend *, int readonly, const char *exportname,
int is_tls);
int (*prepare) (struct backend *, void *handle, int readonly);
@@ -413,9 +414,11 @@ extern void backend_unload (struct backend *b, void
(*unload) (void))
__attribute__((__nonnull__ (1)));
extern int backend_list_exports (struct backend *b, int readonly,
- int default_only,
+ int ignored,
struct nbdkit_exports *exports)
__attribute__((__nonnull__ (1, 4)));
+extern const char *backend_default_export (struct backend *b, int readonly)
+ __attribute__((__nonnull__ (1)));
/* exportname is only valid for this call and almost certainly will be
* freed on return of this function, so backends must save the
* exportname if they need to refer to it later.
diff --git a/server/backend.c b/server/backend.c
index 2ca40d61..00e65e3c 100644
--- a/server/backend.c
+++ b/server/backend.c
@@ -166,13 +166,12 @@ backend_list_exports (struct backend *b, int readonly, int
default_only,
struct handle *h = get_handle (conn, b->i);
int r;
+ assert (!default_only); /* XXX Switch to is_tls... */
controlpath_debug ("%s: list_exports readonly=%d default_only=%d",
b->name, readonly, default_only);
assert (h->handle == NULL);
assert ((h->state & HANDLE_OPEN) == 0);
- if (default_only && h->default_exportname)
- return nbdkit_add_export (exports, h->default_exportname, NULL);
r = b->list_exports (b, readonly, default_only, exports);
if (r == -1)
@@ -180,13 +179,40 @@ backend_list_exports (struct backend *b, int readonly, int
default_only,
else {
size_t count = nbdkit_exports_count (exports);
controlpath_debug ("%s: list_exports returned %zu names",
b->name, count);
- /* Best effort caching of default export name */
- if (!h->default_exportname && count)
- h->default_exportname = strdup (nbdkit_get_export (exports, 0).name);
}
return r;
}
+const char *
+backend_default_export (struct backend *b, int readonly)
+{
+ GET_CONN;
+ struct handle *h = get_handle (conn, b->i);
+ const char *s;
+
+ controlpath_debug ("%s: default_export readonly=%d tls=%d",
+ b->name, readonly, conn->using_tls);
+
+ if (h->default_exportname == NULL) {
+ assert (h->handle == NULL);
+ assert ((h->state & HANDLE_OPEN) == 0);
+ s = b->default_export (b, readonly, conn->using_tls);
+ /* Ignore over-length strings. XXX Also ignore non-UTF8? */
+ if (s && strnlen (s, NBD_MAX_STRING + 1) > NBD_MAX_STRING) {
+ controlpath_debug ("%s: default_export: ignoring invalid
string",
+ b->name);
+ s = NULL;
+ }
+ if (s) {
+ /* Best effort caching */
+ h->default_exportname = strdup (s);
+ if (h->default_exportname == NULL)
+ return s;
+ }
+ }
+ return h->default_exportname;
+}
+
int
backend_open (struct backend *b, int readonly, const char *exportname)
{
@@ -202,18 +228,13 @@ backend_open (struct backend *b, int readonly, const char
*exportname)
if (readonly)
h->can_write = 0;
- /* Best-effort determination of the canonical name for default export */
+ /* Determine the canonical name for default export */
if (!*exportname) {
- if (!h->default_exportname) {
- CLEANUP_EXPORTS_FREE struct nbdkit_exports *exps = NULL;
-
- exps = nbdkit_exports_new (true);
- if (exps && b->list_exports (b, readonly, true, exps) == 0
&&
- nbdkit_exports_count (exps))
- h->default_exportname = strdup (nbdkit_get_export (exps, 0).name);
+ exportname = backend_default_export (b, readonly);
+ if (exportname == NULL) {
+ nbdkit_error ("default export (\"\") not permitted");
+ return -1;
}
- if (h->default_exportname)
- exportname = h->default_exportname;
}
/* Most filters will call next_open first, resulting in
diff --git a/server/exports.c b/server/exports.c
index 3f819622..8d3faec4 100644
--- a/server/exports.c
+++ b/server/exports.c
@@ -52,12 +52,10 @@ DEFINE_VECTOR_TYPE(exports, struct nbdkit_export);
struct nbdkit_exports {
exports exports;
-
- bool default_only;
};
struct nbdkit_exports *
-nbdkit_exports_new (int default_only)
+nbdkit_exports_new (void)
{
struct nbdkit_exports *r;
@@ -67,7 +65,6 @@ nbdkit_exports_new (int default_only)
return NULL;
}
r->exports = (exports) empty_vector;
- r->default_only = default_only != 0;
return r;
}
@@ -107,9 +104,6 @@ nbdkit_add_export (struct nbdkit_exports *exps,
{
struct nbdkit_export e = { NULL, NULL };
- if (exps->default_only && exps->exports.size == 1)
- return 0;
-
if (exps->exports.size == MAX_EXPORTS) {
nbdkit_error ("nbdkit_add_export: too many exports");
errno = EINVAL;
diff --git a/server/filters.c b/server/filters.c
index 0cfae344..bb22be76 100644
--- a/server/filters.c
+++ b/server/filters.c
@@ -249,6 +249,17 @@ filter_list_exports (struct backend *b, int readonly, int
default_only,
return backend_list_exports (b->next, readonly, default_only, exports);
}
+static const char *
+filter_default_export (struct backend *b, int readonly, int is_tls)
+{
+ struct backend_filter *f = container_of (b, struct backend_filter, backend);
+
+ if (f->filter.default_export)
+ return f->filter.default_export (backend_default_export, b->next,
+ readonly, is_tls);
+ return backend_default_export (b->next, readonly);
+}
+
static void *
filter_open (struct backend *b, int readonly, const char *exportname,
int is_tls)
@@ -555,6 +566,7 @@ static struct backend filter_functions = {
.after_fork = filter_after_fork,
.preconnect = filter_preconnect,
.list_exports = filter_list_exports,
+ .default_export = filter_default_export,
.open = filter_open,
.prepare = filter_prepare,
.finalize = filter_finalize,
diff --git a/server/plugins.c b/server/plugins.c
index 4e320868..8172759f 100644
--- a/server/plugins.c
+++ b/server/plugins.c
@@ -163,6 +163,7 @@ plugin_dump_fields (struct backend *b)
HAS (after_fork);
HAS (preconnect);
HAS (list_exports);
+ HAS (default_export);
HAS (open);
HAS (close);
@@ -285,7 +286,6 @@ static int
plugin_list_exports (struct backend *b, int readonly, int default_only,
struct nbdkit_exports *exports)
{
- GET_CONN;
struct backend_plugin *p = container_of (b, struct backend_plugin, backend);
if (!p->plugin.list_exports)
@@ -294,6 +294,17 @@ plugin_list_exports (struct backend *b, int readonly, int
default_only,
return p->plugin.list_exports (readonly, default_only, exports);
}
+static const char *
+plugin_default_export (struct backend *b, int readonly, int is_tls)
+{
+ struct backend_plugin *p = container_of (b, struct backend_plugin, backend);
+
+ if (!p->plugin.default_export)
+ return "";
+
+ return p->plugin.default_export (readonly, is_tls);
+}
+
static void *
plugin_open (struct backend *b, int readonly, const char *exportname,
int is_tls)
@@ -761,6 +772,7 @@ static struct backend plugin_functions = {
.after_fork = plugin_after_fork,
.preconnect = plugin_preconnect,
.list_exports = plugin_list_exports,
+ .default_export = plugin_default_export,
.open = plugin_open,
.prepare = plugin_prepare,
.finalize = plugin_finalize,
diff --git a/server/protocol-handshake-newstyle.c
b/server/protocol-handshake-newstyle.c
index 8f41c7a8..6e5f3822 100644
--- a/server/protocol-handshake-newstyle.c
+++ b/server/protocol-handshake-newstyle.c
@@ -85,7 +85,7 @@ send_newstyle_option_reply_exportnames (uint32_t option)
size_t i;
CLEANUP_EXPORTS_FREE struct nbdkit_exports *exps = NULL;
- exps = nbdkit_exports_new (false);
+ exps = nbdkit_exports_new ();
if (exps == NULL)
return send_newstyle_option_reply (option, NBD_REP_ERR_TOO_BIG);
if (backend_list_exports (top, read_only, false, exps) == -1)
@@ -301,6 +301,7 @@ negotiate_handshake_newstyle_options (void)
struct nbd_export_name_option_reply handshake_finish;
const char *optname;
uint64_t exportsize;
+ struct backend *b;
for (nr_options = 0; nr_options < MAX_NR_OPTIONS; ++nr_options) {
CLEANUP_FREE char *data = NULL;
@@ -445,6 +446,12 @@ negotiate_handshake_newstyle_options (void)
return -1;
conn->using_tls = true;
debug ("using TLS on this connection");
+ /* Wipe out any cached default export name. */
+ for_each_backend (b) {
+ struct handle *h = get_handle (conn, b->i);
+ free (h->default_exportname);
+ h->default_exportname = NULL;
+ }
}
break;
diff --git a/plugins/cc/cc.c b/plugins/cc/cc.c
index 807ffcb6..1d688056 100644
--- a/plugins/cc/cc.c
+++ b/plugins/cc/cc.c
@@ -348,6 +348,14 @@ cc_preconnect (int readonly)
return 0;
}
+static const char *
+cc_default_export (int readonly, int is_tls)
+{
+ if (subplugin.default_export)
+ return subplugin.default_export (readonly, is_tls);
+ return "";
+}
+
static void *
cc_open (int readonly)
{
@@ -563,6 +571,7 @@ static struct nbdkit_plugin plugin = {
.after_fork = cc_after_fork,
.preconnect = cc_preconnect,
+ .default_export = cc_default_export,
.open = cc_open,
.close = cc_close,
diff --git a/plugins/ondemand/ondemand.c b/plugins/ondemand/ondemand.c
index f2fcb68e..51de54cc 100644
--- a/plugins/ondemand/ondemand.c
+++ b/plugins/ondemand/ondemand.c
@@ -223,6 +223,13 @@ ondemand_list_exports (int readonly, int default_only,
return 0;
}
+static const char *
+ondemand_default_export (int readonly, int is_tls)
+{
+ /* We always accept "" as an export name; canonicalize it to
"default". */
+ return "default";
+}
+
struct handle {
int fd;
int64_t size;
@@ -366,8 +373,7 @@ ondemand_open (int readonly)
nbdkit_error ("internal error: expected nbdkit_export_name () !=
NULL");
goto error;
}
- if (strcmp (h->exportname, "") == 0)
- h->exportname = "default";
+ assert (strcmp (h->exportname, "") != 0); /* see .default_export
*/
/* Verify that the export name is valid. */
if (strlen (h->exportname) > NAME_MAX ||
@@ -625,6 +631,7 @@ static struct nbdkit_plugin plugin = {
.get_ready = ondemand_get_ready,
.list_exports = ondemand_list_exports,
+ .default_export = ondemand_default_export,
.can_multi_conn = ondemand_can_multi_conn,
.can_trim = ondemand_can_trim,
diff --git a/filters/tls-fallback/tls-fallback.c
b/filters/tls-fallback/tls-fallback.c
index 822748b4..0fcc2bcf 100644
--- a/filters/tls-fallback/tls-fallback.c
+++ b/filters/tls-fallback/tls-fallback.c
@@ -76,6 +76,15 @@ tls_fallback_get_ready (nbdkit_next_get_ready *next, void
*nxdata,
/* TODO: list_exports needs is_tls parameter */
+static const char *
+tls_fallback_default_export (nbdkit_next_default_export *next, void *nxdata,
+ int readonly, int is_tls)
+{
+ if (!is_tls)
+ return "";
+ return next (nxdata, readonly);
+}
+
/* Helper for determining if this connection is insecure. This works
* because we can treat all handles on a binary basis: secure or
* insecure, which lets .open get away without allocating a more
@@ -183,7 +192,8 @@ static struct nbdkit_filter filter = {
.config = tls_fallback_config,
.config_help = tls_fallback_config_help,
.get_ready = tls_fallback_get_ready,
- /* XXX .init_exports needs is_tls parameter */
+ /* XXX .list_exports needs is_tls parameter */
+ .default_export = tls_fallback_default_export,
.open = tls_fallback_open,
.get_size = tls_fallback_get_size,
.can_write = tls_fallback_can_write,
diff --git a/tests/test-eval-exports.sh b/tests/test-eval-exports.sh
index aa694aaa..7c946378 100755
--- a/tests/test-eval-exports.sh
+++ b/tests/test-eval-exports.sh
@@ -67,7 +67,8 @@ do_nbdkit ()
# Check how the default export name is handled
# nbdinfo currently makes multiple connections, so we can't use the
# long-running server for validating default export name.
- nbdkit -U - -v eval list_exports="cat
'$PWD/eval-exports.list'" \
+ # XXX FIXME: requires .default_export in eval
+ : || nbdkit -U - -v eval list_exports="cat
'$PWD/eval-exports.list'" \
open='[ "$3" = "'"$1"'" ] || {
echo EINVAL wrong export >&2; exit 1; }' \
get_size='echo 0' --run 'nbdsh -u "$uri" -c
"exit()"'
# Check what exports are listed
diff --git a/tests/test-layers-filter.c b/tests/test-layers-filter.c
index 3f295588..f6acb448 100644
--- a/tests/test-layers-filter.c
+++ b/tests/test-layers-filter.c
@@ -115,6 +115,14 @@ test_layers_filter_list_exports (nbdkit_next_list_exports
*next, void *nxdata,
return next (nxdata, readonly, default_only, exports);
}
+static const char *
+test_layers_filter_default_export (nbdkit_next_default_export *next,
+ void *nxdata, int readonly, int is_tls)
+{
+ DEBUG_FUNCTION;
+ return next (nxdata, readonly);
+}
+
static void *
test_layers_filter_open (nbdkit_next_open *next, void *nxdata,
int readonly, const char *exportname, int is_tls)
@@ -380,6 +388,7 @@ static struct nbdkit_filter filter = {
.after_fork = test_layers_filter_after_fork,
.preconnect = test_layers_filter_preconnect,
.list_exports = test_layers_filter_list_exports,
+ .default_export = test_layers_filter_default_export,
.open = test_layers_filter_open,
.close = test_layers_filter_close,
.prepare = test_layers_filter_prepare,
diff --git a/tests/test-layers-plugin.c b/tests/test-layers-plugin.c
index 1dfd069e..254244eb 100644
--- a/tests/test-layers-plugin.c
+++ b/tests/test-layers-plugin.c
@@ -101,6 +101,13 @@ test_layers_plugin_list_exports (int readonly, int
default_only,
return nbdkit_add_export (exports, "", NULL);
}
+static const char *
+test_layers_plugin_default_export (int readonly, int is_tls)
+{
+ DEBUG_FUNCTION;
+ return "";
+}
+
static void *
test_layers_plugin_open (int readonly)
{
@@ -257,6 +264,7 @@ static struct nbdkit_plugin plugin = {
.after_fork = test_layers_plugin_after_fork,
.preconnect = test_layers_plugin_preconnect,
.list_exports = test_layers_plugin_list_exports,
+ .default_export = test_layers_plugin_default_export,
.open = test_layers_plugin_open,
.close = test_layers_plugin_close,
.get_size = test_layers_plugin_get_size,
diff --git a/tests/test-layers.c b/tests/test-layers.c
index dd826f36..db0a2da0 100644
--- a/tests/test-layers.c
+++ b/tests/test-layers.c
@@ -326,20 +326,10 @@ main (int argc, char *argv[])
"test_layers_plugin_preconnect",
NULL);
- /* list_exports methods called in outer-to-inner order, complete
- * in inner-to-outer order. But since we didn't send NBD_OPT_LIST,
- * the outer filter does not expose a list; rather, the rest of the
- * chain is used to resolve the canonical name of the default export.
+ /* XXX We should test NBD_OPT_INFO here for coverage of
+ * .list_exports. However it would be easier to do by using libnbd
+ * than open-coding a naive client.
*/
- log_verify_seen_in_order
- ("filter3: test_layers_filter_list_exports",
- "testlayersfilter2: list_exports",
- "filter2: test_layers_filter_list_exports",
- "testlayersfilter1: list_exports",
- "filter1: test_layers_filter_list_exports",
- "testlayersplugin: list_exports",
- "test_layers_plugin_list_exports",
- NULL);
/* open methods called in outer-to-inner order, but thanks to next
* pointer, complete in inner-to-outer order. */
--
2.28.0
Eric Blake
2020-Aug-27 02:16 UTC
[Libguestfs] [nbdkit PATCH v2 2/8] api: Add nbdkit_add_default_export
Rather than defaulting .list_exports to blindly returning "", it is
nicer to have it reflect the result of .default_export. Meanwhile,
language bindings will have a C callback for both .list_exports and
.default_export, even if the underlying script does not, which means
any logic in plugins.c for calling .default_export when .list_export
is missing would have to be duplicated in each language binding.
Better is to make it easy for languages, by wrapping things in a new
helper function; this also lets us take maximum advantage of caching
done in backend.c (since language bindings can't access our internals,
they would have to duplicate caching if we did not add this helper
function).
Signed-off-by: Eric Blake <eblake@redhat.com>
---
docs/nbdkit-plugin.pod | 21 +++++++++++++--------
include/nbdkit-common.h | 2 ++
server/internal.h | 4 ++++
server/backend.c | 15 ++++++++-------
server/exports.c | 24 ++++++++++++++++++++++++
server/nbdkit.syms | 1 +
server/plugins.c | 2 +-
server/test-public.c | 9 ++++++++-
plugins/sh/methods.c | 2 +-
tests/test-layers.c | 12 ++++++++++++
10 files changed, 74 insertions(+), 18 deletions(-)
diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod
index 1f208b27..50cf991d 100644
--- a/docs/nbdkit-plugin.pod
+++ b/docs/nbdkit-plugin.pod
@@ -675,10 +675,11 @@ error message and return C<-1>.
This optional callback is called if the client tries to list the
exports served by the plugin (using C<NBD_OPT_LIST>). If the plugin
-does not supply this callback then a single export called C<"">
is
-returned. The NBD protocol defines C<""> as the default
export, so
-this is suitable for plugins which ignore the export name and always
-serve the same content. See also L</EXPORT NAME> below.
+does not supply this callback then the result of C<.default_export> is
+advertised as the lone export. The NBD protocol defines C<"">
as the
+default export, so this is suitable for plugins which ignore the
+export name and always serve the same content. See also L</EXPORT
+NAME> below.
The C<readonly> flag informs the plugin that the server was started
with the I<-r> flag on the command line, which is the same value
@@ -694,12 +695,13 @@ C<"">). The plugin can ignore this flag
and return all exports if it
wants.
The C<exports> parameter is an opaque object for collecting the list
-of exports. Call C<nbdkit_add_export> to add a single export to the
-list. If the plugin has a concept of a default export (usually but
-not always called C<"">) then it should return that first in
the list.
+of exports. Call C<nbdkit_add_export> as needed to add specific
+exports to the list, or C<nbdkit_add_default_export> to add a single
+entry based on the results of C<.default_export>.
int nbdkit_add_export (struct nbdkit_export *exports,
const char *name, const char *description);
+ int nbdkit_add_default_export (struct nbdkit_export *exports);
The C<name> must be a non-NULL, UTF-8 string between 0 and 4096 bytes
in length. Export names must be unique. C<description> is an
@@ -725,7 +727,10 @@ default export C<"">, where the plugin
provides a UTF-8 string between
connection continues with the empty name; if the plugin returns a
valid string, nbdkit returns that name to clients that support it (see
the C<NBD_INFO_NAME> response to C<NBD_OPT_GO>), and behaves as if
the
-client had passed that string instead of an empty name.
+client had passed that string instead of an empty name. Similarly, if
+the plugin does not supply a C<.list_exports> callback, the result of
+this callback determines what export name to advertise to a client
+requesting an export list.
The C<readonly> flag informs the plugin that the server was started
with the I<-r> flag on the command line, which is the same value
diff --git a/include/nbdkit-common.h b/include/nbdkit-common.h
index c377e18d..5e8c0b6f 100644
--- a/include/nbdkit-common.h
+++ b/include/nbdkit-common.h
@@ -130,6 +130,8 @@ struct nbdkit_exports;
NBDKIT_EXTERN_DECL (int, nbdkit_add_export,
(struct nbdkit_exports *,
const char *name, const char *description));
+NBDKIT_EXTERN_DECL (int, nbdkit_add_default_export,
+ (struct nbdkit_exports *));
/* A static non-NULL pointer which can be used when you don't need a
* per-connection handle.
diff --git a/server/internal.h b/server/internal.h
index 8c8448e6..e2a68513 100644
--- a/server/internal.h
+++ b/server/internal.h
@@ -543,4 +543,8 @@ extern struct connection *threadlocal_get_conn (void);
struct connection *conn = threadlocal_get_conn (); \
assert (conn != NULL)
+/* exports.c */
+extern int exports_resolve_default (struct nbdkit_exports *exports,
+ struct backend *b, int readonly);
+
#endif /* NBDKIT_INTERNAL_H */
diff --git a/server/backend.c b/server/backend.c
index 00e65e3c..3e0a1d24 100644
--- a/server/backend.c
+++ b/server/backend.c
@@ -164,7 +164,7 @@ backend_list_exports (struct backend *b, int readonly, int
default_only,
{
GET_CONN;
struct handle *h = get_handle (conn, b->i);
- int r;
+ size_t count;
assert (!default_only); /* XXX Switch to is_tls... */
controlpath_debug ("%s: list_exports readonly=%d default_only=%d",
@@ -173,14 +173,15 @@ backend_list_exports (struct backend *b, int readonly, int
default_only,
assert (h->handle == NULL);
assert ((h->state & HANDLE_OPEN) == 0);
- r = b->list_exports (b, readonly, default_only, exports);
- if (r == -1)
+ if (b->list_exports (b, readonly, default_only, exports) == -1 ||
+ exports_resolve_default (exports, b, readonly) == -1) {
controlpath_debug ("%s: list_exports failed", b->name);
- else {
- size_t count = nbdkit_exports_count (exports);
- controlpath_debug ("%s: list_exports returned %zu names",
b->name, count);
+ return -1;
}
- return r;
+
+ count = nbdkit_exports_count (exports);
+ controlpath_debug ("%s: list_exports returned %zu names",
b->name, count);
+ return 0;
}
const char *
diff --git a/server/exports.c b/server/exports.c
index 8d3faec4..3259fbdb 100644
--- a/server/exports.c
+++ b/server/exports.c
@@ -52,6 +52,7 @@ DEFINE_VECTOR_TYPE(exports, struct nbdkit_export);
struct nbdkit_exports {
exports exports;
+ bool use_default;
};
struct nbdkit_exports *
@@ -65,6 +66,7 @@ nbdkit_exports_new (void)
return NULL;
}
r->exports = (exports) empty_vector;
+ r->use_default = false;
return r;
}
@@ -141,3 +143,25 @@ nbdkit_add_export (struct nbdkit_exports *exps,
return 0;
}
+
+int
+nbdkit_add_default_export (struct nbdkit_exports *exps)
+{
+ exps->use_default = true;
+ return 0;
+}
+
+int
+exports_resolve_default (struct nbdkit_exports *exps, struct backend *b,
+ int readonly)
+{
+ const char *def = NULL;
+
+ if (exps->use_default) {
+ def = backend_default_export (b, readonly);
+ exps->use_default = false;
+ }
+ if (def)
+ return nbdkit_add_export (exps, def, NULL);
+ return 0;
+}
diff --git a/server/nbdkit.syms b/server/nbdkit.syms
index a67669b7..212e36aa 100644
--- a/server/nbdkit.syms
+++ b/server/nbdkit.syms
@@ -39,6 +39,7 @@
# The functions we want plugins and filters to call.
global:
nbdkit_absolute_path;
+ nbdkit_add_default_export;
nbdkit_add_export;
nbdkit_add_extent;
nbdkit_debug;
diff --git a/server/plugins.c b/server/plugins.c
index 8172759f..da28b2f1 100644
--- a/server/plugins.c
+++ b/server/plugins.c
@@ -289,7 +289,7 @@ plugin_list_exports (struct backend *b, int readonly, int
default_only,
struct backend_plugin *p = container_of (b, struct backend_plugin, backend);
if (!p->plugin.list_exports)
- return nbdkit_add_export (exports, "", NULL);
+ return nbdkit_add_default_export (exports);
return p->plugin.list_exports (readonly, default_only, exports);
}
diff --git a/server/test-public.c b/server/test-public.c
index 0149dd9b..ee71f2fc 100644
--- a/server/test-public.c
+++ b/server/test-public.c
@@ -66,7 +66,14 @@ threadlocal_get_conn (void)
abort ();
}
-int connection_get_status (void)
+int
+connection_get_status (void)
+{
+ abort ();
+}
+
+const char *
+backend_default_export (struct backend *b, int readonly)
{
abort ();
}
diff --git a/plugins/sh/methods.c b/plugins/sh/methods.c
index 8e2e4256..2192ae25 100644
--- a/plugins/sh/methods.c
+++ b/plugins/sh/methods.c
@@ -316,7 +316,7 @@ sh_list_exports (int readonly, int default_only,
return parse_exports (script, s, slen, exports);
case MISSING:
- return nbdkit_add_export (exports, "", NULL);
+ return nbdkit_add_default_export (exports);
case ERROR:
return -1;
diff --git a/tests/test-layers.c b/tests/test-layers.c
index db0a2da0..fa7730e6 100644
--- a/tests/test-layers.c
+++ b/tests/test-layers.c
@@ -331,6 +331,18 @@ main (int argc, char *argv[])
* than open-coding a naive client.
*/
+ /* .default_export methods called in outer-to-inner order. */
+ log_verify_seen_in_order
+ ("testlayersfilter3: default_export",
+ "filter3: test_layers_filter_default_export",
+ "testlayersfilter2: default_export",
+ "filter2: test_layers_filter_default_export",
+ "testlayersfilter1: default_export",
+ "filter1: test_layers_filter_default_export",
+ "testlayersplugin: default_export",
+ "test_layers_plugin_default_export",
+ NULL);
+
/* open methods called in outer-to-inner order, but thanks to next
* pointer, complete in inner-to-outer order. */
log_verify_seen_in_order
--
2.28.0
Eric Blake
2020-Aug-27 02:16 UTC
[Libguestfs] [nbdkit PATCH v2 3/8] server: Respond to NBD_INFO_NAME request
The NBD spec says that additional information in response to
NBD_OPT_GO or NBD_OPT_INFO, such as NBD_INFO_NAME, is optional, but
also recommends that if a client hints about wanting a particular
piece of information, that it is better for the server to reply to
those known hints. This is also the the only way for a client to
learn the canonical name that is used when connecting to the default
export name.
libnbd 1.4 is adding new APIs to request the client hint, and to track
what the server replies with, making this easy enough to test.
However, at present, the ondemand plugin is the only one to advertise
an alternate name for the default export, so the test will be further
enhanced when .default_export is plumbed through the sh plugin.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
tests/Makefile.am | 4 +-
server/protocol-handshake-newstyle.c | 59 +++++++++++++-
tests/test-export-info.sh | 112 +++++++++++++++++++++++++++
3 files changed, 169 insertions(+), 6 deletions(-)
create mode 100755 tests/test-export-info.sh
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 2db56ded..29c4811b 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -491,8 +491,8 @@ TESTS += test-eflags.sh
EXTRA_DIST += test-eflags.sh
# Test export name.
-TESTS += test-export-name.sh
-EXTRA_DIST += test-export-name.sh
+TESTS += test-export-name.sh test-export-info.sh
+EXTRA_DIST += test-export-name.sh test-export-info.sh
# cdi plugin test.
TESTS += test-cdi.sh
diff --git a/server/protocol-handshake-newstyle.c
b/server/protocol-handshake-newstyle.c
index 6e5f3822..1777219f 100644
--- a/server/protocol-handshake-newstyle.c
+++ b/server/protocol-handshake-newstyle.c
@@ -156,6 +156,36 @@ send_newstyle_option_reply_info_export (uint32_t option,
uint32_t reply,
return 0;
}
+/* Can be used for NBD_INFO_NAME and NBD_INFO_DESCRIPTION. */
+static int
+send_newstyle_option_reply_info_str (uint32_t option, uint32_t reply,
+ uint16_t info, const char *str,
+ size_t len)
+{
+ GET_CONN;
+ struct nbd_fixed_new_option_reply fixed_new_option_reply;
+ struct nbd_fixed_new_option_reply_info_name_or_desc name;
+
+ if (len == -1)
+ len = strlen (str);
+
+ fixed_new_option_reply.magic = htobe64 (NBD_REP_MAGIC);
+ fixed_new_option_reply.option = htobe32 (option);
+ fixed_new_option_reply.reply = htobe32 (reply);
+ fixed_new_option_reply.replylen = htobe32 (sizeof info + len);
+ name.info = htobe16 (info);
+
+ if (conn->send (&fixed_new_option_reply,
+ sizeof fixed_new_option_reply, SEND_MORE) == -1 ||
+ conn->send (&name, sizeof name, SEND_MORE) == -1 ||
+ conn->send (str, len, 0) == -1) {
+ nbdkit_error ("write: %s: %m", name_of_nbd_opt (option));
+ return -1;
+ }
+
+ return 0;
+}
+
static int
send_newstyle_option_reply_meta_context (uint32_t option, uint32_t reply,
uint32_t context_id,
@@ -533,16 +563,37 @@ negotiate_handshake_newstyle_options (void)
exportsize) == -1)
return -1;
- /* For now we ignore all other info requests (but we must
- * ignore NBD_INFO_EXPORT if it was requested, because we
- * replied already above). Therefore this loop doesn't do
- * much at the moment.
+ /* For now we send NBD_INFO_NAME if requested, and ignore all
+ * other info requests (including NBD_INFO_EXPORT if it was
+ * requested, because we replied already above).
+ * XXX NBD_INFO_DESCRIPTION is easy once we add .export_description.
*/
for (i = 0; i < nrinfos; ++i) {
memcpy (&info, &data[4 + exportnamelen + 2 + i*2], 2);
info = be16toh (info);
switch (info) {
case NBD_INFO_EXPORT: /* ignore - reply sent above */ break;
+ case NBD_INFO_NAME:
+ {
+ const char *name = &data[4];
+ size_t namelen = exportnamelen;
+
+ if (exportnamelen == 0) {
+ name = backend_default_export (top, read_only);
+ if (!name) {
+ debug ("newstyle negotiation: %s: "
+ "NBD_INFO_NAME: no name to send", optname);
+ break;
+ }
+ namelen = -1;
+ }
+ if (send_newstyle_option_reply_info_str (option,
+ NBD_REP_INFO,
+ NBD_INFO_NAME,
+ name, namelen) == -1)
+ return -1;
+ }
+ break;
default:
debug ("newstyle negotiation: %s: "
"ignoring NBD_INFO_* request %u (%s)",
diff --git a/tests/test-export-info.sh b/tests/test-export-info.sh
new file mode 100755
index 00000000..b0dd4ce8
--- /dev/null
+++ b/tests/test-export-info.sh
@@ -0,0 +1,112 @@
+#!/usr/bin/env bash
+# nbdkit
+# Copyright (C) 2019-2020 Red Hat Inc.
+#
+# Redistribution and use in source and binary forms, with or without
+# modification, are permitted provided that the following conditions are
+# met:
+#
+# * Redistributions of source code must retain the above copyright
+# notice, this list of conditions and the following disclaimer.
+#
+# * Redistributions in binary form must reproduce the above copyright
+# notice, this list of conditions and the following disclaimer in the
+# documentation and/or other materials provided with the distribution.
+#
+# * Neither the name of Red Hat nor the names of its contributors may be
+# used to endorse or promote products derived from this software without
+# specific prior written permission.
+#
+# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS
IS'' AND
+# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
+# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
+# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
+# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+# SUCH DAMAGE.
+
+source ./functions.sh
+set -e
+set -x
+
+requires_plugin sh
+requires nbdsh -c 'print (h.set_full_info)'
+
+export sock=`mktemp -u`
+files="$sock export-info.pid"
+rm -f $files
+cleanup_fn rm -f $files
+
+# Create an nbdkit sh plugin for checking NBD_INFO replies to NBD_OPT_GO.
+# XXX Update when .default_export and .export_description are implemented in sh
+start_nbdkit -P export-info.pid -U $sock \
+ sh - <<'EOF'
+case "$1" in
+ default_export) echo hello ;;
+ open) echo "$3" ;;
+ export_description) echo "$2 world" ;;
+ get_size) echo 0 ;;
+ *) exit 2 ;;
+esac
+EOF
+
+# Without client request, nothing is advertised
+nbdsh -c '
+import os
+
+h.set_opt_mode (True)
+h.connect_unix (os.environ["sock"])
+
+h.set_export_name ("a")
+h.opt_info ()
+try:
+ h.get_canonical_export_name ()
+ assert False
+except nbd.Error as ex:
+ pass
+
+h.set_export_name ("")
+h.opt_info ()
+try:
+ h.get_canonical_export_name ()
+ assert False
+except nbd.Error as ex:
+ pass
+
+h.opt_go ()
+try:
+ h.get_canonical_export_name ()
+ assert False
+except nbd.Error as ex:
+ pass
+
+h.shutdown ()
+'
+
+# With client request, reflect the export name back
+nbdsh -c '
+import os
+
+h.set_opt_mode (True)
+h.set_full_info (True)
+h.connect_unix (os.environ["sock"])
+
+h.set_export_name ("a")
+h.opt_info ()
+assert h.get_canonical_export_name () == "a"
+
+h.set_export_name ("")
+h.opt_info ()
+# XXX Adjust once default export works
+assert h.get_canonical_export_name () == ""
+
+h.opt_go ()
+assert h.get_canonical_export_name () == ""
+
+h.shutdown ()
+'
--
2.28.0
Eric Blake
2020-Aug-27 02:16 UTC
[Libguestfs] [nbdkit PATCH v2 4/8] api: Add nbdkit_str[n]dup_intern helper
Implementing .default_export with its 'const char *' return is tricky
in the sh plugin: we must return dynamic memory, but must avoid a
use-after-free. And we don't want to change the return type of
.default_export to 'char *', because that would make our choice of
malloc()/free() part of the API, preventing either nbdkit or a plugin
from experimenting with an alternate allocator implementation.
Elsewhere, we have done things like nbdkit_extents_new(), so that even
though the client is directing allocation, the actual call to malloc()
is done by nbdkit proper, so that nbdkit later calling free() does not
tie the plugin's hands, and nbdkit could change its underlying
allocation without breaking ABI. (Note that nbdkit_realpath() and
nbdkit_absolute_path() require the caller to use free(), but as they
are used during .config, it's less of a burden for plugins to take
care of that in .unload.)
We could document that the burden is on the plugin to avoid the memory
leak, by making sh track all strings it returns, then finally clean
them up during .unload. But this is still a memory leak in the case
of .default_export, which can be called multiple times when there are
multiple clients, and presumably with different values per client
call; better would be freeing the memory at .close when each client
goes away. Except that .default_export is called before .open, and
therefore before we have access to the handle struct that will
eventually make it to .close.
So, the solution is to let nbdkit track an alternate string on our
behalf, where nbdkit then cleans it up as the client goes away.
There are several places in the existing code base that can also take
advantage of this copying functionality, including in filters that
want to pass altered strings to .config while still obeying lifetime
rules.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
docs/nbdkit-filter.pod | 9 +++++---
docs/nbdkit-plugin.pod | 33 +++++++++++++++++++++++++---
include/nbdkit-common.h | 4 ++++
server/internal.h | 7 +++++-
server/connections.c | 2 +-
server/main.c | 26 ++++++----------------
server/nbdkit.syms | 2 ++
server/plugins.c | 11 +++-------
server/public.c | 48 +++++++++++++++++++++++++++++++++++++++++
filters/log/log.c | 10 +++------
10 files changed, 110 insertions(+), 42 deletions(-)
diff --git a/docs/nbdkit-filter.pod b/docs/nbdkit-filter.pod
index dd667c0c..7d72d138 100644
--- a/docs/nbdkit-filter.pod
+++ b/docs/nbdkit-filter.pod
@@ -183,7 +183,8 @@ You can modify parameters when you call the C<next>
function. However
be careful when modifying strings because for some methods
(eg. C<.config>) the plugin may save the string pointer that you pass
along. So you may have to ensure that the string is not freed for the
-lifetime of the server.
+lifetime of the server; you may find C<nbdkit_strdup_intern> helpful
+for avoiding a memory leak while still obeying lifecycle constraints.
Note that if your filter registers a callback but in that callback it
doesn't call the C<next> function then the corresponding method in
the
@@ -450,8 +451,10 @@ requires write access to the underlying device in case a
journal needs
to be replayed for consistency as part of the mounting process.
The C<exportname> string is only guaranteed to be available during the
-call. If the filter needs to use it (other than immediately passing
-it down to the next layer) it must take a copy. The C<exportname> and
+call (different than the lifetime for the return of C<nbdkit_export_name>
+used by plugins). If the filter needs to use it (other than immediately
+passing it down to the next layer) it must take a copy, although
+C<nbdkit_strdup_intern> is useful for this task. The C<exportname>
and
C<is_tls> parameters are provided so that filters do not need to use
the plugin-only interfaces of C<nbdkit_export_name> and
C<nbdkit_is_tls>.
diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod
index 50cf991d..056b1450 100644
--- a/docs/nbdkit-plugin.pod
+++ b/docs/nbdkit-plugin.pod
@@ -748,7 +748,10 @@ leak unintended information.
If the plugin returns C<NULL> or an invalid string (such as longer
than 4096 bytes), the client is not permitted to connect to the
default export. However, this is not an error in the protocol, so it
-is not necessary to call C<nbdkit_error>.
+is not necessary to call C<nbdkit_error>. If the callback will
+not be returning a compile-time constant string, you may find
+C<nbdkit_strdup_intern> helpful for returning a value that avoids a
+memory leak.
=head2 C<.open>
@@ -1504,8 +1507,9 @@ content.
Return the optional NBD export name if one was negotiated with the
current client (this uses thread-local magic so no parameter is
-required). The returned string is only valid while the client is
-connected, so if you need to store it in the plugin you must copy it.
+required). The returned string is valid at least through the
+C<.close> of the current connection, but if you need to store it
+in the plugin for use by more than one client you must copy it.
The export name is a free-form text string, it is not necessarily a
path or filename and it does not need to begin with a C<'/'>
@@ -1516,6 +1520,29 @@ client data, be cautious when parsing it.>
On error, C<nbdkit_error> is called and the call returns C<NULL>.
+=head1 STRING LIFETIME
+
+Some callbacks are specified to return C<const char *>, even when a
+plugin may not have a suitable compile-time constant to return.
+Returning dynamically-allocated memory for such a callback would
+induce a memory leak or otherwise complicate the plugin to perform
+additional bookkeeping. For these cases, nbdkit provides a
+convenience function for creating a copy of a string for better
+lifetime management.
+
+ const char *nbdkit_strdup_intern (const char *str);
+ const char *nbdkit_strndup_intern (const char *str, size_t n);
+
+Returns a copy of C<str>, possibly limited to a maximum of C<n>
bytes,
+so that the caller may reclaim str and use the copy in its place. If
+the copy is created outside the scope of a connection (such as during
+C<.load> or C<.config>), the lifetime of the copy will last at
least
+through C<.unload>; if it was created within a connection (such as
+during C<.preconnect>, C<.default_export>, or C<.open>), the
lifetime
+will last at least through C<.close>.
+
+On error, C<nbdkit_error> is called and the call returns C<NULL>.
+
=head1 AUTHENTICATION
A server may use C<nbdkit_is_tls> to limit which export names work
diff --git a/include/nbdkit-common.h b/include/nbdkit-common.h
index 5e8c0b6f..e9d31421 100644
--- a/include/nbdkit-common.h
+++ b/include/nbdkit-common.h
@@ -120,6 +120,10 @@ NBDKIT_EXTERN_DECL (int, nbdkit_nanosleep, (unsigned sec,
unsigned nsec));
NBDKIT_EXTERN_DECL (int, nbdkit_peer_name,
(struct sockaddr *addr, socklen_t *addrlen));
NBDKIT_EXTERN_DECL (void, nbdkit_shutdown, (void));
+NBDKIT_EXTERN_DECL (const char *, nbdkit_strdup_intern,
+ (const char *str));
+NBDKIT_EXTERN_DECL (const char *, nbdkit_strndup_intern,
+ (const char *str, size_t n));
struct nbdkit_extents;
NBDKIT_EXTERN_DECL (int, nbdkit_add_extent,
diff --git a/server/internal.h b/server/internal.h
index e2a68513..f11f897e 100644
--- a/server/internal.h
+++ b/server/internal.h
@@ -237,6 +237,7 @@ reset_handle (struct handle *h)
h->can_cache = -1;
}
+DEFINE_VECTOR_TYPE(string_vector, char *);
struct connection {
pthread_mutex_t request_lock;
pthread_mutex_t read_lock;
@@ -258,8 +259,9 @@ struct connection {
bool structured_replies;
bool meta_context_base_allocation;
+ string_vector interns;
char *exportname_from_set_meta_context;
- char *exportname;
+ const char *exportname;
int sockin, sockout;
connection_recv_function recv;
@@ -298,6 +300,9 @@ extern int protocol_recv_request_send_reply (void);
*/
#define base_allocation_id 1
+/* public.c */
+extern void free_interns (void);
+
/* crypto.c */
#define root_tls_certificates_dir sysconfdir "/pki/" PACKAGE_NAME
extern void crypto_init (bool tls_set_on_cli);
diff --git a/server/connections.c b/server/connections.c
index 67a68469..d9f685c9 100644
--- a/server/connections.c
+++ b/server/connections.c
@@ -360,7 +360,7 @@ free_connection (struct connection *conn)
pthread_mutex_destroy (&conn->status_lock);
free (conn->exportname_from_set_meta_context);
- free (conn->exportname);
+ free_interns ();
/* This is needed in order to free a field in struct handle. */
for_each_backend (b)
diff --git a/server/main.c b/server/main.c
index 17c4c324..ae552447 100644
--- a/server/main.c
+++ b/server/main.c
@@ -631,15 +631,9 @@ main (int argc, char *argv[])
*
* Keys must live for the life of nbdkit. Since we want to avoid
* modifying argv (so that /proc/PID/cmdline remains sane) but we
- * need to create a key from argv[i] = "key=value" we must save the
- * keys in an array which is freed at the end of main().
+ * need to create a key from argv[i] = "key=value" we must intern
+ * the keys, which are then freed at the end of main().
*/
- char **keys = calloc (argc, sizeof (char *));
- if (keys == NULL) {
- perror ("calloc");
- exit (EXIT_FAILURE);
- }
-
magic_config_key = top->magic_config_key (top);
for (i = 0; optind < argc; ++i, ++optind) {
size_t n;
@@ -647,12 +641,10 @@ main (int argc, char *argv[])
p = strchr (argv[optind], '=');
n = p - argv[optind];
if (p && is_config_key (argv[optind], n)) { /* Is it key=value? */
- keys[optind] = strndup (argv[optind], n);
- if (keys[optind] == NULL) {
- perror ("strndup");
+ const char *key = nbdkit_strndup_intern (argv[optind], n);
+ if (key == NULL)
exit (EXIT_FAILURE);
- }
- top->config (top, keys[optind], p+1);
+ top->config (top, key, p+1);
}
else if (magic_config_key == NULL) {
if (i == 0) /* magic script parameter */
@@ -677,9 +669,7 @@ main (int argc, char *argv[])
if (dump_plugin) {
top->dump_fields (top);
top->free (top);
- for (i = 1; i < argc; ++i)
- free (keys[i]);
- free (keys);
+ free_interns ();
exit (EXIT_SUCCESS);
}
@@ -717,9 +707,7 @@ main (int argc, char *argv[])
crypto_free ();
close_quit_pipe ();
- for (i = 1; i < argc; ++i)
- free (keys[i]);
- free (keys);
+ free_interns ();
/* Note: Don't exit here, otherwise this won't work when compiled
* for libFuzzer.
diff --git a/server/nbdkit.syms b/server/nbdkit.syms
index 212e36aa..d17878b7 100644
--- a/server/nbdkit.syms
+++ b/server/nbdkit.syms
@@ -74,6 +74,8 @@
nbdkit_set_error;
nbdkit_shutdown;
nbdkit_stdio_safe;
+ nbdkit_strdup_intern;
+ nbdkit_strndup_intern;
nbdkit_vdebug;
nbdkit_verror;
diff --git a/server/plugins.c b/server/plugins.c
index da28b2f1..7425857e 100644
--- a/server/plugins.c
+++ b/server/plugins.c
@@ -328,17 +328,13 @@ plugin_open (struct backend *b, int readonly, const char
*exportname,
* will still need to save the export name in the handle because of
* the lifetime issue.
*/
- conn->exportname = strdup (exportname);
- if (conn->exportname == NULL) {
- nbdkit_error ("strdup: %m");
+ conn->exportname = nbdkit_strdup_intern (exportname);
+ if (conn->exportname == NULL)
return NULL;
- }
r = p->plugin.open (readonly);
- if (r == NULL) {
- free (conn->exportname);
+ if (r == NULL)
conn->exportname = NULL;
- }
return r;
}
@@ -367,7 +363,6 @@ plugin_close (struct backend *b, void *handle)
if (handle && p->plugin.close)
p->plugin.close (handle);
- free (conn->exportname);
conn->exportname = NULL;
}
diff --git a/server/public.c b/server/public.c
index d10d466e..512e9caa 100644
--- a/server/public.c
+++ b/server/public.c
@@ -726,3 +726,51 @@ nbdkit_peer_name (struct sockaddr *addr, socklen_t
*addrlen)
return 0;
}
+
+/* Functions for manipulating intern'd strings. */
+
+static string_vector global_interns;
+
+void
+free_interns (void)
+{
+ struct connection *conn = threadlocal_get_conn ();
+ string_vector *list = conn ? &conn->interns : &global_interns;
+
+ string_vector_iter (list, (void *) free);
+ string_vector_reset (list);
+}
+
+const char *
+nbdkit_strndup_intern (const char *str, size_t n)
+{
+ struct connection *conn = threadlocal_get_conn ();
+ string_vector *list = conn ? &conn->interns : &global_interns;
+ char *copy;
+
+ if (str == NULL) {
+ nbdkit_error ("nbdkit_strndup_intern: no string given");
+ errno = EINVAL;
+ return NULL;
+ }
+
+ copy = strndup (str, n);
+ if (copy == NULL) {
+ nbdkit_error ("strndup: %m");
+ return NULL;
+ }
+
+ if (string_vector_append (list, copy) == -1) {
+ nbdkit_error ("malloc: %m");
+ free (copy);
+ return NULL;
+ }
+
+ return copy;
+}
+
+const char *
+nbdkit_strdup_intern (const char *str)
+{
+ return nbdkit_strndup_intern (str, SIZE_MAX);
+}
diff --git a/filters/log/log.c b/filters/log/log.c
index 71c21211..4525f362 100644
--- a/filters/log/log.c
+++ b/filters/log/log.c
@@ -134,7 +134,7 @@ log_get_ready (nbdkit_next_get_ready *next, void *nxdata,
int thread_model)
struct handle {
uint64_t connection;
uint64_t id;
- char *exportname;
+ const char *exportname;
int tls;
};
@@ -305,9 +305,8 @@ log_open (nbdkit_next_open *next, void *nxdata,
* it in log_prepare. We must take a copy because this string has a
* short lifetime.
*/
- h->exportname = strdup (exportname);
+ h->exportname = nbdkit_strdup_intern (exportname);
if (h->exportname == NULL) {
- nbdkit_error ("strdup: %m");
free (h);
return NULL;
}
@@ -322,10 +321,7 @@ log_open (nbdkit_next_open *next, void *nxdata,
static void
log_close (void *handle)
{
- struct handle *h = handle;
-
- free (h->exportname);
- free (h);
+ free (handle);
}
static int
--
2.28.0
Eric Blake
2020-Aug-27 02:16 UTC
[Libguestfs] [nbdkit PATCH v2 5/8] sh, eval: Implement .default_export
Use the recently added nbdkit_strndup_intern to make this possible.
Testsuite coverage is added in both tls-fallback as well as a cleanup
to test-eval-exports.sh.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
plugins/eval/nbdkit-eval-plugin.pod | 2 ++
plugins/sh/nbdkit-sh-plugin.pod | 20 +++++++++++--
plugins/sh/methods.h | 1 +
plugins/eval/eval.c | 2 ++
plugins/sh/methods.c | 45 ++++++++++++++++++++++++++++-
plugins/sh/sh.c | 1 +
plugins/sh/example.sh | 2 +-
tests/test-eval-exports.sh | 35 ++++++++++++++--------
tests/test-export-info.sh | 7 ++---
tests/test-tls-fallback.sh | 11 ++++---
10 files changed, 102 insertions(+), 24 deletions(-)
diff --git a/plugins/eval/nbdkit-eval-plugin.pod
b/plugins/eval/nbdkit-eval-plugin.pod
index baf79adf..eb3c7f5a 100644
--- a/plugins/eval/nbdkit-eval-plugin.pod
+++ b/plugins/eval/nbdkit-eval-plugin.pod
@@ -112,6 +112,8 @@ features):
=item B<list_exports=>SCRIPT
+=item B<default_export=>SCRIPT
+
=item B<open=>SCRIPT
=item B<pread=>SCRIPT
diff --git a/plugins/sh/nbdkit-sh-plugin.pod b/plugins/sh/nbdkit-sh-plugin.pod
index 07d90b57..b827b658 100644
--- a/plugins/sh/nbdkit-sh-plugin.pod
+++ b/plugins/sh/nbdkit-sh-plugin.pod
@@ -315,8 +315,24 @@ export name or description, although this could be possible
under a
new mode supporting escape sequences.
This method is I<not> required; if it is absent, the list of exports
-advertised by nbdkit will be the single export with the empty string
-as a name and no description.
+advertised by nbdkit will be the single name result of C<default_export>
+and no description.
+
+=item C<default_export>
+
+ /path/to/script default_export <readonly> <tls>
+
+The C<readonly> and C<tls> parameters will be C<true> or
C<false>.
+
+On success this should print a name on stdout to use in place of the
+default export C<"">, then exit with code C<0>. For
convenience, the
+output can be any of the list forms recognized by C<list_exports>, in
+which case the first listed export name is used, and where an empty
+list uses C<"">. Given the current set of recognized export
lists, it
+is not possible for the resulting name to include a newline.
+
+This method is I<not> required; if it is absent, the default export
+name will be the empty string, C<"">.
=item C<open>
diff --git a/plugins/sh/methods.h b/plugins/sh/methods.h
index 69017fa4..3fd4ef42 100644
--- a/plugins/sh/methods.h
+++ b/plugins/sh/methods.h
@@ -46,6 +46,7 @@ extern int sh_after_fork (void);
extern int sh_preconnect (int readonly);
extern int sh_list_exports (int readonly, int default_only,
struct nbdkit_exports *exports);
+extern const char *sh_default_export (int readonly, int is_tls);
extern void *sh_open (int readonly);
extern void sh_close (void *handle);
extern int64_t sh_get_size (void *handle);
diff --git a/plugins/eval/eval.c b/plugins/eval/eval.c
index 2bd5e79f..a123734e 100644
--- a/plugins/eval/eval.c
+++ b/plugins/eval/eval.c
@@ -68,6 +68,7 @@ static const char *known_methods[] = {
"close",
"config",
"config_complete",
+ "default_export",
"dump_plugin",
"extents",
"flush",
@@ -395,6 +396,7 @@ static struct nbdkit_plugin plugin = {
.preconnect = sh_preconnect,
.list_exports = sh_list_exports,
+ .default_export = sh_default_export,
.open = sh_open,
.close = sh_close,
diff --git a/plugins/sh/methods.c b/plugins/sh/methods.c
index 2192ae25..55d6d535 100644
--- a/plugins/sh/methods.c
+++ b/plugins/sh/methods.c
@@ -241,7 +241,9 @@ parse_exports (const char *script,
{
const char *n, *d, *p, *q;
- /* The first line determines how to parse the rest of s */
+ /* The first line determines how to parse the rest of s. Keep
+ * sh_default_export in sync with this.
+ */
if ((p = skip_prefix (s, "INTERLEAVED\n")) != NULL) {
n = p;
while ((d = strchr (n, '\n')) != NULL) {
@@ -331,6 +333,47 @@ sh_list_exports (int readonly, int default_only,
}
}
+const char *
+sh_default_export (int readonly, int is_tls)
+{
+ const char *method = "default_export";
+ const char *script = get_script (method);
+ const char *args[] = { script, method, readonly ? "true" :
"false",
+ is_tls ? "true" : "false", NULL };
+ CLEANUP_FREE char *s = NULL;
+ size_t slen;
+ const char *p, *n;
+
+ switch (call_read (&s, &slen, args)) {
+ case OK:
+ /* The first line determines how to parse the rest of s. For now,
+ * all export modes treat the next line as the first export.
+ */
+ if ((p = skip_prefix (s, "INTERLEAVED\n")) != NULL ||
+ (p = skip_prefix (s, "NAMES+DESCRIPTIONS\n")) != NULL ||
+ (p = skip_prefix (s, "NAMES\n")) != NULL)
+ ;
+ else
+ p = s;
+ n = strchr (p, '\n') ?: s + slen;
+ return nbdkit_strndup_intern (p, n - p);
+
+ case MISSING:
+ return "";
+
+ case ERROR:
+ return NULL;
+
+ case RET_FALSE:
+ nbdkit_error ("%s: %s method returned unexpected code (3/false)",
+ script, method);
+ errno = EIO;
+ return NULL;
+
+ default: abort ();
+ }
+}
+
void *
sh_open (int readonly)
{
diff --git a/plugins/sh/sh.c b/plugins/sh/sh.c
index 374888a4..4fcc2a5a 100644
--- a/plugins/sh/sh.c
+++ b/plugins/sh/sh.c
@@ -301,6 +301,7 @@ static struct nbdkit_plugin plugin = {
.preconnect = sh_preconnect,
.list_exports = sh_list_exports,
+ .default_export = sh_default_export,
.open = sh_open,
.close = sh_close,
diff --git a/plugins/sh/example.sh b/plugins/sh/example.sh
index 4f547db0..e0e1b773 100755
--- a/plugins/sh/example.sh
+++ b/plugins/sh/example.sh
@@ -85,7 +85,7 @@ case "$1" in
echo parallel
;;
- list_exports)
+ list_exports | default_export)
# The following lists the names of all files in the current
# directory that do not contain whitespace, backslash, or single
# quotes. No description accompanies the export names.
diff --git a/tests/test-eval-exports.sh b/tests/test-eval-exports.sh
index 7c946378..14b4c2ce 100755
--- a/tests/test-eval-exports.sh
+++ b/tests/test-eval-exports.sh
@@ -37,8 +37,8 @@ source ./functions.sh
set -e
set -x
-# requires nbdinfo --version # nbdinfo 1.3.9 was broken, so check this instead:
-requires nbdkit -U - memory 1 --run 'nbdinfo --size --json
"$uri"'
+requires nbdinfo --version
+requires nbdsh -c 'print (h.set_full_info)'
requires jq --version
sock=$(mktemp -u)
@@ -57,20 +57,19 @@ diff -u <(jq -c \
'[.exports[] | [."export-name", .description,
."export-size"]]' \
eval-exports.out) <(printf %s\\n
'[["",null,0]]')
-# Start a long-running server with .list_exports set to varying contents
+# Start a long-running server with .list_exports and .default_export
+# set to varying contents
start_nbdkit -P eval-exports.pid -U $sock eval get_size='echo
"$2"|wc -c' \
- open='echo "$3"' list_exports="cat
'$PWD/eval-exports.list'"
+ open='echo "$3"' list_exports="cat
'$PWD/eval-exports.list'" \
+ default_export="cat '$PWD/eval-exports.list'"
# do_nbdkit EXPNAME EXPOUT
do_nbdkit ()
{
# Check how the default export name is handled
- # nbdinfo currently makes multiple connections, so we can't use the
- # long-running server for validating default export name.
- # XXX FIXME: requires .default_export in eval
- : || nbdkit -U - -v eval list_exports="cat
'$PWD/eval-exports.list'" \
- open='[ "$3" = "'"$1"'" ] || {
echo EINVAL wrong export >&2; exit 1; }' \
- get_size='echo 0' --run 'nbdsh -u "$uri" -c
"exit()"'
+ nbdinfo --no-content nbd+unix://\?socket=$sock >eval-exports.out
+ diff -u <(sed -n 's/export="\(.*\)":/\1/p'
eval-exports.out) \
+ <(printf %s\\n "$1")
# Check what exports are listed
nbdinfo --list --json nbd+unix://\?socket=$sock >eval-exports.out
cat eval-exports.out
@@ -79,9 +78,21 @@ do_nbdkit ()
eval-exports.out) <(printf %s\\n "$2")
}
-# With no file, .list_exports fails, but connecting works
+# With no file, .list_exports and .default_export both fail, preventing
+# connection to the default export, but not other exports
nbdinfo --list --json nbd+unix://\?socket=$sock && fail=1
-nbdsh -u nbd+unix://\?socket=$sock -c 'quit()'
+nbdsh -u nbd+unix://\?socket=$sock -c 'quit()' && fail=1
+nbdsh -u nbd+unix:///name\?socket=$sock -c 'quit()'
+
+# Setting .default_export but not .list_exports advertises the canonical name
+nbdkit -U - eval default_export='echo hello' get_size='echo 0'
\
+ --run 'nbdinfo --list "$uri"' >eval-exports.out
+diff -u <(grep '^export=' eval-exports.out) <(echo
'export="hello":')
+
+# Failing .default_export without .list_exports results in an empty list
+nbdkit -U - eval default_export='echo ENOENT >&2; exit 1'
get_size='echo 0' \
+ --run 'nbdinfo --list "$uri"' >eval-exports.out
+diff -u <(grep '^export=' eval-exports.out) /dev/null
# Various spellings of empty lists, producing 0 exports
for fmt in '' 'NAMES\n' 'INTERLEAVED\n'
'NAMES+DESCRIPTIONS\n'; do
diff --git a/tests/test-export-info.sh b/tests/test-export-info.sh
index b0dd4ce8..984d86c8 100755
--- a/tests/test-export-info.sh
+++ b/tests/test-export-info.sh
@@ -43,7 +43,7 @@ rm -f $files
cleanup_fn rm -f $files
# Create an nbdkit sh plugin for checking NBD_INFO replies to NBD_OPT_GO.
-# XXX Update when .default_export and .export_description are implemented in sh
+# XXX Update when .export_description is implemented in sh
start_nbdkit -P export-info.pid -U $sock \
sh - <<'EOF'
case "$1" in
@@ -102,11 +102,10 @@ assert h.get_canonical_export_name () == "a"
h.set_export_name ("")
h.opt_info ()
-# XXX Adjust once default export works
-assert h.get_canonical_export_name () == ""
+assert h.get_canonical_export_name () == "hello"
h.opt_go ()
-assert h.get_canonical_export_name () == ""
+assert h.get_canonical_export_name () == "hello"
h.shutdown ()
'
diff --git a/tests/test-tls-fallback.sh b/tests/test-tls-fallback.sh
index cf54a606..94449f31 100755
--- a/tests/test-tls-fallback.sh
+++ b/tests/test-tls-fallback.sh
@@ -60,12 +60,15 @@ cleanup_fn rm -f $files
# Run dual-mode server
start_nbdkit -P $pid -U $sock --tls=on --tls-psk=keys.psk \
--filter=tls-fallback sh - tlsreadme=$'dummy\n' <<\EOF
+check () {
+ if test "$1" != true; then
+ echo 'EINVAL unexpected tls mode' 2>&1; exit 1
+ fi
+}
case $1 in
list_exports) echo NAMES; echo hello; echo world ;;
- open) if test "$4" != true; then
- echo 'EINVAL unexpected tls mode' 2>&1; exit 1
- fi
- echo $3 ;;
+ default_export) check "$3"; echo hello ;;
+ open) check "$4"; echo $3 ;;
get_size) echo "$2" | wc -c ;;
pread) echo "$2" | dd skip=$4 count=$3 iflag=skip_bytes,count_bytes
;;
can_write | can_trim) exit 0 ;;
--
2.28.0
Eric Blake
2020-Aug-27 02:16 UTC
[Libguestfs] [nbdkit PATCH v2 6/8] api: Alter .list_exports
I'm about to add an 'exportname' filter, and in the process, I
noticed
a few shortcomings in our API. Time to fix those before the 1.22
release locks our API in stone. The existing 'tls-fallback' filter
already mentioned that we have situations where we want to expose a
different list before and after TLS negotiation on a tls=1 setup, but
.list_exports is executed before .open has set nbdkit_is_tls(). And
since a previous patch split out the .default_export functionality, it
is easiest to just repurpose the existing parameter to a new name.
However, it does adjust the signature for filters calling into the
next layer, as filters don't change whether TLS has been attempted.
This also fixes the cc plugin to expose .list_exports.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
docs/nbdkit-filter.pod | 10 ++++++--
docs/nbdkit-plugin.pod | 17 ++++++-------
filters/log/nbdkit-log-filter.pod | 2 +-
plugins/sh/nbdkit-sh-plugin.pod | 8 ++-----
include/nbdkit-filter.h | 3 +--
include/nbdkit-plugin.h | 2 +-
server/internal.h | 5 ++--
server/backend.c | 9 ++++---
server/filters.c | 6 ++---
server/plugins.c | 4 ++--
server/protocol-handshake-newstyle.c | 2 +-
plugins/cc/cc.c | 9 +++++++
plugins/sh/methods.c | 5 ++--
filters/log/log.c | 8 +++----
filters/tls-fallback/tls-fallback.c | 12 ++++++++--
tests/test-tls-fallback.sh | 36 ++++++++++++++++++++++++----
tests/test-layers-filter.c | 4 ++--
TODO | 6 -----
18 files changed, 93 insertions(+), 55 deletions(-)
diff --git a/docs/nbdkit-filter.pod b/docs/nbdkit-filter.pod
index 7d72d138..79c39f0d 100644
--- a/docs/nbdkit-filter.pod
+++ b/docs/nbdkit-filter.pod
@@ -340,12 +340,18 @@ an error message and return C<-1>.
=head2 C<.list_exports>
int (*list_exports) (nbdkit_next_list_exports *next, void *nxdata,
- int readonly, int default_only,
+ int readonly, int is_tls,
struct nbdkit_exports *exports);
This intercepts the plugin C<.list_exports> method and can be used to
filter which exports are advertised.
+The C<readonly> parameter matches what is passed to <.preconnect>
and
+C<.open>, and may be changed by the filter when calling into the
+plugin. The C<is_tls> parameter informs the filter whether TLS
+negotiation has been completed by the client, but is not passed on to
+C<next> because it cannot be altered.
+
It is possible for filters to transform the exports list received back
from the layer below. Without error checking it would look like this:
@@ -357,7 +363,7 @@ from the layer below. Without error checking it would look
like this:
char *name, *desc;
exports2 = nbdkit_exports_new ();
- next_list_exports (nxdata, readonly, default_only, exports);
+ next_list_exports (nxdata, readonly, exports);
for (i = 0; i < nbdkit_exports_count (exports2); ++i) {
e = nbdkit_get_export (exports2, i);
name = adjust (e.name);
diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod
index 056b1450..9f5d6356 100644
--- a/docs/nbdkit-plugin.pod
+++ b/docs/nbdkit-plugin.pod
@@ -670,7 +670,7 @@ error message and return C<-1>.
=head2 C<.list_exports>
- int list_exports (int readonly, int default_only,
+ int list_exports (int readonly, int is_tls,
struct nbdkit_exports *exports);
This optional callback is called if the client tries to list the
@@ -688,11 +688,11 @@ not yet have a way to let the client advertise an intent
to be
read-only even when the server allows writes, so this parameter may
not be as useful as it appears.
-If the C<default_only> flag is set then the client is querying for the
-name of the default export, and the plugin may optimize by adding only
-a single export to the returned list (the default export name, usually
-C<"">). The plugin can ignore this flag and return all exports
if it
-wants.
+The C<is_tls> flag informs the plugin whether this listing was
+requested after the client has completed TLS negotiation. When
+running the server in a mode that permits but does not require TLS, be
+careful that any exports listed when C<is_tls> is C<false> do not
leak
+unintended information.
The C<exports> parameter is an opaque object for collecting the list
of exports. Call C<nbdkit_add_export> as needed to add specific
@@ -783,8 +783,9 @@ server requires TLS authentication, then that has occurred
as well.
But if the server is set up to have optional TLS authentication, you
may check C<nbdkit_is_tls> to learn whether the client has completed
TLS authentication. When running the server in a mode that permits
-but not requires TLS, be careful that you do not allow unauthenticated
-clients to cause a denial of service against authentication.
+but does not require TLS, be careful that you do not allow
+unauthenticated clients to cause a denial of service against
+authentication.
If there is an error, C<.open> should call C<nbdkit_error> with an
error message and return C<NULL>.
diff --git a/filters/log/nbdkit-log-filter.pod
b/filters/log/nbdkit-log-filter.pod
index 721fc118..40bce76d 100644
--- a/filters/log/nbdkit-log-filter.pod
+++ b/filters/log/nbdkit-log-filter.pod
@@ -72,7 +72,7 @@ on the connection).
An example logging session of a client that requests an export list
before performing a single successful read is:
- 2020-08-06 02:07:23.080415 ListExports id=1 readonly=0 default_only=0 ...
+ 2020-08-06 02:07:23.080415 ListExports id=1 readonly=0 tls=0 ...
2020-08-06 02:07:23.080502 ...ListExports id=1 exports=[""] return=0
2020-08-06 02:07:23.080712 connection=1 Connect export='' tls=0
size=0x400 write=1 flush=1 rotational=0 trim=1 zero=2 fua=2 extents=1 cache=2
fast_zero=1
2020-08-06 02:07:23.080907 connection=1 Read id=1 offset=0x0 count=0x200 ...
diff --git a/plugins/sh/nbdkit-sh-plugin.pod b/plugins/sh/nbdkit-sh-plugin.pod
index b827b658..c6e9c432 100644
--- a/plugins/sh/nbdkit-sh-plugin.pod
+++ b/plugins/sh/nbdkit-sh-plugin.pod
@@ -268,13 +268,9 @@ with status C<1>; unrecognized output is ignored.
=item C<list_exports>
- /path/to/script list_exports <readonly> <default_only>
+ /path/to/script list_exports <readonly> <tls>
-The C<readonly> parameter will be C<true> or C<false>. The
-C<default_only> parameter will be C<true> if the caller is only
-interested in the canonical name of the default export, or C<false> to
-get a full list of export names; the script may safely ignore this
-parameter and always provide a full list if desired.
+The C<readonly> and C<tls> parameters will be C<true> or
C<false>.
The first line of output informs nbdkit how to parse the rest of the
output, the remaining lines then supply the inputs of the C
diff --git a/include/nbdkit-filter.h b/include/nbdkit-filter.h
index 2c5b36be..63458d59 100644
--- a/include/nbdkit-filter.h
+++ b/include/nbdkit-filter.h
@@ -66,7 +66,6 @@ typedef int nbdkit_next_get_ready (nbdkit_backend *nxdata);
typedef int nbdkit_next_after_fork (nbdkit_backend *nxdata);
typedef int nbdkit_next_preconnect (nbdkit_backend *nxdata, int readonly);
typedef int nbdkit_next_list_exports (nbdkit_backend *nxdata, int readonly,
- int ignored,
struct nbdkit_exports *exports);
typedef const char *nbdkit_next_default_export (nbdkit_backend *nxdata,
int readonly);
@@ -179,7 +178,7 @@ struct nbdkit_filter {
int (*preconnect) (nbdkit_next_preconnect *next, nbdkit_backend *nxdata,
int readonly);
int (*list_exports) (nbdkit_next_list_exports *next, nbdkit_backend *nxdata,
- int readonly, int default_only,
+ int readonly, int is_tls,
struct nbdkit_exports *exports);
const char * (*default_export) (nbdkit_next_default_export *next,
nbdkit_backend *nxdata,
diff --git a/include/nbdkit-plugin.h b/include/nbdkit-plugin.h
index 28e83757..539e3f6e 100644
--- a/include/nbdkit-plugin.h
+++ b/include/nbdkit-plugin.h
@@ -140,7 +140,7 @@ struct nbdkit_plugin {
int (*get_ready) (void);
int (*after_fork) (void);
- int (*list_exports) (int readonly, int default_only,
+ int (*list_exports) (int readonly, int is_tls,
struct nbdkit_exports *exports);
const char * (*default_export) (int readonly, int is_tls);
};
diff --git a/server/internal.h b/server/internal.h
index f11f897e..fe485117 100644
--- a/server/internal.h
+++ b/server/internal.h
@@ -370,7 +370,7 @@ struct backend {
void (*get_ready) (struct backend *);
void (*after_fork) (struct backend *);
int (*preconnect) (struct backend *, int readonly);
- int (*list_exports) (struct backend *, int readonly, int ignored,
+ int (*list_exports) (struct backend *, int readonly, int is_tls,
struct nbdkit_exports *exports);
const char *(*default_export) (struct backend *, int readonly, int is_tls);
void *(*open) (struct backend *, int readonly, const char *exportname,
@@ -419,9 +419,8 @@ extern void backend_unload (struct backend *b, void
(*unload) (void))
__attribute__((__nonnull__ (1)));
extern int backend_list_exports (struct backend *b, int readonly,
- int ignored,
struct nbdkit_exports *exports)
- __attribute__((__nonnull__ (1, 4)));
+ __attribute__((__nonnull__ (1, 3)));
extern const char *backend_default_export (struct backend *b, int readonly)
__attribute__((__nonnull__ (1)));
/* exportname is only valid for this call and almost certainly will be
diff --git a/server/backend.c b/server/backend.c
index 3e0a1d24..2023c40b 100644
--- a/server/backend.c
+++ b/server/backend.c
@@ -159,21 +159,20 @@ backend_unload (struct backend *b, void (*unload) (void))
}
int
-backend_list_exports (struct backend *b, int readonly, int default_only,
+backend_list_exports (struct backend *b, int readonly,
struct nbdkit_exports *exports)
{
GET_CONN;
struct handle *h = get_handle (conn, b->i);
size_t count;
- assert (!default_only); /* XXX Switch to is_tls... */
- controlpath_debug ("%s: list_exports readonly=%d default_only=%d",
- b->name, readonly, default_only);
+ controlpath_debug ("%s: list_exports readonly=%d tls=%d",
+ b->name, readonly, conn->using_tls);
assert (h->handle == NULL);
assert ((h->state & HANDLE_OPEN) == 0);
- if (b->list_exports (b, readonly, default_only, exports) == -1 ||
+ if (b->list_exports (b, readonly, conn->using_tls, exports) == -1 ||
exports_resolve_default (exports, b, readonly) == -1) {
controlpath_debug ("%s: list_exports failed", b->name);
return -1;
diff --git a/server/filters.c b/server/filters.c
index bb22be76..dd4417be 100644
--- a/server/filters.c
+++ b/server/filters.c
@@ -238,15 +238,15 @@ plugin_magic_config_key (struct backend *b)
}
static int
-filter_list_exports (struct backend *b, int readonly, int default_only,
+filter_list_exports (struct backend *b, int readonly, int is_tls,
struct nbdkit_exports *exports)
{
struct backend_filter *f = container_of (b, struct backend_filter, backend);
if (f->filter.list_exports)
return f->filter.list_exports (backend_list_exports, b->next,
- readonly, default_only, exports);
- return backend_list_exports (b->next, readonly, default_only, exports);
+ readonly, is_tls, exports);
+ return backend_list_exports (b->next, readonly, exports);
}
static const char *
diff --git a/server/plugins.c b/server/plugins.c
index 7425857e..8061b2e8 100644
--- a/server/plugins.c
+++ b/server/plugins.c
@@ -283,7 +283,7 @@ plugin_preconnect (struct backend *b, int readonly)
}
static int
-plugin_list_exports (struct backend *b, int readonly, int default_only,
+plugin_list_exports (struct backend *b, int readonly, int is_tls,
struct nbdkit_exports *exports)
{
struct backend_plugin *p = container_of (b, struct backend_plugin, backend);
@@ -291,7 +291,7 @@ plugin_list_exports (struct backend *b, int readonly, int
default_only,
if (!p->plugin.list_exports)
return nbdkit_add_default_export (exports);
- return p->plugin.list_exports (readonly, default_only, exports);
+ return p->plugin.list_exports (readonly, is_tls, exports);
}
static const char *
diff --git a/server/protocol-handshake-newstyle.c
b/server/protocol-handshake-newstyle.c
index 1777219f..7a034360 100644
--- a/server/protocol-handshake-newstyle.c
+++ b/server/protocol-handshake-newstyle.c
@@ -88,7 +88,7 @@ send_newstyle_option_reply_exportnames (uint32_t option)
exps = nbdkit_exports_new ();
if (exps == NULL)
return send_newstyle_option_reply (option, NBD_REP_ERR_TOO_BIG);
- if (backend_list_exports (top, read_only, false, exps) == -1)
+ if (backend_list_exports (top, read_only, exps) == -1)
return send_newstyle_option_reply (option, NBD_REP_ERR_PLATFORM);
for (i = 0; i < nbdkit_exports_count (exps); i++) {
diff --git a/plugins/cc/cc.c b/plugins/cc/cc.c
index 1d688056..381d1f7e 100644
--- a/plugins/cc/cc.c
+++ b/plugins/cc/cc.c
@@ -348,6 +348,14 @@ cc_preconnect (int readonly)
return 0;
}
+static int
+cc_list_exports (int readonly, int is_tls, struct nbdkit_exports *exports)
+{
+ if (subplugin.list_exports)
+ return subplugin.list_exports (readonly, is_tls, exports);
+ return nbdkit_add_default_export (exports);
+}
+
static const char *
cc_default_export (int readonly, int is_tls)
{
@@ -571,6 +579,7 @@ static struct nbdkit_plugin plugin = {
.after_fork = cc_after_fork,
.preconnect = cc_preconnect,
+ .list_exports = cc_list_exports,
.default_export = cc_default_export,
.open = cc_open,
.close = cc_close,
diff --git a/plugins/sh/methods.c b/plugins/sh/methods.c
index 55d6d535..a0446fb2 100644
--- a/plugins/sh/methods.c
+++ b/plugins/sh/methods.c
@@ -303,13 +303,12 @@ parse_exports (const char *script,
}
int
-sh_list_exports (int readonly, int default_only,
- struct nbdkit_exports *exports)
+sh_list_exports (int readonly, int is_tls, struct nbdkit_exports *exports)
{
const char *method = "list_exports";
const char *script = get_script (method);
const char *args[] = { script, method, readonly ? "true" :
"false",
- default_only ? "true" : "false",
NULL };
+ is_tls ? "true" : "false", NULL };
CLEANUP_FREE char *s = NULL;
size_t slen;
diff --git a/filters/log/log.c b/filters/log/log.c
index 4525f362..b6b967a3 100644
--- a/filters/log/log.c
+++ b/filters/log/log.c
@@ -247,16 +247,16 @@ output_return (struct handle *h, const char *act, uint64_t
id, int r, int *err)
/* List exports. */
static int
log_list_exports (nbdkit_next_list_exports *next, void *nxdata,
- int readonly, int default_only,
+ int readonly, int is_tls,
struct nbdkit_exports *exports)
{
static uint64_t id;
int r;
int err;
- output (NULL, "ListExports", ++id, "readonly=%d
default_only=%d ...",
- readonly, default_only);
- r = next (nxdata, readonly, default_only, exports);
+ output (NULL, "ListExports", ++id, "readonly=%d tls=%d
...",
+ readonly, is_tls);
+ r = next (nxdata, readonly, exports);
if (r == -1) {
err = errno;
output_return (NULL, "...ListExports", id, r, &err);
diff --git a/filters/tls-fallback/tls-fallback.c
b/filters/tls-fallback/tls-fallback.c
index 0fcc2bcf..c1e549df 100644
--- a/filters/tls-fallback/tls-fallback.c
+++ b/filters/tls-fallback/tls-fallback.c
@@ -74,7 +74,15 @@ tls_fallback_get_ready (nbdkit_next_get_ready *next, void
*nxdata,
return next (nxdata);
}
-/* TODO: list_exports needs is_tls parameter */
+static int
+tls_fallback_list_exports (nbdkit_next_list_exports *next, void *nxdata,
+ int readonly, int is_tls,
+ struct nbdkit_exports *exports)
+{
+ if (!is_tls)
+ return nbdkit_add_export (exports, "", NULL);
+ return next (nxdata, readonly, exports);
+}
static const char *
tls_fallback_default_export (nbdkit_next_default_export *next, void *nxdata,
@@ -192,7 +200,7 @@ static struct nbdkit_filter filter = {
.config = tls_fallback_config,
.config_help = tls_fallback_config_help,
.get_ready = tls_fallback_get_ready,
- /* XXX .list_exports needs is_tls parameter */
+ .list_exports = tls_fallback_list_exports,
.default_export = tls_fallback_default_export,
.open = tls_fallback_open,
.get_size = tls_fallback_get_size,
diff --git a/tests/test-tls-fallback.sh b/tests/test-tls-fallback.sh
index 94449f31..fb232bf9 100755
--- a/tests/test-tls-fallback.sh
+++ b/tests/test-tls-fallback.sh
@@ -35,7 +35,7 @@ set -e
set -x
requires_plugin sh
-requires nbdsh -c 'exit (not h.supports_tls ())'
+requires nbdsh -c 'print (h.set_full_info)' -c 'exit (not
h.supports_tls ())'
# Does the nbdkit binary support TLS?
if ! nbdkit --dump-config | grep -sq tls=yes; then
@@ -66,9 +66,12 @@ check () {
fi
}
case $1 in
- list_exports) echo NAMES; echo hello; echo world ;;
+ list_exports) check "$3"; echo INTERLEAVED
+ echo hello; echo world
+ echo world; echo tour ;;
default_export) check "$3"; echo hello ;;
open) check "$4"; echo $3 ;;
+ export_description) echo "=$2=" ;;
get_size) echo "$2" | wc -c ;;
pread) echo "$2" | dd skip=$4 count=$3 iflag=skip_bytes,count_bytes
;;
can_write | can_trim) exit 0 ;;
@@ -79,8 +82,19 @@ EOF
# Plaintext client sees only dummy volume
nbdsh -c '
import os
-h.set_export_name ("hello")
+
+def f (name, desc):
+ assert name == ""
+ assert desc == ""
+
+h.set_opt_mode (True)
+h.set_full_info (True)
h.connect_unix (os.environ["sock"])
+assert h.opt_list (f) == 1
+h.opt_info ()
+assert h.get_canonical_export_name() == ""
+h.set_export_name ("hello")
+h.opt_go ()
assert h.get_size () == 512
assert not h.can_trim()
assert h.pread (5, 0) == b"dummy"
@@ -89,11 +103,25 @@ assert h.pread (5, 0) == b"dummy"
# Encrypted client sees desired volumes
nbdsh -c '
import os
-h.set_export_name ("hello")
+
+def f (name, desc):
+ if name == "hello":
+ assert desc == "world"
+ elif name == "world":
+ assert desc == "tour"
+ else:
+ assert False
+
+h.set_opt_mode (True)
+h.set_full_info (True)
h.set_tls (nbd.TLS_REQUIRE)
h.set_tls_psk_file ("keys.psk")
h.set_tls_username ("qemu")
h.connect_unix (os.environ["sock"])
+assert h.opt_list (f) == 2
+h.opt_info ()
+assert h.get_canonical_export_name() == "hello"
+h.opt_go ()
assert h.get_size () == 6
assert h.can_trim ()
assert h.pread (5, 0) == b"hello"
diff --git a/tests/test-layers-filter.c b/tests/test-layers-filter.c
index f6acb448..b3575963 100644
--- a/tests/test-layers-filter.c
+++ b/tests/test-layers-filter.c
@@ -108,11 +108,11 @@ test_layers_filter_preconnect (nbdkit_next_preconnect
*next,
static int
test_layers_filter_list_exports (nbdkit_next_list_exports *next, void *nxdata,
- int readonly, int default_only,
+ int readonly, int is_tls,
struct nbdkit_exports *exports)
{
DEBUG_FUNCTION;
- return next (nxdata, readonly, default_only, exports);
+ return next (nxdata, readonly, exports);
}
static const char *
diff --git a/TODO b/TODO
index 9c32a94b..e4dd072f 100644
--- a/TODO
+++ b/TODO
@@ -71,12 +71,6 @@ General ideas for improvements
continue to keep their non-standard handshake while utilizing nbdkit
to prototype new behaviors in serving the kernel.
-* The current signature of .list_exports is awkward; it overloads the
- list response to NBD_OPT_LIST with the resolution of the default
- export name in .open, and it is missing a tls parameter. It is
- probably worth splitting out a new .default_export callback for the
- latter purpose, as well as fixing the signature for the former.
-
* At present, we ignore NBD_INFO_DESCRIPTION requests from a client
during NBD_OPT_GO. This may be easiest if we add a new
.get_description callback, called after .open alongside .get_size
--
2.28.0
Eric Blake
2020-Aug-27 02:16 UTC
[Libguestfs] [nbdkit PATCH v2 7/8] api: Add .export_description
I'm about to add an 'exportname' filter, and in the process, I
noticed
a few shortcomings in our API. Time to fix those before the 1.22
release locks our API in stone. Having .default_export makes it easy
to answer NBD_INFO_NAME in response to a client request during
NBD_OPT_GO, but answering NBD_INFO_DESCRIPTION is awkward - there's no
guarantee the export name was given with a description in
.list_exports. Note, however, that while we map "" into a canonical
name prior to .open (as the plugin is easier to write if it can just
assume the client already passed in the canonical name), we don't need
a description until after .open, alongside .get_size.
The recent libnbd 1.4 release has added 'nbd_set_full_info()' as the
knob for whether a client will request name and description during
NBD_OPT_GO; adding this functionality in nbdkit makes it easier to
test that API in libnbd.
---
docs/nbdkit-filter.pod | 9 ++++
docs/nbdkit-plugin.pod | 20 ++++++++
docs/nbdkit-protocol.pod | 14 ++++++
plugins/eval/nbdkit-eval-plugin.pod | 2 +
plugins/sh/nbdkit-sh-plugin.pod | 11 +++++
include/nbdkit-filter.h | 3 ++
include/nbdkit-plugin.h | 1 +
server/internal.h | 3 ++
server/backend.c | 22 +++++++++
server/filters.c | 13 +++++
server/plugins.c | 13 +++++
server/protocol-handshake-newstyle.c | 25 ++++++++--
plugins/sh/methods.h | 1 +
plugins/cc/cc.c | 59 +++++++++++++----------
plugins/eval/eval.c | 70 ++++++++++++++-------------
plugins/sh/methods.c | 32 +++++++++++++
plugins/sh/sh.c | 71 ++++++++++++++--------------
filters/tls-fallback/tls-fallback.c | 42 +++++++++-------
tests/test-export-info.sh | 34 ++++++-------
tests/test-tls-fallback.sh | 6 +++
TODO | 5 --
21 files changed, 320 insertions(+), 136 deletions(-)
diff --git a/docs/nbdkit-filter.pod b/docs/nbdkit-filter.pod
index 79c39f0d..b6a73f96 100644
--- a/docs/nbdkit-filter.pod
+++ b/docs/nbdkit-filter.pod
@@ -551,6 +551,15 @@ This function is only called once per connection and cached
by nbdkit.
Similarly, repeated calls to C<next_ops-E<gt>get_size> will return
a
cached value.
+=head2 C<.export_description>
+
+ const char *export_description (struct nbdkit_next_ops *next_ops,
+ void *nxdata, void *handle);
+
+This intercepts the plugin C<.export_description> method and can be
+used to read or modify the export description that the NBD client
+will see.
+
=head2 C<.can_write>
=head2 C<.can_flush>
diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod
index 9f5d6356..18412b6a 100644
--- a/docs/nbdkit-plugin.pod
+++ b/docs/nbdkit-plugin.pod
@@ -814,6 +814,26 @@ to get the size (in bytes) of the block device being
exported.
The returned size must be E<ge> 0. If there is an error,
C<.get_size>
should call C<nbdkit_error> with an error message and return C<-1>.
+=head2 C<.export_description>
+
+ const char *export_description (void *handle);
+
+This is called during the option negotiation phase only if the
+client specifically requested an export description (see the
+C<NBD_INFO_DESCRIPTION> response to C<NBD_OPT_GO>). Any
+description provided must be human-readable UTF-8, no longer
+than 4096 bytes. Ideally, this description should match any
+description set during C<.list_exports>, but that is not
+enforced.
+
+If the plugin returns C<NULL> or an invalid string (such as longer
+than 4096 bytes), or if this callback is omitted, no description is
+offered to the client. As this is not an error in the protocol, it is
+not necessary to call C<nbdkit_error>. If the callback will not be
+returning a compile-time constant string, you may find
+C<nbdkit_strdup_intern> helpful for returning a value that avoids a
+memory leak.
+
=head2 C<.can_write>
int can_write (void *handle);
diff --git a/docs/nbdkit-protocol.pod b/docs/nbdkit-protocol.pod
index b923d367..2f46d735 100644
--- a/docs/nbdkit-protocol.pod
+++ b/docs/nbdkit-protocol.pod
@@ -200,6 +200,20 @@ request is no faster than a counterpart write would be,
while normal
zero requests still benefit from compressed network traffic regardless
of the time taken.
+=item C<NBD_INFO_NAME>, C<NBD_INFO_DESCRIPTION>
+
+Supported in nbdkit E<ge> 1.22.0.
+
+These protocol extensions allow a client to learn more information
+about an export during C<NBD_OPT_GO>. The C<.default_export>
callback
+can inform a client of a canonical non-empty name in place of the
+default export C<"">, and the C<.export_description>
callback can give
+a client more details about the export.
+
+=item C<NBD_INFO_BLOCK_SIZE>
+
+I<Not supported>.
+
=item Resize Extension
I<Not supported>.
diff --git a/plugins/eval/nbdkit-eval-plugin.pod
b/plugins/eval/nbdkit-eval-plugin.pod
index eb3c7f5a..22167ec5 100644
--- a/plugins/eval/nbdkit-eval-plugin.pod
+++ b/plugins/eval/nbdkit-eval-plugin.pod
@@ -100,6 +100,8 @@ features):
=item B<dump_plugin=>SCRIPT
+=item B<export_description=>SCRIPT
+
=item B<extents=>SCRIPT
=item B<flush=>SCRIPT
diff --git a/plugins/sh/nbdkit-sh-plugin.pod b/plugins/sh/nbdkit-sh-plugin.pod
index c6e9c432..fe86e53c 100644
--- a/plugins/sh/nbdkit-sh-plugin.pod
+++ b/plugins/sh/nbdkit-sh-plugin.pod
@@ -350,6 +350,17 @@ handle will be C<""> (empty string).
/path/to/script close <handle>
+=item C<export_description>
+
+ /path/to/script export_description <handle>
+
+The script should print a human-readable description of the disk image
+on stdout. If the description ends with a newline character then the
+newline is removed.
+
+This method is I<not> required; if it is absent, no export description
+will be provided to the client.
+
=item C<get_size>
/path/to/script get_size <handle>
diff --git a/include/nbdkit-filter.h b/include/nbdkit-filter.h
index 63458d59..afe83e0b 100644
--- a/include/nbdkit-filter.h
+++ b/include/nbdkit-filter.h
@@ -80,6 +80,7 @@ struct nbdkit_next_ops {
/* The rest of the next ops are the same as normal plugin operations. */
int64_t (*get_size) (nbdkit_backend *nxdata);
+ const char * (*export_description) (nbdkit_backend *nxdata);
int (*can_write) (nbdkit_backend *nxdata);
int (*can_flush) (nbdkit_backend *nxdata);
@@ -195,6 +196,8 @@ struct nbdkit_filter {
int64_t (*get_size) (struct nbdkit_next_ops *next_ops, nbdkit_backend
*nxdata,
void *handle);
+ const char * (*export_description) (struct nbdkit_next_ops *next_ops,
+ nbdkit_backend *nxdata, void *handle);
int (*can_write) (struct nbdkit_next_ops *next_ops, nbdkit_backend *nxdata,
void *handle);
diff --git a/include/nbdkit-plugin.h b/include/nbdkit-plugin.h
index 539e3f6e..db010a5a 100644
--- a/include/nbdkit-plugin.h
+++ b/include/nbdkit-plugin.h
@@ -143,6 +143,7 @@ struct nbdkit_plugin {
int (*list_exports) (int readonly, int is_tls,
struct nbdkit_exports *exports);
const char * (*default_export) (int readonly, int is_tls);
+ const char * (*export_description) (void *handle);
};
NBDKIT_EXTERN_DECL (void, nbdkit_set_error, (int err));
diff --git a/server/internal.h b/server/internal.h
index fe485117..e7619469 100644
--- a/server/internal.h
+++ b/server/internal.h
@@ -379,6 +379,7 @@ struct backend {
int (*finalize) (struct backend *, void *handle);
void (*close) (struct backend *, void *handle);
+ const char *(*export_description) (struct backend *, void *handle);
int64_t (*get_size) (struct backend *, void *handle);
int (*can_write) (struct backend *, void *handle);
int (*can_flush) (struct backend *, void *handle);
@@ -423,6 +424,8 @@ extern int backend_list_exports (struct backend *b, int
readonly,
__attribute__((__nonnull__ (1, 3)));
extern const char *backend_default_export (struct backend *b, int readonly)
__attribute__((__nonnull__ (1)));
+extern const char *backend_export_description (struct backend *b)
+ __attribute__((__nonnull__ (1)));
/* exportname is only valid for this call and almost certainly will be
* freed on return of this function, so backends must save the
* exportname if they need to refer to it later.
diff --git a/server/backend.c b/server/backend.c
index 2023c40b..89bf1015 100644
--- a/server/backend.c
+++ b/server/backend.c
@@ -360,6 +360,28 @@ backend_reopen (struct backend *b, int readonly, const char
*exportname)
return 0;
}
+const char *
+backend_export_description (struct backend *b)
+{
+ GET_CONN;
+ struct handle *h = get_handle (conn, b->i);
+ const char *s;
+
+ controlpath_debug ("%s: export_description", b->name);
+
+ assert (h->handle && (h->state & HANDLE_CONNECTED));
+ /* Caching is not useful for this value. */
+ s = b->export_description (b, h->handle);
+
+ /* Ignore over-length strings. XXX Also ignore non-UTF8? */
+ if (s && strnlen (s, NBD_MAX_STRING + 1) > NBD_MAX_STRING) {
+ controlpath_debug ("%s: export_description: ignoring invalid
string",
+ b->name);
+ s = NULL;
+ }
+ return s;
+}
+
int64_t
backend_get_size (struct backend *b)
{
diff --git a/server/filters.c b/server/filters.c
index dd4417be..54abf9a4 100644
--- a/server/filters.c
+++ b/server/filters.c
@@ -291,6 +291,7 @@ 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,
.can_flush = backend_can_flush,
@@ -334,6 +335,17 @@ filter_finalize (struct backend *b, void *handle)
return 0;
}
+static const char *
+filter_export_description (struct backend *b, void *handle)
+{
+ struct backend_filter *f = container_of (b, struct backend_filter, backend);
+
+ if (f->filter.export_description)
+ return f->filter.export_description (&next_ops, b->next, handle);
+ else
+ return backend_export_description (b->next);
+}
+
static int64_t
filter_get_size (struct backend *b, void *handle)
{
@@ -571,6 +583,7 @@ static struct backend filter_functions = {
.prepare = filter_prepare,
.finalize = filter_finalize,
.close = filter_close,
+ .export_description = filter_export_description,
.get_size = filter_get_size,
.can_write = filter_can_write,
.can_flush = filter_can_flush,
diff --git a/server/plugins.c b/server/plugins.c
index 8061b2e8..38db3a89 100644
--- a/server/plugins.c
+++ b/server/plugins.c
@@ -167,6 +167,7 @@ plugin_dump_fields (struct backend *b)
HAS (open);
HAS (close);
+ HAS (export_description);
HAS (get_size);
HAS (can_write);
HAS (can_flush);
@@ -366,6 +367,17 @@ plugin_close (struct backend *b, void *handle)
conn->exportname = NULL;
}
+static const char *
+plugin_export_description (struct backend *b, void *handle)
+{
+ struct backend_plugin *p = container_of (b, struct backend_plugin, backend);
+
+ if (p->plugin.export_description)
+ return p->plugin.export_description (handle);
+ else
+ return NULL;
+}
+
static int64_t
plugin_get_size (struct backend *b, void *handle)
{
@@ -772,6 +784,7 @@ static struct backend plugin_functions = {
.prepare = plugin_prepare,
.finalize = plugin_finalize,
.close = plugin_close,
+ .export_description = plugin_export_description,
.get_size = plugin_get_size,
.can_write = plugin_can_write,
.can_flush = plugin_can_flush,
diff --git a/server/protocol-handshake-newstyle.c
b/server/protocol-handshake-newstyle.c
index 7a034360..6d3e53c7 100644
--- a/server/protocol-handshake-newstyle.c
+++ b/server/protocol-handshake-newstyle.c
@@ -563,10 +563,10 @@ negotiate_handshake_newstyle_options (void)
exportsize) == -1)
return -1;
- /* For now we send NBD_INFO_NAME if requested, and ignore all
- * other info requests (including NBD_INFO_EXPORT if it was
- * requested, because we replied already above).
- * XXX NBD_INFO_DESCRIPTION is easy once we add .export_description.
+ /* For now we send NBD_INFO_NAME and NBD_INFO_DESCRIPTION if
+ * requested, and ignore all other info requests (including
+ * NBD_INFO_EXPORT if it was requested, because we replied
+ * already above).
*/
for (i = 0; i < nrinfos; ++i) {
memcpy (&info, &data[4 + exportnamelen + 2 + i*2], 2);
@@ -594,6 +594,23 @@ negotiate_handshake_newstyle_options (void)
return -1;
}
break;
+ case NBD_INFO_DESCRIPTION:
+ {
+ const char *desc = backend_export_description (top);
+
+ if (!desc) {
+ debug ("newstyle negotiation: %s: "
+ "NBD_INFO_DESCRIPTION: no description to
send",
+ optname);
+ break;
+ }
+ if (send_newstyle_option_reply_info_str (option,
+ NBD_REP_INFO,
+ NBD_INFO_DESCRIPTION,
+ desc, -1) == -1)
+ return -1;
+ }
+ break;
default:
debug ("newstyle negotiation: %s: "
"ignoring NBD_INFO_* request %u (%s)",
diff --git a/plugins/sh/methods.h b/plugins/sh/methods.h
index 3fd4ef42..42eb560c 100644
--- a/plugins/sh/methods.h
+++ b/plugins/sh/methods.h
@@ -49,6 +49,7 @@ extern int sh_list_exports (int readonly, int default_only,
extern const char *sh_default_export (int readonly, int is_tls);
extern void *sh_open (int readonly);
extern void sh_close (void *handle);
+extern const char *sh_export_description (void *handle);
extern int64_t sh_get_size (void *handle);
extern int sh_pread (void *handle, void *buf, uint32_t count, uint64_t offset,
uint32_t flags);
diff --git a/plugins/cc/cc.c b/plugins/cc/cc.c
index 381d1f7e..ae641088 100644
--- a/plugins/cc/cc.c
+++ b/plugins/cc/cc.c
@@ -377,6 +377,14 @@ cc_close (void *handle)
subplugin.close (handle);
}
+static const char *
+cc_export_description (void *handle)
+{
+ if (subplugin.export_description)
+ return subplugin.export_description (handle);
+ return NULL;
+}
+
static int64_t
cc_get_size (void *handle)
{
@@ -575,34 +583,35 @@ static struct nbdkit_plugin plugin = {
/* And we must provide callbacks for everything else, which are
* passed through to the subplugin.
*/
- .get_ready = cc_get_ready,
- .after_fork = cc_after_fork,
+ .get_ready = cc_get_ready,
+ .after_fork = cc_after_fork,
- .preconnect = cc_preconnect,
- .list_exports = cc_list_exports,
- .default_export = cc_default_export,
- .open = cc_open,
- .close = cc_close,
+ .preconnect = cc_preconnect,
+ .list_exports = cc_list_exports,
+ .default_export = cc_default_export,
+ .open = cc_open,
+ .close = cc_close,
- .get_size = cc_get_size,
- .can_write = cc_can_write,
- .can_flush = cc_can_flush,
- .is_rotational = cc_is_rotational,
- .can_trim = cc_can_trim,
- .can_zero = cc_can_zero,
- .can_fast_zero = cc_can_fast_zero,
- .can_extents = cc_can_extents,
- .can_fua = cc_can_fua,
- .can_multi_conn = cc_can_multi_conn,
- .can_cache = cc_can_cache,
+ .export_description = cc_export_description,
+ .get_size = cc_get_size,
+ .can_write = cc_can_write,
+ .can_flush = cc_can_flush,
+ .is_rotational = cc_is_rotational,
+ .can_trim = cc_can_trim,
+ .can_zero = cc_can_zero,
+ .can_fast_zero = cc_can_fast_zero,
+ .can_extents = cc_can_extents,
+ .can_fua = cc_can_fua,
+ .can_multi_conn = cc_can_multi_conn,
+ .can_cache = cc_can_cache,
- .pread = cc_pread,
- .pwrite = cc_pwrite,
- .flush = cc_flush,
- .trim = cc_trim,
- .zero = cc_zero,
- .extents = cc_extents,
- .cache = cc_cache,
+ .pread = cc_pread,
+ .pwrite = cc_pwrite,
+ .flush = cc_flush,
+ .trim = cc_trim,
+ .zero = cc_zero,
+ .extents = cc_extents,
+ .cache = cc_cache,
.errno_is_preserved = 1,
};
diff --git a/plugins/eval/eval.c b/plugins/eval/eval.c
index a123734e..ae84bd40 100644
--- a/plugins/eval/eval.c
+++ b/plugins/eval/eval.c
@@ -70,6 +70,7 @@ static const char *known_methods[] = {
"config_complete",
"default_export",
"dump_plugin",
+ "export_description",
"extents",
"flush",
"get_ready",
@@ -380,45 +381,46 @@ eval_config_complete (void)
#define THREAD_MODEL NBDKIT_THREAD_MODEL_PARALLEL
static struct nbdkit_plugin plugin = {
- .name = "eval",
- .version = PACKAGE_VERSION,
- .load = eval_load,
- .unload = eval_unload,
+ .name = "eval",
+ .version = PACKAGE_VERSION,
+ .load = eval_load,
+ .unload = eval_unload,
- .dump_plugin = sh_dump_plugin,
+ .dump_plugin = sh_dump_plugin,
- .config = eval_config,
- .config_complete = eval_config_complete,
- .config_help = eval_config_help,
- .thread_model = sh_thread_model,
- .get_ready = sh_get_ready,
- .after_fork = sh_after_fork,
+ .config = eval_config,
+ .config_complete = eval_config_complete,
+ .config_help = eval_config_help,
+ .thread_model = sh_thread_model,
+ .get_ready = sh_get_ready,
+ .after_fork = sh_after_fork,
- .preconnect = sh_preconnect,
- .list_exports = sh_list_exports,
- .default_export = sh_default_export,
- .open = sh_open,
- .close = sh_close,
+ .preconnect = sh_preconnect,
+ .list_exports = sh_list_exports,
+ .default_export = sh_default_export,
+ .open = sh_open,
+ .close = sh_close,
- .get_size = sh_get_size,
- .can_write = sh_can_write,
- .can_flush = sh_can_flush,
- .is_rotational = sh_is_rotational,
- .can_trim = sh_can_trim,
- .can_zero = sh_can_zero,
- .can_extents = sh_can_extents,
- .can_fua = sh_can_fua,
- .can_multi_conn = sh_can_multi_conn,
- .can_cache = sh_can_cache,
- .can_fast_zero = sh_can_fast_zero,
+ .export_description = sh_export_description,
+ .get_size = sh_get_size,
+ .can_write = sh_can_write,
+ .can_flush = sh_can_flush,
+ .is_rotational = sh_is_rotational,
+ .can_trim = sh_can_trim,
+ .can_zero = sh_can_zero,
+ .can_extents = sh_can_extents,
+ .can_fua = sh_can_fua,
+ .can_multi_conn = sh_can_multi_conn,
+ .can_cache = sh_can_cache,
+ .can_fast_zero = sh_can_fast_zero,
- .pread = sh_pread,
- .pwrite = sh_pwrite,
- .flush = sh_flush,
- .trim = sh_trim,
- .zero = sh_zero,
- .extents = sh_extents,
- .cache = sh_cache,
+ .pread = sh_pread,
+ .pwrite = sh_pwrite,
+ .flush = sh_flush,
+ .trim = sh_trim,
+ .zero = sh_zero,
+ .extents = sh_extents,
+ .cache = sh_cache,
.errno_is_preserved = 1,
};
diff --git a/plugins/sh/methods.c b/plugins/sh/methods.c
index a0446fb2..beafcd25 100644
--- a/plugins/sh/methods.c
+++ b/plugins/sh/methods.c
@@ -457,6 +457,38 @@ sh_close (void *handle)
}
}
+const char *
+sh_export_description (void *handle)
+{
+ const char *method = "export_description";
+ const char *script = get_script (method);
+ struct sh_handle *h = handle;
+ const char *args[] = { script, method, h->h, NULL };
+ CLEANUP_FREE char *s = NULL;
+ size_t slen;
+
+ switch (call_read (&s, &slen, args)) {
+ case OK:
+ if (slen > 0 && s[slen-1] == '\n')
+ s[slen-1] = '\0';
+ return nbdkit_strdup_intern (s);
+
+ case MISSING:
+ return NULL;
+
+ case ERROR:
+ return NULL;
+
+ case RET_FALSE:
+ nbdkit_error ("%s: %s method returned unexpected code (3/false)",
+ script, method);
+ errno = EIO;
+ return NULL;
+
+ default: abort ();
+ }
+}
+
int64_t
sh_get_size (void *handle)
{
diff --git a/plugins/sh/sh.c b/plugins/sh/sh.c
index 4fcc2a5a..db75d386 100644
--- a/plugins/sh/sh.c
+++ b/plugins/sh/sh.c
@@ -284,46 +284,47 @@ sh_config_complete (void)
#define THREAD_MODEL NBDKIT_THREAD_MODEL_PARALLEL
static struct nbdkit_plugin plugin = {
- .name = "sh",
- .version = PACKAGE_VERSION,
- .load = sh_load,
- .unload = sh_unload,
+ .name = "sh",
+ .version = PACKAGE_VERSION,
+ .load = sh_load,
+ .unload = sh_unload,
- .dump_plugin = sh_dump_plugin,
+ .dump_plugin = sh_dump_plugin,
- .config = sh_config,
- .config_complete = sh_config_complete,
- .config_help = sh_config_help,
- .magic_config_key = "script",
- .thread_model = sh_thread_model,
- .get_ready = sh_get_ready,
- .after_fork = sh_after_fork,
+ .config = sh_config,
+ .config_complete = sh_config_complete,
+ .config_help = sh_config_help,
+ .magic_config_key = "script",
+ .thread_model = sh_thread_model,
+ .get_ready = sh_get_ready,
+ .after_fork = sh_after_fork,
- .preconnect = sh_preconnect,
- .list_exports = sh_list_exports,
- .default_export = sh_default_export,
- .open = sh_open,
- .close = sh_close,
+ .preconnect = sh_preconnect,
+ .list_exports = sh_list_exports,
+ .default_export = sh_default_export,
+ .open = sh_open,
+ .close = sh_close,
- .get_size = sh_get_size,
- .can_write = sh_can_write,
- .can_flush = sh_can_flush,
- .is_rotational = sh_is_rotational,
- .can_trim = sh_can_trim,
- .can_zero = sh_can_zero,
- .can_extents = sh_can_extents,
- .can_fua = sh_can_fua,
- .can_multi_conn = sh_can_multi_conn,
- .can_cache = sh_can_cache,
- .can_fast_zero = sh_can_fast_zero,
+ .export_description = sh_export_description,
+ .get_size = sh_get_size,
+ .can_write = sh_can_write,
+ .can_flush = sh_can_flush,
+ .is_rotational = sh_is_rotational,
+ .can_trim = sh_can_trim,
+ .can_zero = sh_can_zero,
+ .can_extents = sh_can_extents,
+ .can_fua = sh_can_fua,
+ .can_multi_conn = sh_can_multi_conn,
+ .can_cache = sh_can_cache,
+ .can_fast_zero = sh_can_fast_zero,
- .pread = sh_pread,
- .pwrite = sh_pwrite,
- .flush = sh_flush,
- .trim = sh_trim,
- .zero = sh_zero,
- .extents = sh_extents,
- .cache = sh_cache,
+ .pread = sh_pread,
+ .pwrite = sh_pwrite,
+ .flush = sh_flush,
+ .trim = sh_trim,
+ .zero = sh_zero,
+ .extents = sh_extents,
+ .cache = sh_cache,
.errno_is_preserved = 1,
};
diff --git a/filters/tls-fallback/tls-fallback.c
b/filters/tls-fallback/tls-fallback.c
index c1e549df..bec983be 100644
--- a/filters/tls-fallback/tls-fallback.c
+++ b/filters/tls-fallback/tls-fallback.c
@@ -119,6 +119,15 @@ tls_fallback_open (nbdkit_next_open *next, void *nxdata,
int readonly,
* can_zero, can_fast_zero, and can_fua).
*/
+static const char *
+tls_fallback_export_description (struct nbdkit_next_ops *next_ops,
+ void *nxdata, void *handle)
+{
+ if (NOT_TLS)
+ return NULL;
+ return next_ops->export_description (nxdata);
+}
+
static int64_t
tls_fallback_get_size (struct nbdkit_next_ops *next_ops, void *nxdata,
void *handle)
@@ -195,22 +204,23 @@ tls_fallback_pread (struct nbdkit_next_ops *next_ops, void
*nxdata,
}
static struct nbdkit_filter filter = {
- .name = "tls-fallback",
- .longname = "nbdkit tls-fallback filter",
- .config = tls_fallback_config,
- .config_help = tls_fallback_config_help,
- .get_ready = tls_fallback_get_ready,
- .list_exports = tls_fallback_list_exports,
- .default_export = tls_fallback_default_export,
- .open = tls_fallback_open,
- .get_size = tls_fallback_get_size,
- .can_write = tls_fallback_can_write,
- .can_flush = tls_fallback_can_flush,
- .is_rotational = tls_fallback_is_rotational,
- .can_extents = tls_fallback_can_extents,
- .can_multi_conn = tls_fallback_can_multi_conn,
- .can_cache = tls_fallback_can_cache,
- .pread = tls_fallback_pread,
+ .name = "tls-fallback",
+ .longname = "nbdkit tls-fallback filter",
+ .config = tls_fallback_config,
+ .config_help = tls_fallback_config_help,
+ .get_ready = tls_fallback_get_ready,
+ .list_exports = tls_fallback_list_exports,
+ .default_export = tls_fallback_default_export,
+ .open = tls_fallback_open,
+ .export_description = tls_fallback_export_description,
+ .get_size = tls_fallback_get_size,
+ .can_write = tls_fallback_can_write,
+ .can_flush = tls_fallback_can_flush,
+ .is_rotational = tls_fallback_is_rotational,
+ .can_extents = tls_fallback_can_extents,
+ .can_multi_conn = tls_fallback_can_multi_conn,
+ .can_cache = tls_fallback_can_cache,
+ .pread = tls_fallback_pread,
};
NBDKIT_REGISTER_FILTER(filter)
diff --git a/tests/test-export-info.sh b/tests/test-export-info.sh
index 984d86c8..5d390835 100755
--- a/tests/test-export-info.sh
+++ b/tests/test-export-info.sh
@@ -43,7 +43,6 @@ rm -f $files
cleanup_fn rm -f $files
# Create an nbdkit sh plugin for checking NBD_INFO replies to NBD_OPT_GO.
-# XXX Update when .export_description is implemented in sh
start_nbdkit -P export-info.pid -U $sock \
sh - <<'EOF'
case "$1" in
@@ -59,36 +58,34 @@ EOF
nbdsh -c '
import os
+def must_fail (f, *args, **kwds):
+ try:
+ f ( *args, **kwds)
+ assert False
+ except nbd.Error as ex:
+ pass
+
h.set_opt_mode (True)
h.connect_unix (os.environ["sock"])
h.set_export_name ("a")
h.opt_info ()
-try:
- h.get_canonical_export_name ()
- assert False
-except nbd.Error as ex:
- pass
+must_fail (h.get_canonical_export_name)
+must_fail (h.get_export_description)
h.set_export_name ("")
h.opt_info ()
-try:
- h.get_canonical_export_name ()
- assert False
-except nbd.Error as ex:
- pass
+must_fail (h.get_canonical_export_name)
+must_fail (h.get_export_description)
h.opt_go ()
-try:
- h.get_canonical_export_name ()
- assert False
-except nbd.Error as ex:
- pass
+must_fail (h.get_canonical_export_name)
+must_fail (h.get_export_description)
h.shutdown ()
'
-# With client request, reflect the export name back
+# With client request, reflect the export name and description back
nbdsh -c '
import os
@@ -99,13 +96,16 @@ h.connect_unix (os.environ["sock"])
h.set_export_name ("a")
h.opt_info ()
assert h.get_canonical_export_name () == "a"
+assert h.get_export_description () == "a world"
h.set_export_name ("")
h.opt_info ()
assert h.get_canonical_export_name () == "hello"
+assert h.get_export_description () == "hello world"
h.opt_go ()
assert h.get_canonical_export_name () == "hello"
+assert h.get_export_description () == "hello world"
h.shutdown ()
'
diff --git a/tests/test-tls-fallback.sh b/tests/test-tls-fallback.sh
index fb232bf9..b6c92918 100755
--- a/tests/test-tls-fallback.sh
+++ b/tests/test-tls-fallback.sh
@@ -93,6 +93,11 @@ h.connect_unix (os.environ["sock"])
assert h.opt_list (f) == 1
h.opt_info ()
assert h.get_canonical_export_name() == ""
+try:
+ h.get_export_description()
+ assert False
+except nbd.Error as ex:
+ pass
h.set_export_name ("hello")
h.opt_go ()
assert h.get_size () == 512
@@ -121,6 +126,7 @@ h.connect_unix (os.environ["sock"])
assert h.opt_list (f) == 2
h.opt_info ()
assert h.get_canonical_export_name() == "hello"
+assert h.get_export_description() == "=hello="
h.opt_go ()
assert h.get_size () == 6
assert h.can_trim ()
diff --git a/TODO b/TODO
index e4dd072f..ff902c7d 100644
--- a/TODO
+++ b/TODO
@@ -71,11 +71,6 @@ General ideas for improvements
continue to keep their non-standard handshake while utilizing nbdkit
to prototype new behaviors in serving the kernel.
-* At present, we ignore NBD_INFO_DESCRIPTION requests from a client
- during NBD_OPT_GO. This may be easiest if we add a new
- .get_description callback, called after .open alongside .get_size
- and .can_write.
-
* Background thread for filters. Some filters (readahead, cache and
proposed scan filter - see below) could be more effective if they
were able to defer work to a background thread. However it's not as
--
2.28.0
Eric Blake
2020-Aug-27 02:16 UTC
[Libguestfs] [nbdkit PATCH v2 8/8] exportname: New filter
Add a new filter to make it easier to add exports to a plugin that
does advertise them, to avoid advertising where a plugin's list might
be an information leak, and to alter which export name is used in
place of "".
I would love to be able to have a strict mode enforcing that .open is
called only with an export name that the plugin was willing to
advertise, but for that, we'll need a way to call the plugin's
.list_exports from within the context of .open (since we can't
guarantee the guest will call NBD_OPT_LIST). So for now, strict mode
requires redundant exportname= command line parameters. It may also
be worth adding a way to pass exportname-file=/path/to/file, which
then gets parsed the same was as the sh plugin (right now with NAMES,
INTERLEAVED, or NAMES+DESCRIPTIONS), to avoid having to do so many
export names directly on the command line.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
.../exportname/nbdkit-exportname-filter.pod | 154 ++++++++
filters/ext2/nbdkit-ext2-filter.pod | 5 +
plugins/file/nbdkit-file-plugin.pod | 9 +-
configure.ac | 2 +
filters/exportname/Makefile.am | 67 ++++
tests/Makefile.am | 4 +
filters/exportname/exportname.c | 342 ++++++++++++++++++
tests/test-exportname.sh | 184 ++++++++++
TODO | 9 +
9 files changed, 773 insertions(+), 3 deletions(-)
create mode 100644 filters/exportname/nbdkit-exportname-filter.pod
create mode 100644 filters/exportname/Makefile.am
create mode 100644 filters/exportname/exportname.c
create mode 100755 tests/test-exportname.sh
diff --git a/filters/exportname/nbdkit-exportname-filter.pod
b/filters/exportname/nbdkit-exportname-filter.pod
new file mode 100644
index 00000000..4de56dee
--- /dev/null
+++ b/filters/exportname/nbdkit-exportname-filter.pod
@@ -0,0 +1,154 @@
+=head1 NAME
+
+nbdkit-exportname-filter - adjust export names between client and plugin
+
+=head1 SYNOPSIS
+
+ nbdkit --filter=exportname plugin [default-export=NAME]
+ [exportname-list=MODE] [exportname-strict=true] [exportname=NAME]...
+ [exportdesc=DESC]
+
+=head1 DESCRIPTION
+
+Some plugins (such as C<nbdkit-file-plugin(1)> and filters (such as
+C<nbdkit-ext2-filter(1)> are able to serve different content based on
+the export name requested by the client. The NBD protocol allows a
+server to advertise the set of export names it is serving. However,
+the list advertised (or absent) from the plugin may not always match
+what you want an actual client to see. This filter can be used to
+alter the advertised list, as well as configuring which export should
+be treated as the default when the client requests the empty string
+(C<"">) as an export name.
+
+=head1 PARAMETERS
+
+=over 4
+
+=item B<default-export=>NAME
+
+When the client requests the default export name (C<"">),
request the
+export C<NAME> from the underlying plugin instead of relying on the
+plugin's choice of default export. Setting NAME to the empty string
+has the same effect as omitting this parameter.
+
+=item B<exportname-list=keep>
+
+=item B<exportname-list=error>
+
+=item B<exportname-list=empty>
+
+=item B<exportname-list=defaultonly>
+
+=item B<exportname-list=explicit>
+
+This parameter determines which exports are advertised to a guest that
+requests a listing via C<NBD_OPT_LIST>. The default mode is
C<keep>
+to advertise whatever the underlying plugin reports. Mode C<error>
+causes clients to see an error rather than an export list. Mode
+C<empty> returns an empty list. Mode C<defaultonly> returns a list
+that contains only the canonical name of the default export. Mode
+C<explicit> returns only the exports set by C<exportname=>. Note
that
+the list of advertised exports need not reflect reality: an advertised
+name may be rejected, or a client may connect to an export name that
+was not advertised, but learned through other means.
+
+=item B<exportname-strict=false>
+
+=item B<exportname-strict=true>
+
+Normally, a client can pass whatever export name it wants, regardless
+of whether that name is advertised. But setting this parameter to
+true will cause the connection to fail if a client requests an export
+name that was not included via an B<exportname=> parameter. At this
+time, it is not possible to restrict a client to exports advertised by
+the plugin without repeating that list via B<exportname>; this
+technical limitation may be lifted in the future.
+
+=item B<exportname=>NAME
+
+This parameter adds C<NAME> to the list of advertised exports; it may
+be set multiple times.
+
+=item B<exportdesc=keep>
+
+=item B<exportdesc=none>
+
+=item B<exportdesc=fixed:>STRING
+
+=item B<exportdesc=script:>SCRIPT
+
+The C<exportdesc> parameter controls what optional descriptions are
+sent alongside an export name. If set to C<keep> (the default),
+descriptions are determined by the plugin. If set to C<none>,
+descriptions from the plugin are ignored (useful if you are worried
+about a potential information leak). If set to C<fixed:STRING>, the
+same fixed string description is offered for every export. If set to
+C<script:SCRIPT>, this filter executes script with C<$name> set to
the
+export to be described, and uses the output of that command as the
+description.
+
+=back
+
+=head1 EXAMPLES
+
+Suppose that the directory /path/to/dir contains permanent files named
+file1, file2, and file3. The following commands show various ways to
+alter the use of export names while serving that directory:
+
+Allow a client requesting C<""> to get the contents of file2,
rather
+than an error:
+
+ nbdkit --filter=exportname file dir=/path/to/dir default-export=file2
+
+Do not advertise any exports; a client must know in advance what
+export names to try:
+
+ nbdkit --filter=exportname file dir=/path/to/dir exportname-list=empty
+
+Allow clients to connect to file1 and file3, but not file2:
+
+ nbdkit --filter=exportname file dir=/path/to/dir \
+ exportname-list=explicit exportname-strict=true \
+ exportname=file1 exportname=file3
+
+Offer C<ls(3)> long descriptions alongside each export name:
+
+ nbdkit --filter=exportname file dir=/path/to/dir \
+ exportdesc=script:'ls -l /path/to/dir/"$name"'
+
+=head1 FILES
+
+=over 4
+
+=item F<$filterdir/nbdkit-exportname-filter.so>
+
+The filter.
+
+Use C<nbdkit --dump-config> to find the location of C<$filterdir>.
+
+=back
+
+=head1 VERSION
+
+C<nbdkit-exportname-filter> first appeared in nbdkit 1.22.
+
+=head1 SEE ALSO
+
+L<nbdkit(1)>,
+L<nbdkit-filter(3)>,
+L<nbdkit-ext2-filter(1)>,
+L<nbdkit-extentlist-filter(1)>,
+L<nbdkit-fua-filter(1)>,
+L<nbdkit-nocache-filter(1)>,
+L<nbdkit-noparallel-filter(1)>,
+L<nbdkit-nozero-filter(1)>,
+L<nbdkit-file-plugin(1)>,
+L<nbdkit-info-plugin(1)>.
+
+=head1 AUTHORS
+
+Eric Blake
+
+=head1 COPYRIGHT
+
+Copyright (C) 2020 Red Hat Inc.
diff --git a/filters/ext2/nbdkit-ext2-filter.pod
b/filters/ext2/nbdkit-ext2-filter.pod
index 78d27f8f..1aef9c2e 100644
--- a/filters/ext2/nbdkit-ext2-filter.pod
+++ b/filters/ext2/nbdkit-ext2-filter.pod
@@ -65,6 +65,10 @@ exportname passed by the client. Note that this mode allows
the
client to deduce which files exist within the disk image, which may be
a security risk in some situations.
+At present, when using this mode, the server does not advertise any
+particular exports; however, you may use
+L<nbdkit-exportname-filter(1)> to perform that task.
+
=back
=head1 FILES
@@ -89,6 +93,7 @@ and removed in nbdkit 1.22.
L<nbdkit(1)>,
L<nbdkit-plugin(3)>,
+L<nbdkit-exportname-filter(1)>,
L<nbdkit-partition-filter(1)>,
L<nbdkit-guestfs-plugin(1)>,
L<http://e2fsprogs.sourceforge.net/>,
diff --git a/plugins/file/nbdkit-file-plugin.pod
b/plugins/file/nbdkit-file-plugin.pod
index 3844d75f..f6f31a76 100644
--- a/plugins/file/nbdkit-file-plugin.pod
+++ b/plugins/file/nbdkit-file-plugin.pod
@@ -65,10 +65,12 @@ directory named C<DIRNAME>, including those found by
following
symbolic links. Other special files in the directory (such as
subdirectories, fifos, or Unix sockets) are ignored. When this mode
is used, the file to be served is chosen by the export name passed by
-the client. For now, the client must already know what files are
-in the directory, although there are plans to expose them via
+the client. For now, the client must already know what files are in
+the directory, although there are plans to expose them via
NBD_OPT_LIST; likewise, a client that requests the default export
-(C<"">) will be rejected. For security, when using directory
mode,
+(C<"">) will be rejected. However, you can use
+L<nbdkit-exportname-filter(1)> to adjust what export names the client
+sees or uses as a default. For security, when using directory mode,
this plugin will not accept export names containing slash (C</>).
=back
@@ -183,6 +185,7 @@ L<nbdkit-plugin(3)>,
L<nbdkit-split-plugin(1)>,
L<nbdkit-partitioning-plugin(1)>,
L<nbdkit-tmpdisk-plugin(1)>,
+L<nbdkit-exportname-filter(1)>,
L<nbdkit-fua-filter(1)>,
L<nbdkit-noextents-filter(1)>.
diff --git a/configure.ac b/configure.ac
index c0142b62..a11a3c59 100644
--- a/configure.ac
+++ b/configure.ac
@@ -104,6 +104,7 @@ filters="\
delay \
error \
exitlast \
+ exportname \
ext2 \
extentlist \
fua \
@@ -1161,6 +1162,7 @@ AC_CONFIG_FILES([Makefile
filters/delay/Makefile
filters/error/Makefile
filters/exitlast/Makefile
+ filters/exportname/Makefile
filters/ext2/Makefile
filters/extentlist/Makefile
filters/fua/Makefile
diff --git a/filters/exportname/Makefile.am b/filters/exportname/Makefile.am
new file mode 100644
index 00000000..1bf54518
--- /dev/null
+++ b/filters/exportname/Makefile.am
@@ -0,0 +1,67 @@
+# nbdkit
+# Copyright (C) 2018-2020 Red Hat Inc.
+#
+# Redistribution and use in source and binary forms, with or without
+# modification, are permitted provided that the following conditions are
+# met:
+#
+# * Redistributions of source code must retain the above copyright
+# notice, this list of conditions and the following disclaimer.
+#
+# * Redistributions in binary form must reproduce the above copyright
+# notice, this list of conditions and the following disclaimer in the
+# documentation and/or other materials provided with the distribution.
+#
+# * Neither the name of Red Hat nor the names of its contributors may be
+# used to endorse or promote products derived from this software without
+# specific prior written permission.
+#
+# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS
IS'' AND
+# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
+# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
+# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
+# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+# SUCH DAMAGE.
+
+include $(top_srcdir)/common-rules.mk
+
+EXTRA_DIST = nbdkit-exportname-filter.pod
+
+filter_LTLIBRARIES = nbdkit-exportname-filter.la
+
+nbdkit_exportname_filter_la_SOURCES = \
+ exportname.c \
+ $(top_srcdir)/include/nbdkit-filter.h \
+ $(NULL)
+
+nbdkit_exportname_filter_la_CPPFLAGS = \
+ -I$(top_srcdir)/include \
+ -I$(top_srcdir)/common/include \
+ -I$(top_srcdir)/common/utils \
+ $(NULL)
+nbdkit_exportname_filter_la_CFLAGS = $(WARNINGS_CFLAGS)
+nbdkit_exportname_filter_la_LDFLAGS = \
+ -module -avoid-version -shared $(SHARED_LDFLAGS) \
+ -Wl,--version-script=$(top_srcdir)/filters/filters.syms \
+ $(NULL)
+nbdkit_exportname_filter_la_LIBADD = \
+ $(top_builddir)/common/utils/libutils.la \
+ $(NULL)
+
+if HAVE_POD
+
+man_MANS = nbdkit-exportname-filter.1
+CLEANFILES += $(man_MANS)
+
+nbdkit-exportname-filter.1: nbdkit-exportname-filter.pod
+ $(PODWRAPPER) --section=1 --man $@ \
+ --html $(top_builddir)/html/$@.html \
+ $<
+
+endif HAVE_POD
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 29c4811b..a316decb 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -1321,6 +1321,10 @@ EXTRA_DIST += \
TESTS += test-exitlast.sh
EXTRA_DIST += test-exitlast.sh
+# exportname filter test.
+TESTS += test-exportname.sh
+EXTRA_DIST += test-exportname.sh
+
# ext2 filter test.
if HAVE_MKE2FS_WITH_D
if HAVE_EXT2
diff --git a/filters/exportname/exportname.c b/filters/exportname/exportname.c
new file mode 100644
index 00000000..910030ec
--- /dev/null
+++ b/filters/exportname/exportname.c
@@ -0,0 +1,342 @@
+/* nbdkit
+ * Copyright (C) 2020 Red Hat Inc.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are
+ * met:
+ *
+ * * Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ *
+ * * Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in the
+ * documentation and/or other materials provided with the distribution.
+ *
+ * * Neither the name of Red Hat nor the names of its contributors may be
+ * used to endorse or promote products derived from this software without
+ * specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS
IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+ * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
+ * PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
+ * USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+ * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+ * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+ * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+
+#include <config.h>
+
+#include <stdio.h>
+#include <stdbool.h>
+#include <string.h>
+
+#include <nbdkit-filter.h>
+
+#include "cleanup.h"
+#include "utils.h"
+
+static const char *default_export;
+static enum {
+ LIST_KEEP,
+ LIST_ERROR,
+ LIST_EMPTY,
+ LIST_DEFAULT,
+ LIST_EXPLICIT,
+} list;
+static bool strict;
+static enum {
+ DESC_KEEP,
+ DESC_NONE,
+ DESC_FIXED,
+ DESC_SCRIPT,
+} desc_mode;
+static const char *desc;
+struct nbdkit_exports *exports;
+
+static void
+exportname_load (void)
+{
+ exports = nbdkit_exports_new ();
+ if (!exports) {
+ nbdkit_error ("malloc: %m");
+ exit (EXIT_FAILURE);
+ }
+}
+
+static void
+exportname_unload (void)
+{
+ nbdkit_exports_free (exports);
+}
+
+/* Called for each key=value passed on the command line. */
+static int
+exportname_config (nbdkit_next_config *next, void *nxdata,
+ const char *key, const char *value)
+{
+ int r;
+
+ if (strcmp (key, "default-export") == 0 ||
+ strcmp (key, "default_export") == 0) {
+ default_export = value;
+ return 0;
+ }
+ if (strcmp (key, "exportname-list") == 0 ||
+ strcmp (key, "exportname_list") == 0) {
+ if (strcmp (value, "keep") == 0)
+ list = LIST_KEEP;
+ else if (strcmp (value, "error") == 0)
+ list = LIST_ERROR;
+ else if (strcmp (value, "empty") == 0)
+ list = LIST_EMPTY;
+ else if (strcmp (value, "defaultonly") == 0 ||
+ strcmp (value, "default-only") == 0)
+ list = LIST_DEFAULT;
+ else if (strcmp (value, "explicit") == 0)
+ list = LIST_EXPLICIT;
+ else {
+ nbdkit_error ("unrecognized exportname-list mode: %s", value);
+ return -1;
+ }
+ return 0;
+ }
+ if (strcmp (key, "exportname-strict") == 0 ||
+ strcmp (key, "exportname_strict") == 0) {
+ r = nbdkit_parse_bool (value);
+ if (r == -1)
+ return -1;
+ strict = r;
+ return 0;
+ }
+ if (strcmp (key, "exportname") == 0)
+ return nbdkit_add_export (exports, value, NULL);
+ if (strcmp (key, "exportdesc") == 0) {
+ if (strcmp (value, "keep") == 0)
+ desc_mode = DESC_KEEP;
+ else if (strcmp (value, "none") == 0) {
+ desc_mode = DESC_NONE;
+ desc = NULL;
+ }
+ else if (strncmp (value, "fixed:", 6) == 0) {
+ desc_mode = DESC_FIXED;
+ desc = value + 6;
+ }
+ else if (strncmp (value, "script:", 7) == 0) {
+ desc_mode = DESC_SCRIPT;
+ desc = value + 7;
+ }
+ else {
+ nbdkit_error ("unrecognized exportdesc mode: %s", value);
+ return -1;
+ }
+ return 0;
+ }
+ return next (nxdata, key, value);
+}
+
+#define exportname_config_help \
+ "default-export=<NAME> Canonical name for the \"\"
default export.\n" \
+ "exportname-list=<MODE> Which exports to advertise: keep
(default), error,\n" \
+ " empty, defaultonly, explicit.\n" \
+ "exportname-strict=<BOOL> Limit clients to explicit exports
(default false).\n" \
+ "exportname=<NAME> Add an explicit export name, may be
repeated.\n" \
+ "exportdesc=<MODE> Set descriptions according to mode:
keep (default),\n" \
+ " none, fixed:STRING, script:SCRIPT.\n" \
+
+static const char *
+get_desc (const char *name, const char *def)
+{
+ FILE *fp;
+ CLEANUP_FREE char *cmd = NULL;
+ size_t cmdlen = 0;
+ char buf[4096]; /* Maximum NBD string; we truncate any longer response */
+ size_t r;
+
+ switch (desc_mode) {
+ case DESC_KEEP:
+ return def;
+ case DESC_NONE:
+ case DESC_FIXED:
+ return desc;
+ case DESC_SCRIPT:
+ break;
+ default:
+ abort ();
+ }
+
+ /* Construct the command. */
+ fp = open_memstream (&cmd, &cmdlen);
+ if (fp == NULL) {
+ nbdkit_debug ("open_memstream: %m");
+ return NULL;
+ }
+ fprintf (fp, "export name; name=");
+ shell_quote (name, fp);
+ fprintf (fp, "\n%s\n", desc);
+ if (fclose (fp) == EOF) {
+ nbdkit_debug ("memstream failed: %m");
+ return NULL;
+ }
+ nbdkit_debug ("%s", cmd);
+ fp = popen (cmd, "r");
+ if (fp == NULL) {
+ nbdkit_debug ("popen: %m");
+ return NULL;
+ }
+
+ /* Now read the description */
+ r = fread (buf, 1, sizeof buf, fp);
+ if (r == 0 && ferror (fp)) {
+ nbdkit_debug ("fread: %m");
+ pclose (fp);
+ return NULL;
+ }
+ pclose (fp);
+ if (r && buf[r-1] == '\n')
+ r--;
+ return nbdkit_strndup_intern (buf, r);
+}
+
+static int
+exportname_list_exports (nbdkit_next_list_exports *next, void *nxdata,
+ int readonly, int is_tls,
+ struct nbdkit_exports *exps)
+{
+ size_t i;
+ struct nbdkit_exports *source;
+ CLEANUP_EXPORTS_FREE struct nbdkit_exports *exps2 = NULL;
+
+ switch (list) {
+ case LIST_KEEP:
+ source = exps2 = nbdkit_exports_new ();
+ if (exps2 == NULL)
+ return -1;
+ if (next (nxdata, readonly, exps2) == -1)
+ return -1;
+ break;
+ case LIST_ERROR:
+ nbdkit_error ("export list restricted by policy");
+ return -1;
+ case LIST_EMPTY:
+ return 0;
+ case LIST_DEFAULT:
+ return nbdkit_add_default_export (exps);
+ case LIST_EXPLICIT:
+ source = exports;
+ break;
+ default:
+ abort ();
+ }
+
+ for (i = 0; i < nbdkit_exports_count (source); i++) {
+ struct nbdkit_export e = nbdkit_get_export (source, i);
+
+ if (nbdkit_add_export (exps, e.name,
+ get_desc (e.name, e.description)) == -1)
+ return -1;
+ }
+ return 0;
+}
+
+static const char *
+exportname_default_export (nbdkit_next_default_export *next, void *nxdata,
+ int readonly, int is_tls)
+{
+ size_t i;
+
+ /* If we are strict, do not allow connection unless "" was
advertised. */
+ if (strict) {
+ for (i = 0; i < nbdkit_exports_count (exports); i++) {
+ if (nbdkit_get_export (exports, i).name[0] == '\0')
+ return default_export ?: "";
+ }
+ return NULL;
+ }
+
+ if (default_export)
+ return default_export;
+ return next (nxdata, readonly);
+}
+
+struct handle {
+ const char *name;
+};
+
+static void *
+exportname_open (nbdkit_next_open *next, void *nxdata,
+ int readonly, const char *exportname, int is_tls)
+{
+ size_t i;
+ struct handle *h;
+
+ if (strict) {
+ for (i = 0; i < nbdkit_exports_count (exports); i++) {
+ if (strcmp (nbdkit_get_export (exports, i).name, exportname) == 0)
+ break;
+ }
+ if (i == nbdkit_exports_count (exports)) {
+ nbdkit_error ("export '%s not found", exportname);
+ errno = ENOENT;
+ return NULL;
+ }
+ }
+
+ h = malloc (sizeof *h);
+ if (h == NULL) {
+ nbdkit_error ("malloc: %m");
+ return NULL;
+ }
+
+ h->name = nbdkit_strdup_intern (exportname);
+ if (h->name == NULL) {
+ free (h);
+ return NULL;
+ }
+ if (next (nxdata, readonly, exportname) == -1) {
+ free (h);
+ return NULL;
+ }
+
+ return h;
+}
+
+static void
+exportname_close (void *handle)
+{
+ free (handle);
+}
+
+static const char *
+exportname_export_description (struct nbdkit_next_ops *next_ops, void *nxdata,
+ void *handle)
+{
+ struct handle *h = handle;
+ const char *def = NULL;
+
+ if (desc_mode == DESC_KEEP)
+ def = next_ops->export_description (nxdata);
+
+ return get_desc (h->name, def);
+}
+
+static struct nbdkit_filter filter = {
+ .name = "exportname",
+ .longname = "nbdkit exportname filter",
+ .load = exportname_load,
+ .unload = exportname_unload,
+ .config = exportname_config,
+ .config_help = exportname_config_help,
+ .list_exports = exportname_list_exports,
+ .default_export = exportname_default_export,
+ .open = exportname_open,
+ .close = exportname_close,
+ .export_description = exportname_export_description,
+};
+
+NBDKIT_REGISTER_FILTER(filter)
diff --git a/tests/test-exportname.sh b/tests/test-exportname.sh
new file mode 100755
index 00000000..2d30df4e
--- /dev/null
+++ b/tests/test-exportname.sh
@@ -0,0 +1,184 @@
+#!/usr/bin/env bash
+# nbdkit
+# Copyright (C) 2019-2020 Red Hat Inc.
+#
+# Redistribution and use in source and binary forms, with or without
+# modification, are permitted provided that the following conditions are
+# met:
+#
+# * Redistributions of source code must retain the above copyright
+# notice, this list of conditions and the following disclaimer.
+#
+# * Redistributions in binary form must reproduce the above copyright
+# notice, this list of conditions and the following disclaimer in the
+# documentation and/or other materials provided with the distribution.
+#
+# * Neither the name of Red Hat nor the names of its contributors may be
+# used to endorse or promote products derived from this software without
+# specific prior written permission.
+#
+# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS
IS'' AND
+# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
+# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
+# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
+# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+# SUCH DAMAGE.
+
+source ./functions.sh
+set -e
+set -x
+
+requires_plugin sh
+requires nbdinfo --version
+requires nbdsh -c 'print (h.set_full_info)'
+requires jq --version
+
+files="exportname.out exportname.sh"
+rm -rf $files
+cleanup_fn rm -rf $files
+
+query='[ [.exports[]] | sort_by(."export-name")[] |
+ [."export-name", .description, ."export-size"] ]'
+fail=0
+
+cat >exportname.sh <<\EOF
+case $1 in
+ list_exports)
+ echo INTERLEAVED
+ echo a; echo x
+ echo b; echo y
+ echo c; echo z
+ ;;
+ default_export) echo a ;;
+ open) echo "$3" ;;
+ export_description | get_size)
+ case $2 in
+ a) echo 1 ;;
+ b) echo 2 ;;
+ c) echo 3 ;;
+ *) exit 1 ;
+ esac ;;
+ *) exit 2 ;;
+esac
+EOF
+chmod +x exportname.sh
+
+# Establish a baseline
+nbdkit -U - sh exportname.sh \
+ --run 'nbdinfo --json --list "$uri"' >
exportname.out
+cat exportname.out
+test "$(jq -c "$query" exportname.out)" = \
+
'[["a","x",1],["b","y",2],["c","z",3]]'
+
+# Set the default export
+nbdkit -U - --filter=exportname sh exportname.sh default-export= \
+ --run 'nbdinfo --no-content --json "$uri"' >
exportname.out
+cat exportname.out
+test "$(jq -c "$query" exportname.out)" =
'[["","1",1]]'
+
+nbdkit -U - --filter=exportname sh exportname.sh default-export=b \
+ --run 'nbdinfo --no-content --json "$uri"' >
exportname.out
+cat exportname.out
+test "$(jq -c "$query" exportname.out)" =
'[["b","2",2]]'
+
+# Test export list policies
+nbdkit -U - --filter=exportname sh exportname.sh exportname-list=keep \
+ --run 'nbdinfo --json --list "$uri"' >
exportname.out
+cat exportname.out
+test "$(jq -c "$query" exportname.out)" = \
+
'[["a","x",1],["b","y",2],["c","z",3]]'
+
+nbdkit -U - --filter=exportname sh exportname.sh exportname-list=error \
+ --run 'nbdinfo --json --list "$uri"' >
exportname.out && fail=1 || :
+test ! -s exportname.out
+
+nbdkit -U - --filter=exportname sh exportname.sh exportname-list=empty \
+ --run 'nbdinfo --json --list "$uri"' >
exportname.out
+cat exportname.out
+test "$(jq -c "$query" exportname.out)" = '[]'
+
+nbdkit -U - --filter=exportname sh exportname.sh default-export=b \
+ exportname-list=defaultonly exportname=a exportname=b \
+ --run 'nbdinfo --json --list "$uri"' >
exportname.out
+cat exportname.out
+test "$(jq -c "$query" exportname.out)" =
'[["b",null,2]]'
+
+nbdkit -U - --filter=exportname sh exportname.sh default-export=b \
+ exportname-list=defaultonly exportname=a exportname=b \
+ --run 'nbdinfo --json --list "$uri"' >
exportname.out
+cat exportname.out
+test "$(jq -c "$query" exportname.out)" =
'[["b",null,2]]'
+
+nbdkit -U - --filter=exportname sh exportname.sh \
+ exportname-list=explicit exportname=b exportname=a \
+ --run 'nbdinfo --json --list "$uri"' >
exportname.out
+cat exportname.out
+test "$(jq -c "$query" exportname.out)" =
'[["a",null,1],["b",null,2]]'
+
+# Test description modes with lists
+nbdkit -U - --filter=exportname sh exportname.sh exportdesc=keep \
+ --run 'nbdinfo --json --list "$uri"' >
exportname.out
+cat exportname.out
+test "$(jq -c "$query" exportname.out)" = \
+
'[["a","x",1],["b","y",2],["c","z",3]]'
+
+nbdkit -U - --filter=exportname sh exportname.sh exportdesc=none \
+ --run 'nbdinfo --json --list "$uri"' >
exportname.out
+cat exportname.out
+test "$(jq -c "$query" exportname.out)" = \
+
'[["a",null,1],["b",null,2],["c",null,3]]'
+
+nbdkit -U - --filter=exportname sh exportname.sh exportdesc=fixed:hi \
+ --run 'nbdinfo --json --list "$uri"' >
exportname.out
+cat exportname.out
+test "$(jq -c "$query" exportname.out)" = \
+
'[["a","hi",1],["b","hi",2],["c","hi",3]]'
+
+nbdkit -U - --filter=exportname sh exportname.sh \
+ exportdesc=script:'echo $name$name' \
+ --run 'nbdinfo --json --list "$uri"' >
exportname.out
+cat exportname.out
+test "$(jq -c "$query" exportname.out)" = \
+
'[["a","aa",1],["b","bb",2],["c","cc",3]]'
+
+# Test description modes with connections
+nbdkit -U - -e c --filter=exportname sh exportname.sh exportdesc=fixed:hi \
+ --run 'nbdinfo --no-content --json "$uri"' >
exportname.out
+cat exportname.out
+test "$(jq -c "$query" exportname.out)" =
'[["c","hi",3]]'
+
+nbdkit -U - -e c --filter=exportname sh exportname.sh \
+ exportdesc=script:'echo $name$name' \
+ --run 'nbdinfo --no-content --json "$uri"' >
exportname.out
+cat exportname.out
+test "$(jq -c "$query" exportname.out)" =
'[["c","cc",3]]'
+
+# Test strict mode
+nbdkit -U - --filter=exportname sh exportname.sh exportname-strict=true \
+ --run 'nbdinfo --no-content --json "$uri"' >
exportname.out && fail=1
+test ! -s exportname.out
+
+nbdkit -U - --filter=exportname sh exportname.sh exportname-strict=true \
+ exportname=a exportname=b exportname=c \
+ --run 'nbdinfo --no-content --json "$uri"' >
exportname.out && fail=1
+test ! -s exportname.out
+
+nbdkit -U - --filter=exportname sh exportname.sh exportname-strict=true \
+ exportname=a exportname=b exportname= default-export=a\
+ --run 'nbdinfo --no-content --json "$uri"' >
exportname.out
+cat exportname.out
+test "$(jq -c "$query" exportname.out)" =
'[["a","1",1]]'
+
+nbdkit -U - -e a --filter=exportname sh exportname.sh exportname-strict=true \
+ exportname=a exportname=b exportname=c \
+ --run 'nbdinfo --no-content --json "$uri"' >
exportname.out
+cat exportname.out
+test "$(jq -c "$query" exportname.out)" =
'[["a","1",1]]'
+
+exit $fail
diff --git a/TODO b/TODO
index ff902c7d..2032b27b 100644
--- a/TODO
+++ b/TODO
@@ -252,6 +252,15 @@ nbdkit-tls-fallback-filter:
* Adjust handling of .list_exports to mask what the plugin advertises
when tls is not yet active.
+nbdkit-exportname-fitler:
+
+* find a way to call the plugin's .list_exports during .open, so that
+ we can enforce exportname-strict=true without command line redundancy
+
+* add a mode for passing in a file containing exportnames in the same
+ manner accepted by the sh/eval plugins, rather than one name (and no
+ description) per config parameter
+
Filters for security
--------------------
--
2.28.0
Richard W.M. Jones
2020-Aug-27 07:57 UTC
Re: [Libguestfs] [nbdkit PATCH v2 2/8] api: Add nbdkit_add_default_export
On Wed, Aug 26, 2020 at 09:16:46PM -0500, Eric Blake wrote:> Rather than defaulting .list_exports to blindly returning "", it is > nicer to have it reflect the result of .default_export. Meanwhile, > language bindings will have a C callback for both .list_exports and > .default_export, even if the underlying script does not, which means > any logic in plugins.c for calling .default_export when .list_export > is missing would have to be duplicated in each language binding. > Better is to make it easy for languages, by wrapping things in a new > helper function; this also lets us take maximum advantage of caching > done in backend.c (since language bindings can't access our internals, > they would have to duplicate caching if we did not add this helper > function).I'm a bit confused by how nbdkit_add_default_export() should be used and the documentation additions in this patch definitely need some work, but I'm going to assume it's used like this: my_list_exports (exports) { nbdkit_add_export (exports, "", some_description); /* Mark the most recently added export as the default: */ nbdkit_add_default_export (exports); /* Some other exports: */ nbdkit_add_export (exports, "foo", NULL); nbdkit_add_export (exports, "bar", NULL); } You could then argue that .default_export should call nbdkit_add_export + nbdkit_add_default_export instead of having to deal with all the interning stuff ... What do you think about that change? my_default_export (exports) { /* nbdkit_add_export should only be called once. */ nbdkit_add_export (exports, "", some_description); nbdkit_add_default_export (exports); } But then again this is differently awkward to consume from the server side. And if we did this I can't really see why we need the .default_export callback at all. The server could just call .list_exports early on and keep track internally of the exports and default export name that the plugin advertises. Rich.> Signed-off-by: Eric Blake <eblake@redhat.com> > --- > docs/nbdkit-plugin.pod | 21 +++++++++++++-------- > include/nbdkit-common.h | 2 ++ > server/internal.h | 4 ++++ > server/backend.c | 15 ++++++++------- > server/exports.c | 24 ++++++++++++++++++++++++ > server/nbdkit.syms | 1 + > server/plugins.c | 2 +- > server/test-public.c | 9 ++++++++- > plugins/sh/methods.c | 2 +- > tests/test-layers.c | 12 ++++++++++++ > 10 files changed, 74 insertions(+), 18 deletions(-) > > diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod > index 1f208b27..50cf991d 100644 > --- a/docs/nbdkit-plugin.pod > +++ b/docs/nbdkit-plugin.pod > @@ -675,10 +675,11 @@ error message and return C<-1>. > > This optional callback is called if the client tries to list the > exports served by the plugin (using C<NBD_OPT_LIST>). If the plugin > -does not supply this callback then a single export called C<""> is > -returned. The NBD protocol defines C<""> as the default export, so > -this is suitable for plugins which ignore the export name and always > -serve the same content. See also L</EXPORT NAME> below. > +does not supply this callback then the result of C<.default_export> is > +advertised as the lone export. The NBD protocol defines C<""> as the > +default export, so this is suitable for plugins which ignore the > +export name and always serve the same content. See also L</EXPORT > +NAME> below. > > The C<readonly> flag informs the plugin that the server was started > with the I<-r> flag on the command line, which is the same value > @@ -694,12 +695,13 @@ C<"">). The plugin can ignore this flag and return all exports if it > wants. > > The C<exports> parameter is an opaque object for collecting the list > -of exports. Call C<nbdkit_add_export> to add a single export to the > -list. If the plugin has a concept of a default export (usually but > -not always called C<"">) then it should return that first in the list. > +of exports. Call C<nbdkit_add_export> as needed to add specific > +exports to the list, or C<nbdkit_add_default_export> to add a single > +entry based on the results of C<.default_export>. > > int nbdkit_add_export (struct nbdkit_export *exports, > const char *name, const char *description); > + int nbdkit_add_default_export (struct nbdkit_export *exports); > > The C<name> must be a non-NULL, UTF-8 string between 0 and 4096 bytes > in length. Export names must be unique. C<description> is an > @@ -725,7 +727,10 @@ default export C<"">, where the plugin provides a UTF-8 string between > connection continues with the empty name; if the plugin returns a > valid string, nbdkit returns that name to clients that support it (see > the C<NBD_INFO_NAME> response to C<NBD_OPT_GO>), and behaves as if the > -client had passed that string instead of an empty name. > +client had passed that string instead of an empty name. Similarly, if > +the plugin does not supply a C<.list_exports> callback, the result of > +this callback determines what export name to advertise to a client > +requesting an export list. > > The C<readonly> flag informs the plugin that the server was started > with the I<-r> flag on the command line, which is the same value > diff --git a/include/nbdkit-common.h b/include/nbdkit-common.h > index c377e18d..5e8c0b6f 100644 > --- a/include/nbdkit-common.h > +++ b/include/nbdkit-common.h > @@ -130,6 +130,8 @@ struct nbdkit_exports; > NBDKIT_EXTERN_DECL (int, nbdkit_add_export, > (struct nbdkit_exports *, > const char *name, const char *description)); > +NBDKIT_EXTERN_DECL (int, nbdkit_add_default_export, > + (struct nbdkit_exports *)); > > /* A static non-NULL pointer which can be used when you don't need a > * per-connection handle. > diff --git a/server/internal.h b/server/internal.h > index 8c8448e6..e2a68513 100644 > --- a/server/internal.h > +++ b/server/internal.h > @@ -543,4 +543,8 @@ extern struct connection *threadlocal_get_conn (void); > struct connection *conn = threadlocal_get_conn (); \ > assert (conn != NULL) > > +/* exports.c */ > +extern int exports_resolve_default (struct nbdkit_exports *exports, > + struct backend *b, int readonly); > + > #endif /* NBDKIT_INTERNAL_H */ > diff --git a/server/backend.c b/server/backend.c > index 00e65e3c..3e0a1d24 100644 > --- a/server/backend.c > +++ b/server/backend.c > @@ -164,7 +164,7 @@ backend_list_exports (struct backend *b, int readonly, int default_only, > { > GET_CONN; > struct handle *h = get_handle (conn, b->i); > - int r; > + size_t count; > > assert (!default_only); /* XXX Switch to is_tls... */ > controlpath_debug ("%s: list_exports readonly=%d default_only=%d", > @@ -173,14 +173,15 @@ backend_list_exports (struct backend *b, int readonly, int default_only, > assert (h->handle == NULL); > assert ((h->state & HANDLE_OPEN) == 0); > > - r = b->list_exports (b, readonly, default_only, exports); > - if (r == -1) > + if (b->list_exports (b, readonly, default_only, exports) == -1 || > + exports_resolve_default (exports, b, readonly) == -1) { > controlpath_debug ("%s: list_exports failed", b->name); > - else { > - size_t count = nbdkit_exports_count (exports); > - controlpath_debug ("%s: list_exports returned %zu names", b->name, count); > + return -1; > } > - return r; > + > + count = nbdkit_exports_count (exports); > + controlpath_debug ("%s: list_exports returned %zu names", b->name, count); > + return 0; > } > > const char * > diff --git a/server/exports.c b/server/exports.c > index 8d3faec4..3259fbdb 100644 > --- a/server/exports.c > +++ b/server/exports.c > @@ -52,6 +52,7 @@ DEFINE_VECTOR_TYPE(exports, struct nbdkit_export); > > struct nbdkit_exports { > exports exports; > + bool use_default; > }; > > struct nbdkit_exports * > @@ -65,6 +66,7 @@ nbdkit_exports_new (void) > return NULL; > } > r->exports = (exports) empty_vector; > + r->use_default = false; > return r; > } > > @@ -141,3 +143,25 @@ nbdkit_add_export (struct nbdkit_exports *exps, > > return 0; > } > + > +int > +nbdkit_add_default_export (struct nbdkit_exports *exps) > +{ > + exps->use_default = true; > + return 0; > +} > + > +int > +exports_resolve_default (struct nbdkit_exports *exps, struct backend *b, > + int readonly) > +{ > + const char *def = NULL; > + > + if (exps->use_default) { > + def = backend_default_export (b, readonly); > + exps->use_default = false; > + } > + if (def) > + return nbdkit_add_export (exps, def, NULL); > + return 0; > +} > diff --git a/server/nbdkit.syms b/server/nbdkit.syms > index a67669b7..212e36aa 100644 > --- a/server/nbdkit.syms > +++ b/server/nbdkit.syms > @@ -39,6 +39,7 @@ > # The functions we want plugins and filters to call. > global: > nbdkit_absolute_path; > + nbdkit_add_default_export; > nbdkit_add_export; > nbdkit_add_extent; > nbdkit_debug; > diff --git a/server/plugins.c b/server/plugins.c > index 8172759f..da28b2f1 100644 > --- a/server/plugins.c > +++ b/server/plugins.c > @@ -289,7 +289,7 @@ plugin_list_exports (struct backend *b, int readonly, int default_only, > struct backend_plugin *p = container_of (b, struct backend_plugin, backend); > > if (!p->plugin.list_exports) > - return nbdkit_add_export (exports, "", NULL); > + return nbdkit_add_default_export (exports); > > return p->plugin.list_exports (readonly, default_only, exports); > } > diff --git a/server/test-public.c b/server/test-public.c > index 0149dd9b..ee71f2fc 100644 > --- a/server/test-public.c > +++ b/server/test-public.c > @@ -66,7 +66,14 @@ threadlocal_get_conn (void) > abort (); > } > > -int connection_get_status (void) > +int > +connection_get_status (void) > +{ > + abort (); > +} > + > +const char * > +backend_default_export (struct backend *b, int readonly) > { > abort (); > } > diff --git a/plugins/sh/methods.c b/plugins/sh/methods.c > index 8e2e4256..2192ae25 100644 > --- a/plugins/sh/methods.c > +++ b/plugins/sh/methods.c > @@ -316,7 +316,7 @@ sh_list_exports (int readonly, int default_only, > return parse_exports (script, s, slen, exports); > > case MISSING: > - return nbdkit_add_export (exports, "", NULL); > + return nbdkit_add_default_export (exports); > > case ERROR: > return -1; > diff --git a/tests/test-layers.c b/tests/test-layers.c > index db0a2da0..fa7730e6 100644 > --- a/tests/test-layers.c > +++ b/tests/test-layers.c > @@ -331,6 +331,18 @@ main (int argc, char *argv[]) > * than open-coding a naive client. > */ > > + /* .default_export methods called in outer-to-inner order. */ > + log_verify_seen_in_order > + ("testlayersfilter3: default_export", > + "filter3: test_layers_filter_default_export", > + "testlayersfilter2: default_export", > + "filter2: test_layers_filter_default_export", > + "testlayersfilter1: default_export", > + "filter1: test_layers_filter_default_export", > + "testlayersplugin: default_export", > + "test_layers_plugin_default_export", > + NULL); > + > /* open methods called in outer-to-inner order, but thanks to next > * pointer, complete in inner-to-outer order. */ > log_verify_seen_in_order > -- > 2.28.0 > > _______________________________________________ > Libguestfs mailing list > Libguestfs@redhat.com > https://www.redhat.com/mailman/listinfo/libguestfs-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org
Richard W.M. Jones
2020-Sep-01 14:24 UTC
Re: [Libguestfs] [nbdkit PATCH v2 4/8] api: Add nbdkit_str[n]dup_intern helper
On Wed, Aug 26, 2020 at 09:16:48PM -0500, Eric Blake wrote:> Implementing .default_export with its 'const char *' return is tricky > in the sh plugin: we must return dynamic memory, but must avoid a > use-after-free. And we don't want to change the return type of > .default_export to 'char *', because that would make our choice of > malloc()/free() part of the API, preventing either nbdkit or a plugin > from experimenting with an alternate allocator implementation. > Elsewhere, we have done things like nbdkit_extents_new(), so that even > though the client is directing allocation, the actual call to malloc() > is done by nbdkit proper, so that nbdkit later calling free() does not > tie the plugin's hands, and nbdkit could change its underlying > allocation without breaking ABI. (Note that nbdkit_realpath() and > nbdkit_absolute_path() require the caller to use free(), but as they > are used during .config, it's less of a burden for plugins to take > care of that in .unload.) > > We could document that the burden is on the plugin to avoid the memory > leak, by making sh track all strings it returns, then finally clean > them up during .unload. But this is still a memory leak in the case > of .default_export, which can be called multiple times when there are > multiple clients, and presumably with different values per client > call; better would be freeing the memory at .close when each client > goes away. Except that .default_export is called before .open, and > therefore before we have access to the handle struct that will > eventually make it to .close. > > So, the solution is to let nbdkit track an alternate string on our > behalf, where nbdkit then cleans it up as the client goes away. > > There are several places in the existing code base that can also take > advantage of this copying functionality, including in filters that > want to pass altered strings to .config while still obeying lifetime > rules.We should probably just push this one straight away, with one small change:> diff --git a/server/public.c b/server/public.c > index d10d466e..512e9caa 100644 > --- a/server/public.c > +++ b/server/public.cThis file is probably going to need: #include "strndup.h" somewhere near the top because unbelievably Win32 lacks this function. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html