[Libguestfs] [PATCH nbdkit] New ondemand plugin.

[Eric Blake]

On 8/14/20 12:20 PM, Richard W.M. Jones wrote:
> This creates filesystems on demand.  A client simply connects with a
> desired export name and a new export is created.  The export is
> persistent (until deleted by the server admin), and clients may
> disconnect and reconnect.  In some respects this is similar to the
> nbdkit-tmpdisk-plugin, or nbdkit-file-plugin with the dir= option.
> ---

> +
> +This is a plugin for L<nbdkit(1)> which creates persistent filesystems
> +on demand.  Clients may simply connect to the server, requesting a
> +particular export name, and a new filesystem is created if it does not
> +exist already.  Clients can also disconnect and reconnect with the
> +same export name and the same filesystem will still be available.
> +Filesystems are stored in a directory on the server, so they also
> +persist over nbdkit and server restarts.
> +
> +Each filesystem is locked while it is in use by a client, preventing
> +two clients from accessing the same filesystem (which would cause
> +corruption).

A rather crucial difference from the file plugin dir= mode ;)

> +
> +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, or even 
create a filesystem on the user's behalf; then run that filter in front 
of the file plugin to let the user create images after all.  The 
exclusion between clients is an interesting trick to figure out, though 
(the existing --filter=noparallel is too strict - you want parallel 
clients visiting different files, but not parallel clients visiting the 
same file).

> +=head2 Security considerations
> +
> +You should B<only> use this in an environment where you trust all your
> +clients, since clients can use this plugin to consume arbitrary
> +amounts of disk space by creating unlimited exports.  It is therefore
> +best to take steps to limit where clients can connect from using
> +L<nbdkit-ip-filter(1)>, firewalls, or TLS client certificates.

Yep, a definite need for this warning.

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

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.

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

> +
> +    /* Skip the "default" filename which refers to the "" export. */
> +    if (strcmp (d->d_name, "default") == 0)
> +      continue;
> +
> +    if (nbdkit_add_export (exports, d->d_name, NULL) == -1)
> +      return -1;
> +  }
> +
> +  /* Did readdir fail? */
> +  if (errno != 0) {
> +    nbdkit_error ("readdir: %s: %m", dir);
> +    return -1;
> +  }
> +
> +  return 0;
> +}
> +
> +struct handle {
> +  int fd;
> +  int64_t size;
> +  const char *exportname;
> +  bool can_punch_hole;
> +};
> +
> +/* 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.

> +
> +static int
> +ondemand_can_trim (void *handle)
> +{
> +#ifdef FALLOC_FL_PUNCH_HOLE
> +  return 1;
> +#else
> +  return 0;
> +#endif
> +}

The more plugins we add that touch the file system, the more it seems 
like it is worth our while to factor out helper libraries.


> +
> +  /* Lock the file to prevent filesystem corruption.  It's safe for
> +   * all clients to be reading.  If a client wants to write it must
> +   * have exclusive access.

Ah, you aren't locking out parallel readers, but rather doing .pwrite 
locking.  Interesting.

> +   *
> +   * This uses a currently Linux-specific extension.  It requires
> +   * Linux >= 3.15 (released in 2014, later backported to RHEL 7).
> +   * There is no sensible way to do this in pure POSIX.
> +   */
> +#ifdef F_OFD_SETLK
> +  memset (&lock, 0, sizeof lock);
> +  if (readonly)
> +    lock.l_type = F_RDLCK;
> +  else
> +    lock.l_type = F_WRLCK;
> +  lock.l_whence = SEEK_SET;
> +  lock.l_start = 0;
> +  lock.l_len = 0;
> +  if (fcntl (h->fd, F_OFD_SETLK, &lock) == -1) {
> +    if (errno == EACCES || errno == EAGAIN) {
> +      nbdkit_error ("%s: filesystem is locked by another client",
> +                    h->exportname);
> +      /* XXX Would be nice if NBD protocol supported some kind of "is
> +       * locked" indication.  If it did we could use it here.

Yeah, I've thought about the idea of adding a way for clients to 
advertise their intent for read vs. read-write during handshaking.

> +++ b/plugins/ondemand/default-command.sh.in
> @@ -0,0 +1,57 @@
> +# nbdkit
> +# -*- mode: shell-script -*-
> +# Copyright (C) 2017-2020 Red Hat Inc.
> +#

No bash shebang line, but it looks portable to POSIX /bin/sh.

> +++ b/tests/test-ondemand.sh

> +++ b/TODO
> @@ -154,6 +154,13 @@ nbdkit-torrent-plugin:
>   * The C++ could be a lot more natural.  At the moment it's a kind of
>     “C with C++ extensions”.
>   
> +nbdkit-ondemand-plugin:
> +
> +* Implement more callbacks, eg. .zero
> +
> +* Allow client to select size up to a limit, eg. by sending export
> +  names like ‘export:4G’.

I like the idea for using exportnames to pass in additional information.

Overall looks pretty reasonable.

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