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 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 | 37 +++++++++++++++++++++++ include/nbdkit-filter.h | 9 ++++-- include/nbdkit-plugin.h | 1 + server/internal.h | 7 +++-- server/backend.c | 45 ++++++++++++++++++---------- server/exports.c | 8 +---- server/filters.c | 12 ++++++++ server/plugins.c | 22 ++++++++++++-- server/protocol-handshake-newstyle.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 | 24 ++++++++------- 16 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..64e3197b 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,37 @@ 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<"">. If the plugin does not supply this callback, +the connection continues with the empty name; if the plugin returns +a 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. 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 +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 +not requires TLS, be careful that a default export name does not leak +unintended information. + +If the plugin returns C<NULL>, 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/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..c5eef456 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,34 @@ 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); + 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 +222,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 218764da..924533cb 100644 --- a/server/plugins.c +++ b/server/plugins.c @@ -162,6 +162,7 @@ plugin_dump_fields (struct backend *b) HAS (after_fork); HAS (preconnect); HAS (list_exports); + HAS (default_export); HAS (open); HAS (close); HAS (get_size); @@ -282,15 +283,29 @@ 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) - return nbdkit_add_export (exports, "", NULL); + if (!p->plugin.list_exports) { + const char *def = backend_default_export (b, readonly); + if (def == NULL) + return 0; + return nbdkit_add_export (exports, def, NULL); + } 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) @@ -757,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/ondemand/ondemand.c b/plugins/ondemand/ondemand.c index f9d73ca6..5bd0ee70 100644 --- a/plugins/ondemand/ondemand.c +++ b/plugins/ondemand/ondemand.c @@ -219,6 +219,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; @@ -362,8 +369,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 || @@ -621,6 +627,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..fa7730e6 100644 --- a/tests/test-layers.c +++ b/tests/test-layers.c @@ -326,19 +326,21 @@ 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. */ + + /* .default_export methods called in outer-to-inner order. */ 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", + ("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 -- 2.28.0

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 | 114 +++++++++++++++++++++++++++ 3 files changed, 171 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..f9475e04 --- /dev/null +++ b/tests/test-export-info.sh @@ -0,0 +1,114 @@ +#!/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)' + +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 +export sock +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 +export sock +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

Instead of needing to provide .unload, we can let nbdkit manage the lifetime for us. However, if we like this approach, it may be wiser to introduce new variants of nbdkit_realpath/absolute_path which return const char * already intern'd. Signed-off-by: Eric Blake <eblake@redhat.com> --- See the cover letter: if we like this, there are several other plugins that could also reduce their .unload, or maybe we want to add convenience functions to make the combo 'nbdkit_realpath/nbdkit_string_intern' simpler. plugins/file/file.c | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/plugins/file/file.c b/plugins/file/file.c index 08418194..6a0aad93 100644 --- a/plugins/file/file.c +++ b/plugins/file/file.c @@ -64,8 +64,8 @@ #define fdatasync fsync #endif -static char *filename = NULL; -static char *directory = NULL; +static const char *filename = NULL; +static const char *directory = NULL; /* posix_fadvise mode: -1 = don't set it, or POSIX_FADV_*. */ static int fadvise_mode = @@ -91,13 +91,6 @@ is_enotsup (int err) return err == ENOTSUP || err == EOPNOTSUPP; } -static void -file_unload (void) -{ - free (filename); - free (directory); -} - /* Called for each key=value passed on the command line. This plugin * only accepts file=<filename> and dir=<dirname>, where exactly * one is required. @@ -111,15 +104,15 @@ file_config (const char *key, const char *value) * existence checks to the last possible moment. */ if (strcmp (key, "file") == 0) { - free (filename); - filename = nbdkit_realpath (value); + CLEANUP_FREE char *tmp = nbdkit_realpath (value); + filename = nbdkit_string_intern (tmp); if (!filename) return -1; } else if (strcmp (key, "directory") == 0 || strcmp (key, "dir") == 0) { - free (directory); - directory = nbdkit_realpath (value); + CLEANUP_FREE char *tmp = nbdkit_realpath (value); + directory = nbdkit_string_intern (value); if (!directory) return -1; } @@ -812,7 +805,6 @@ static struct nbdkit_plugin plugin = { .name = "file", .longname = "nbdkit file plugin", .version = PACKAGE_VERSION, - .unload = file_unload, .config = file_config, .config_complete = file_config_complete, .config_help = file_config_help, -- 2.28.0

Use the recently added nbdkit_string_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 | 54 +++++++++++++++++++++++++++-- plugins/sh/sh.c | 1 + plugins/sh/example.sh | 2 +- tests/test-eval-exports.sh | 35 ++++++++++++------- tests/test-tls-fallback.sh | 11 +++--- 9 files changed, 107 insertions(+), 21 deletions(-) diff --git a/plugins/eval/nbdkit-eval-plugin.pod b/plugins/eval/nbdkit-eval-plugin.pod index 7126c6de..eed97f11 100644 --- a/plugins/eval/nbdkit-eval-plugin.pod +++ b/plugins/eval/nbdkit-eval-plugin.pod @@ -110,6 +110,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 07fb3e48..a2631b39 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) { @@ -310,13 +312,18 @@ sh_list_exports (int readonly, int default_only, default_only ? "true" : "false", NULL }; CLEANUP_FREE char *s = NULL; size_t slen; + const char *def; switch (call_read (&s, &slen, args)) { case OK: return parse_exports (script, s, slen, exports); case MISSING: - return nbdkit_add_export (exports, "", NULL); + /* Match what is done for a C plugin. */ + def = sh_default_export (readonly, nbdkit_is_tls ()); + if (!def) + return 0; + return nbdkit_add_export (exports, def, NULL); case ERROR: return -1; @@ -331,6 +338,49 @@ 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; + CLEANUP_FREE char *name = NULL; + + 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; + name = strndup (p, n - p); + return nbdkit_string_intern (name); + + case MISSING: + return nbdkit_string_intern (""); + + 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-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