7:41 p.m.
On 7/21/20 3:47 PM, Richard W.M. Jones wrote:
To allow filters to modify the export name as it passes through the
layers this commit makes several changes:
The filter .open callback now takes an extra parameter, the export
name. This is always non-NULL (for oldstyle it is ""). This string
has a short lifetime and filters that need to hang on to it must take
a copy. The filter must pass the exportname parameter down to the
next layer (or potentially modify it).
The plugin .open callback is unchanged, but the binding to it in
server/plugins.c takes a copy of the exportname in the handle and this
is what nbdkit_export_name returns to plugins. Eventually we will
deprecate that function and the V3 .open callback in plugins will have
the extra parameter, but this is not implemented yet.
nbdkit_export_name can only be called from plugins, not filters.
Filters should use the .open(...exportname) parameter if they need the
export name.
Filter .reopen also takes the exportname. The only filter that uses
it (retry) must save the exportname from .open in order to pass it to
.reopen. This filter already does the same for the readonly flag so
this seems reasonable.
The handling of NBD_OPT_SET_META_CONTEXT interacted with the export
name (see commit 20b8509a9ccdab118ce7b7043be63bbad74f1e79). I have
attempted to keep previous behaviour in this change, but note that
there is no regression test for this. I added a debug message so we
can tell when this unusual case actually happens which should help
with diagnosis of problems.
Yeah, that commit specifically mentioned that I used gdb breakpoints in
qemu-io to test things, and was not interested in hacking libnbd at the
time. Although now that we are debating about exposing nbd_opt_*
commands in libnbd to someone that opts in, maybe that _could_ be a case
to write such a regression test. Meanwhile, I'll just try to fire up
another gdb session to prove to myself that things still work.
All of the above is internal refactoring and should have no visible
external effect on server behaviour or how plugins are written.
Filters need to be updated as discussed above. All the in-tree
filters have been, and we assume there are no out of tree filters
because of the unstable filter API.
And even if there are, whoever maintains it can get a good idea of what
to do by reading this patch ;)
See also this thread:
https://www.redhat.com/archives/libguestfs/2020-July/thread.html#00087
---
+++ b/server/internal.h
@@ -246,8 +246,6 @@ struct connection {
struct handle *handles; /* One per plugin and filter. */
size_t nr_handles;
- char exportname[NBD_MAX_STRING + 1];
- uint32_t exportnamelen;
uint32_t cflags;
uint16_t eflags;
bool handshake_complete;
@@ -255,6 +253,9 @@ struct connection {
bool structured_replies;
bool meta_context_base_allocation;
+ char *exportname_from_set_meta_context;
+ char *exportname;
Interesting switch from array to pointer, but it should work.
+++ b/server/plugins.c
@@ -278,12 +278,30 @@ plugin_preconnect (struct backend *b, int readonly)
}
static void *
-plugin_open (struct backend *b, int readonly)
+plugin_open (struct backend *b, int readonly, const char *exportname)
{
+ GET_CONN;
struct backend_plugin *p = container_of (b, struct backend_plugin, backend);
assert (p->plugin.open != NULL);
+ /* Save the exportname since the lifetime of the string passed in
+ * here is likely to be brief. In addition this provides a place
+ * for nbdkit_export_name to retrieve it if called from the plugin.
+ *
+ * In API V3 we propose to pass the exportname as an extra parameter
+ * to the (new) plugin.open and deprecate nbdkit_export_name for V3
+ * users. Even then we will still need to save it in the handle
+ * because of the lifetime issue.
+ */
+ if (conn->exportname == NULL) {
Can't we assert(!conn->exportname) at this point? After all, we only
ever call .open at most once per connection.
+++ b/server/protocol-handshake-newstyle.c
@@ -200,11 +200,29 @@ conn_recv_full (void *buf, size_t len, const char *fmt, ...)
* in that function, and must not cause any wire traffic.
*/
static int
-finish_newstyle_options (uint64_t *exportsize)
+finish_newstyle_options (uint64_t *exportsize,
+ const char *exportname_in, uint32_t exportnamelen)
{
GET_CONN;
- if (protocol_common_open (exportsize, &conn->eflags) == -1)
+ /* Since the exportname string passed here comes directly out of the
+ * NBD protocol make a temporary copy of the exportname into a
+ * \0-terminated buffer.
+ */
+ CLEANUP_FREE char *exportname = strndup (exportname_in, exportnamelen);
+ if (exportname == NULL) {
+ nbdkit_error ("strndup: %m");
+ return -1;
+ }
+
+ if (conn->exportname_from_set_meta_context &&
+ strcmp (conn->exportname_from_set_meta_context, exportname) != 0) {
+ debug ("newstyle negotiation: NBD_OPT_SET_META_CONTEXT export name
\"%s\" ≠ final client exportname \"%s\", so discarding the previous
context",
Long line, and I don't know if we use UTF-8 in other debug messages or
should stick to straight ascii. Compilation may have a glitch if
compiled under a different locale than the end binary runs in, but these
days, it's uncommon to find someone running in a single-byte locale
instead of UTF-8.
+++ b/filters/ext2/ext2.c
/* Create the per-connection handle. */
static void *
-ext2_open (nbdkit_next_open *next, void *nxdata, int readonly)
+ext2_open (nbdkit_next_open *next, void *nxdata,
+ int readonly, const char *exportname)
+
+ /* If file == NULL (ie. using exportname) then don't
+ * pass the client exportname to the lower layers.
+ */
+ exportname = file ? exportname : "";
+
+ /* Request write access to the underlying plugin, for journal replay. */
+ if (next (nxdata, 0, exportname) == -1) {
Nice demonstration of when the filter changes things vs. does
pass-through, and will be easy enough to copy into the tar filter.
+ free (h->exportname);
+ free (h);
+ return NULL;
+ }
+
return h;
}
@@ -148,7 +166,7 @@ ext2_prepare (struct nbdkit_next_ops *next_ops, void *nxdata, void
*handle,
struct ext2_inode inode;
int64_t r;
CLEANUP_FREE char *name = NULL;
- const char *fname = file ?: nbdkit_export_name ();
+ const char *fname = file ?: h->exportname;
Hmm - we already pass the same 'readonly' state to filter's .prepare,
but not to backend_prepare(), which has to reconstruct it. Would it be
easier to also change the signature of backend_prepare() to take both
the original readonly and exportname passed to backend_open(), rather
than making the filter have to save it off in the filter? It looks like
protocol-handshake.c is the only caller, and still has everything in
scope at the time.
+++ b/filters/log/log.c
@@ -227,11 +227,12 @@ output_return (struct handle *h, const char *act, uint64_t id, int
r, int *err)
/* Open a connection. */
static void *
-log_open (nbdkit_next_open *next, void *nxdata, int readonly)
+log_open (nbdkit_next_open *next, void *nxdata,
+ int readonly, const char *exportname)
{
struct handle *h;
- if (next (nxdata, readonly) == -1)
+ if (next (nxdata, readonly, exportname) == -1)
return NULL;
Pre-existing - the log filter should include the exportname somewhere in
its output log. Well, nothing like the present to fix it ;)
+++ b/filters/retry/retry.c
@@ -106,16 +106,18 @@ retry_config (nbdkit_next_config *next, void *nxdata,
struct retry_handle {
int readonly; /* Save original readonly setting. */
+ const char *exportname; /* Client exportname. */
unsigned reopens;
bool open;
};
static void *
-retry_open (nbdkit_next_open *next, void *nxdata, int readonly)
+retry_open (nbdkit_next_open *next, void *nxdata,
+ int readonly, const char *exportname)
{
struct retry_handle *h;
- if (next (nxdata, readonly) == -1)
+ if (next (nxdata, readonly, exportname) == -1)
return NULL;
h = malloc (sizeof *h);
@@ -125,6 +127,12 @@ retry_open (nbdkit_next_open *next, void *nxdata, int readonly)
}
h->readonly = readonly;
+ h->exportname = strdup (exportname);
While I think we can avoid the strdup in the ext2 filter, I agree that
the retry filter does have to store it.
Otherwise looking good.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org