Libguestfs - Aug 2020 - Re: [PATCH nbdkit] New ondemand plugin.

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

On 8/15/20 4:13 PM, Richard W.M. Jones wrote:
>>> +/* 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?
Oops, yeah.
> 
>> 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.
> 
Okay, once my libnbd stuff settles, I'll be able to get back to 
finalizing how the .list_exports stuff should work in nbdkit.
>>> +/* 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 ...
> 
Hence your protocol enhancement proposal.  Yeah, letting a client tell 
the server during handshaking about both UUID and intent to be read-only 
vs. read-write are now seeming more worthwhile.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Question: Should the new directory= parameter of the file
plugin be "dir="?  It's shorter.  It's also consistent with
the
plugins floppy, iso, linuxdisk, and of course ondemand ...

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