[Libguestfs] [PATCH nbdkit] server: Pass the export name through filter .open calls.

[Eric Blake]

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