[Libguestfs] [PATCH nbdkit] New ondemand plugin.

[Richard W.M. Jones]

On Sat, Aug 15, 2020 at 03:41:39PM -0500, Eric Blake wrote:
> On 8/14/20 12:20 PM, Richard W.M. Jones wrote:
> >+Similar plugins include L<nbdkit-file-plugin(1)> which can serve a
> >+predefined set of exports (clients cannot create more),
>
> Hmm - I wonder if it is worth a filter that runs a script any time
> an .open fails.  That script could send email to an administrator,

But this would duplicate existing infrastructure which can send alerts
on syslog messages.  However nbdkit (and indeed other servers) could
help by having better diagnostics.  I wonder if there are extra fields
we could be providing to journald.

> >+++ b/plugins/ondemand/ondemand.c
> 
> >+/* Because we rewind the exportsdir handle, we need a lock to protect
> >+ * list_exports from being called in parallel.
> >+ */
> >+static pthread_mutex_t exports_lock = PTHREAD_MUTEX_INITIALIZER;
> 
> An alternative is to diropen() each time .list_exports gets called.
> Either way should work, though.

diropen == opendir?

> I still have some pending patches to improve .list_exports (split
> out a .default_export function, add an is_tls parameter), so there
> may be some churn in this area (for that matter, I have not yet
> pushed my patches for .list_exports in the file plugin, because I
> was trying to minimize churn while working on pending patches).
> We'll just have to see how it goes; I don't mind rebasing.

Right - this is indeed one major reason I sent this for review - it's
a new plugin which integrally uses list_exports.

> >+
> >+static int
> >+ondemand_list_exports (int readonly, int default_only,
> >+                       struct nbdkit_exports *exports)
> >+{
> >+  ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&exports_lock);
> >+  struct dirent *d;
> >+
> >+  /* First entry should be the default export.  XXX Should we check if
> >+   * the "default" file was created?  I don't think we need to.
> >+   */
> >+  if (nbdkit_add_export (exports, "", NULL) == -1)
> >+    return -1;
> >+  if (default_only) return 0;
> >+
> >+  /* Read the rest of the exports. */
> >+  rewinddir (exportsdir);
> >+
> >+  /* XXX Output is not sorted.  Does it matter? */
> >+  while (errno = 0, (d = readdir (exportsdir)) != NULL) {
> >+    /* Skip all dot files.  "." anywhere in the export name is
> >+     * rejected by the plugin, so commands can use dot files to "hide"
> >+     * files in the export dir (eg. if needing to keep state).
> >+     */
> >+    if (d->d_name[0] == '.')
> >+      continue;
> 
> This skips dot files (leading dot); but not those containing '.' or
> ':' elsewhere.  Did you want to skip more files, so that you aren't
> advertising stuff that can't pass open?

Yes, that's a bug.  Also we should skip filenames containg ':' anywhere.

> >+/* Since clients that want multi-conn should all pass the same export
> >+ * name, this is safe.
> >+ */
> >+static int
> >+ondemand_can_multi_conn (void *handle)
> >+{
> >+  return 1;
> >+}
> 
> Hmm. Are you permitting multiple clients to the same export, or did
> you decide that was too likely to cause fs corruption, and locking
> out users on the same export?  The docs above said otherwise, making
> this look wrong.

So I think this is wrong because my locking implementation will
prevent the (single) client from connecting multiple times.  That's a
bug: we should allow the same client to connect multiple times, which
I believe is safe.  However that requires client UUID ...

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