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

[Richard W.M. Jones]

On Tue, Jul 21, 2020 at 07:41:21PM -0500, Eric Blake wrote:
> On 7/21/20 3:47 PM, Richard W.M. Jones wrote:
> >+++ 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.

I don't think so - backend_reopen will call plugin_open a second time.
As a test I added assert (conn->exportname == NULL) before this line
and it crashed in tests/test-retry.sh.

> >+++ 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.

I thought we might be using ‘’ quotes (as we do in libguestfs) but I
see that we don't.  I think we should use Unicode characters more
where they are appropriate, but I'll break up this long line.  I also
added a comment.

> >@@ -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.

Possibly, although this would be a seperate change.

Thanks,

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v