Richard W.M. Jones
2020-Apr-08 09:19 UTC
[Libguestfs] [PATCH nbdkit v3] tmpdisk: Generalize the tmpdisk plugin.
v2 was here: https://www.redhat.com/archives/libguestfs/2020-April/msg00075.html In v3: - Add [VAR=VALUE ...] to manual. - Various minor improvements to the manual. - Work (at least, in theory - not tested) with block devices or symlinks. I didn't document this because it's hard to ensure these files or block devices would be cleaned up, so here be dragons. - Remove O_NOCTTY. - Store the vars in order (instead of building the list in reverse). - Add a new test of the command feature and various corner cases. Rich.
Richard W.M. Jones
2020-Apr-08 09:19 UTC
[Libguestfs] [PATCH nbdkit v3] tmpdisk: Pass any parameters as shell variables to the command.
This allows us to be much more flexible about what commands can be used. It also means we do not need to encode any special behaviour for type or label parameters. --- plugins/tmpdisk/nbdkit-tmpdisk-plugin.pod | 115 ++++++++++---- tests/Makefile.am | 2 + plugins/tmpdisk/tmpdisk.c | 184 ++++++++++++++++------ plugins/tmpdisk/default-command.sh.in | 6 + tests/test-tmpdisk-command.sh | 52 ++++++ 5 files changed, 278 insertions(+), 81 deletions(-) diff --git a/plugins/tmpdisk/nbdkit-tmpdisk-plugin.pod b/plugins/tmpdisk/nbdkit-tmpdisk-plugin.pod index 490bcf6c..d6967378 100644 --- a/plugins/tmpdisk/nbdkit-tmpdisk-plugin.pod +++ b/plugins/tmpdisk/nbdkit-tmpdisk-plugin.pod @@ -4,9 +4,9 @@ nbdkit-tmpdisk-plugin - create a fresh temporary filesystem for each client =head1 SYNOPSIS - nbdkit tmpdisk [size=]SIZE - [type=ext4|xfs|vfat|...] [label=LABEL] - [command=COMMAND] + nbdkit tmpdisk [size=]SIZE [type=ext4|xfs|vfat|...] [label=LABEL] + + nbdkit tmpdisk [size=]SIZE command=COMMAND [VAR=VALUE ...] =head1 DESCRIPTION @@ -23,10 +23,39 @@ this plugin sees a different disk. The size of the disk is chosen using the C<size> parameter. The filesystem type is C<ext4> but this can be changed using the C<type> -parameter (controlling the I<-t> option of mkfs). +parameter (controlling the I<-t> option of mkfs). The filesystem +label may be set using C<label>. -Instead of running mkfs you can run an arbitrary C<command> to create -the disk. +=head2 The command parameter + +Instead of running mkfs you can run an arbitrary command (a shell +script fragment) to create the disk. + +The other parameters to the plugin are turned into shell variables +passed to the command. For example C<type> becomes the shell variable +C<$type>, etc. Any parameters you want can be passed to the plugin +and will be turned into shell variables (not only C<type> and +C<label>) making this a very flexible method to create temporary disks +of all kinds. + +Two special variables are also passed to the shell script fragment: + +=over 4 + +=item C<$disk> + +The absolute path of the disk file. Note that this is not +pre-created, you must create it yourself, for example using: + + truncate -s $size "$disk" + +=item C<$size> + +The virtual size in bytes. This is the C<size> parameter, converted +to bytes. Note the final size served to the client is whatever disk +size C<command> creates. + +=back =head2 Security considerations @@ -37,7 +66,7 @@ best to limit the number of clients using L<nbdkit-limit-filter(1)> or take steps to limit where clients can connect from using L<nbdkit-ip-filter(1)>, firewalls, or TLS client certificates. -=head1 EXAMPLE +=head1 EXAMPLES =head2 Remote tmpfs @@ -56,6 +85,43 @@ containing: Clients would see a fresh, empty C</var/scratch> directory after boot. +=head2 Overriding mkfs options + +Using C<command> allows you to easily override any mkfs option, for +example: + + nbdkit tmpdisk 16G command=' + truncate -s $size "$disk" + mke2fs -F -N 10000 -t ext4 "$disk" + ' + +=head2 Serve a fresh blank disk to each client + +Again using C<command>, this demonstrates serving any file that you +can create locally to the client. This is different from +L<nbdkit-memory-plugin(1)> because the clients all see their own +private disk (instead of all seeing the same shared disk): + + nbdkit tmpdisk 16G command=' truncate -s $size "$disk" ' + +=head2 Serve a fresh operating system to each client + + nbdkit tmpdisk 16G os=fedora-31 \ + command=' virt-builder -o "$disk" --size ${size}b "$os" ' + +=head2 Serve a throwaway snapshot of a base image to each client + +You could create a base image using L<virt-builder(1)> or a similar +tool, and then serve different throwaway snapshots to each client: + + virt-builder fedora-31 -o /var/tmp/base.img + nbdkit tmpdisk size=0 base=/var/tmp/base.img \ + command=' cp --sparse=always "$base" "$disk" ' + +The unusual C<size=0> parameter is because when using C<command>, +C<size> is only a request. The final size is taken from the C<$disk> +output by the L<cp(1)> command. + =head1 PARAMETERS =over 4 @@ -63,30 +129,8 @@ Clients would see a fresh, empty C</var/scratch> directory after boot. =item B<command='>COMMANDB<'> Instead of running L<mkfs(8)> to create the initial filesystem, run -C<COMMAND> (which usually must be quoted to protect it from the -shell). The following shell variables may be used in C<COMMAND>: - -=over 4 - -=item C<$disk> - -The absolute path of the file that the command must initialize. This -file is created by nbdkit before the command runs. - -=item C<$label> - -The filesystem label (from the C<label> parameter). - -=item C<$size> - -The virtual size in bytes, which is the same as the file size. - -=item C<$type> - -The filesystem type (from the C<type> parameter), defaulting to -C<ext4>. (Commands can ignore this if it is not relevant). - -=back +C<COMMAND> (a shell script fragment which usually must be quoted to +protect it from the shell). See L</The command parameter> above. =item B<label=>LABEL @@ -96,6 +140,10 @@ Select the filesystem label. The default is not set. Specify the virtual size of the disk image. +If using C<command>, this is only a suggested size. The actual size +of the resulting disk will be the size of the disk created by +C<command>. + This parameter is required. C<size=> is a magic config key and may be omitted in most cases. @@ -141,14 +189,17 @@ C<nbdkit-tmpdisk-plugin> first appeared in nbdkit 1.20. L<nbdkit(1)>, L<nbdkit-plugin(3)>, L<nbdkit-data-plugin(1)>, +L<nbdkit-eval-plugin(1)>, L<nbdkit-file-plugin(1)>, L<nbdkit-ip-filter(1)>, L<nbdkit-limit-filter(1)>, L<nbdkit-linuxdisk-plugin(1)>, L<nbdkit-memory-plugin(1)>, +L<nbdkit-sh-plugin(1)>, L<nbdkit-loop(1)>, L<nbdkit-tls(1)>, -L<mkfs(8)>. +L<mkfs(8)>, +L<virt-builder(1)>. =head1 AUTHORS diff --git a/tests/Makefile.am b/tests/Makefile.am index ecfc3612..2aa95890 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -206,6 +206,7 @@ EXTRA_DIST = \ test-tar.sh \ test-tls.sh \ test-tls-psk.sh \ + test-tmpdisk-command.sh \ test-truncate1.sh \ test-truncate2.sh \ test-truncate3.sh \ @@ -800,6 +801,7 @@ endif HAVE_PERL # tmpdisk plugin test. LIBGUESTFS_TESTS += test-tmpdisk +TESTS += test-tmpdisk-command.sh test_tmpdisk_SOURCES = \ test-tmpdisk.c \ diff --git a/plugins/tmpdisk/tmpdisk.c b/plugins/tmpdisk/tmpdisk.c index 5e8df151..6d8efc08 100644 --- a/plugins/tmpdisk/tmpdisk.c +++ b/plugins/tmpdisk/tmpdisk.c @@ -41,7 +41,9 @@ #include <unistd.h> #include <fcntl.h> #include <errno.h> +#include <assert.h> #include <sys/types.h> +#include <sys/stat.h> #include <sys/wait.h> #define NBDKIT_API_VERSION 2 @@ -51,9 +53,13 @@ #include "utils.h" static const char *tmpdir = "/var/tmp"; -static int64_t size = -1; -static const char *label = NULL; -static const char *type = "ext4"; +static int64_t requested_size = -1; /* size parameter on the command line */ + +/* Shell variables. */ +static struct var { + struct var *next; + const char *key, *value; +} *vars, *last_var; /* This comes from default-command.c which is generated from * default-command.sh.in. @@ -70,29 +76,61 @@ tmpdisk_load (void) tmpdir = s; } +static void +tmpdisk_unload (void) +{ + struct var *v, *v_next; + + for (v = vars; v != NULL; v = v_next) { + v_next = v->next; + free (v); + } +} + static int tmpdisk_config (const char *key, const char *value) { if (strcmp (key, "command") == 0) { command = value; } - else if (strcmp (key, "label") == 0) { - if (strcmp (value, "") == 0) - label = NULL; - else - label = value; - } else if (strcmp (key, "size") == 0) { - size = nbdkit_parse_size (value); - if (size == -1) + requested_size = nbdkit_parse_size (value); + if (requested_size == -1) return -1; } - else if (strcmp (key, "type") == 0) { - type = value; + + /* This parameter cannot be set on the command line since it is used + * to pass the disk name to the command. + */ + else if (strcmp (key, "disk") == 0) { + nbdkit_error ("'disk' parameter cannot be set on the command line"); + return -1; } + + /* Any other parameter will be forwarded to a shell variable. */ else { - nbdkit_error ("unknown parameter '%s'", key); - return -1; + struct var *new_var; + + new_var = malloc (sizeof *new_var); + if (new_var == NULL) { + perror ("malloc"); + exit (EXIT_FAILURE); + } + + new_var->next = NULL; + new_var->key = key; + new_var->value = value; + + /* Append it to the linked list. */ + if (vars == NULL) { + assert (last_var == NULL); + vars = last_var = new_var; + } + else { + assert (last_var != NULL); + last_var->next = new_var; + last_var = new_var; + } } return 0; @@ -101,7 +139,7 @@ tmpdisk_config (const char *key, const char *value) static int tmpdisk_config_complete (void) { - if (size == -1) { + if (requested_size == -1) { nbdkit_error ("size parameter is required"); return -1; } @@ -117,6 +155,7 @@ tmpdisk_config_complete (void) struct handle { int fd; + int64_t size; bool can_punch_hole; }; @@ -152,7 +191,9 @@ tmpdisk_can_fua (void *handle) static int64_t tmpdisk_get_size (void *handle) { - return size; + struct handle *h = handle; + + return h->size; } /* This creates and runs the full "mkfs" (or whatever) command. */ @@ -163,6 +204,7 @@ run_command (const char *disk) CLEANUP_FREE char *cmd = NULL; size_t len = 0; int r; + struct var *v; fp = open_memstream (&cmd, &len); if (fp == NULL) { @@ -173,21 +215,26 @@ run_command (const char *disk) /* Avoid stdin/stdout leaking (because of nbdkit -s). */ fprintf (fp, "exec </dev/null >/dev/null\n"); - /* Set the shell variables. */ + /* Set the standard shell variables. */ fprintf (fp, "disk="); shell_quote (disk, fp); putc ('\n', fp); - if (label) { - fprintf (fp, "label="); - shell_quote (label, fp); + fprintf (fp, "size=%" PRIi64 "\n", requested_size); + putc ('\n', fp); + + /* The other parameters/shell variables. */ + for (v = vars; v != NULL; v = v->next) { + /* Keys probably can never contain shell-unsafe chars (because of + * nbdkit's own restrictions), but quoting it makes it safe. + */ + shell_quote (v->key, fp); + putc ('=', fp); + shell_quote (v->value, fp); putc ('\n', fp); } - fprintf (fp, "size=%" PRIi64 "\n", size); - fprintf (fp, "type="); - shell_quote (type, fp); putc ('\n', fp); - putc ('\n', fp); + /* The command. */ fprintf (fp, "%s", command); if (fclose (fp) == EOF) { @@ -216,11 +263,28 @@ run_command (const char *disk) return 0; } +/* For block devices, stat->st_size is not the true size. */ +static int64_t +block_device_size (int fd) +{ + off_t size; + + size = lseek (fd, 0, SEEK_END); + if (size == -1) { + nbdkit_error ("lseek: %m"); + return -1; + } + + return size; +} + static void * tmpdisk_open (int readonly) { struct handle *h; - CLEANUP_FREE char *disk = NULL; + CLEANUP_FREE char *dir = NULL, *disk = NULL; + int flags; + struct stat statbuf; h = malloc (sizeof *h); if (h == NULL) { @@ -228,36 +292,25 @@ tmpdisk_open (int readonly) goto error; } h->fd = -1; + h->size = -1; h->can_punch_hole = true; - /* Create the new disk image for this connection. */ - if (asprintf (&disk, "%s/tmpdiskXXXXXX", tmpdir) == -1) { + /* For security reasons we have to create a temporary directory + * under tmpdir that only the current user can access. If we + * created it in a shared directory then another user might be able + * to see the temporary file being created and interfere with it + * before we reopen it in the plugin below. + */ + if (asprintf (&dir, "%s/tmpdiskXXXXXX", tmpdir) == -1) { nbdkit_error ("asprintf: %m"); goto error; } - -#ifdef HAVE_MKOSTEMP - h->fd = mkostemp (disk, O_CLOEXEC); -#else - /* Racy, fix your libc. */ - h->fd = mkstemp (disk); - if (h->fd >= 0) { - h->fd = set_cloexec (h->fd); - if (h->fd == -1) { - int e = errno; - unlink (disk); - errno = e; - } - } -#endif - if (h->fd == -1) { - nbdkit_error ("mkstemp: %m"); + if (mkdtemp (dir) == NULL) { + nbdkit_error ("%s: %m", dir); goto error; } - - /* Truncate the disk to a sparse file of the right size. */ - if (ftruncate (h->fd, size) == -1) { - nbdkit_error ("ftruncate: %s: %m", disk); + if (asprintf (&disk, "%s/disk", dir) == -1) { + nbdkit_error ("asprintf: %m"); goto error; } @@ -265,11 +318,43 @@ tmpdisk_open (int readonly) if (run_command (disk) == -1) goto error; + /* The external command must have created the disk, and then we must + * find the true size. + */ + if (readonly) + flags = O_RDONLY | O_CLOEXEC; + else + flags = O_RDWR | O_CLOEXEC; + h->fd = open (disk, flags); + if (h->fd == -1) { + nbdkit_error ("open: %s: %m", disk); + goto error; + } + + if (fstat (h->fd, &statbuf) == -1) { + nbdkit_error ("fstat: %s: %m", disk); + goto error; + } + + /* The command could set $disk to a regular file or a block device + * (or a symlink to either), so we must check that here. + */ + if (S_ISBLK (statbuf.st_mode)) { + h->size = block_device_size (h->fd); + if (h->size == -1) + goto error; + } + else /* Regular file. */ + h->size = statbuf.st_size; + nbdkit_debug ("tmpdisk: requested_size = %" PRIi64 ", size = %" PRIi64, + requested_size, h->size); + /* We don't need the disk to appear in the filesystem since we hold * a file descriptor and access it through that, so unlink the disk. * This also ensures it is always cleaned up. */ unlink (disk); + rmdir (dir); /* Return the handle. */ return h; @@ -414,6 +499,7 @@ static struct nbdkit_plugin plugin = { .version = PACKAGE_VERSION, .load = tmpdisk_load, + .unload = tmpdisk_unload, .config = tmpdisk_config, .config_complete = tmpdisk_config_complete, .config_help = tmpdisk_config_help, diff --git a/plugins/tmpdisk/default-command.sh.in b/plugins/tmpdisk/default-command.sh.in index 251e0b7b..d60402b5 100644 --- a/plugins/tmpdisk/default-command.sh.in +++ b/plugins/tmpdisk/default-command.sh.in @@ -30,6 +30,9 @@ # OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF # SUCH DAMAGE. +# If not set, default to ext4. +type="${type:-ext4}" + labelopt='-L' case "$type" in @@ -44,6 +47,9 @@ case "$type" in extra='-f' ;; esac +# Create the output disk. +truncate -s $size "$disk" + if [ "x$label" = "x" ]; then mkfs -t "$type" $extra "$disk" else diff --git a/tests/test-tmpdisk-command.sh b/tests/test-tmpdisk-command.sh new file mode 100755 index 00000000..8a9dd2d0 --- /dev/null +++ b/tests/test-tmpdisk-command.sh @@ -0,0 +1,52 @@ +#!/usr/bin/env bash +# nbdkit +# Copyright (C) 2020 Red Hat Inc. +# +# Redistribution and use in source and binary forms, with or without +# modification, are permitted provided that the following conditions are +# met: +# +# * Redistributions of source code must retain the above copyright +# notice, this list of conditions and the following disclaimer. +# +# * Redistributions in binary form must reproduce the above copyright +# notice, this list of conditions and the following disclaimer in the +# documentation and/or other materials provided with the distribution. +# +# * Neither the name of Red Hat nor the names of its contributors may be +# used to endorse or promote products derived from this software without +# specific prior written permission. +# +# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS IS'' AND +# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, +# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A +# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR +# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF +# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND +# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, +# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT +# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF +# SUCH DAMAGE. + +source ./functions.sh +set -e +set -x + +requires nbdsh -c 'exit (not h.supports_uri ())' + +# - If multiple parameters appear, last one is used. +# - Test quoting. +# - size=0 because we ignore it in the command itself. +nbdkit -f -v -U - tmpdisk 0 a=2 a=1 b=1024 c="a ' b ' c" \ + command=' +set -x +set -e +if [ $a -ne 1 ]; then exit 1; fi +if [ "$c" != "a '\'' b '\'' c" ]; then exit 1; fi +truncate -s $b "$disk" +' \ + --run ' +nbdsh -u "$uri" -c "assert h.get_size() == 1024" +' -- 2.25.0
Eric Blake
2020-Apr-08 13:47 UTC
Re: [Libguestfs] [PATCH nbdkit v3] tmpdisk: Pass any parameters as shell variables to the command.
On 4/8/20 4:19 AM, Richard W.M. Jones wrote:> This allows us to be much more flexible about what commands can be > used. It also means we do not need to encode any special behaviour > for type or label parameters. > --- > plugins/tmpdisk/nbdkit-tmpdisk-plugin.pod | 115 ++++++++++---- > tests/Makefile.am | 2 + > plugins/tmpdisk/tmpdisk.c | 184 ++++++++++++++++------ > plugins/tmpdisk/default-command.sh.in | 6 + > tests/test-tmpdisk-command.sh | 52 ++++++ > 5 files changed, 278 insertions(+), 81 deletions(-) >> +=head2 Serve a fresh operating system to each client > + > + nbdkit tmpdisk 16G os=fedora-31 \ > + command=' virt-builder -o "$disk" --size ${size}b "$os" ' > + > +=head2 Serve a throwaway snapshot of a base image to each client > + > +You could create a base image using L<virt-builder(1)> or a similar > +tool, and then serve different throwaway snapshots to each client: > + > + virt-builder fedora-31 -o /var/tmp/base.img > + nbdkit tmpdisk size=0 base=/var/tmp/base.img \ > + command=' cp --sparse=always "$base" "$disk" ' > +Nice contrast on how to cache the startup costs.> > +/* For block devices, stat->st_size is not the true size. */ > +static int64_t > +block_device_size (int fd) > +{ > + off_t size; > + > + size = lseek (fd, 0, SEEK_END); > + if (size == -1) { > + nbdkit_error ("lseek: %m"); > + return -1; > + } > + > + return size; > +}As you've mentioned elsewhere, we may want to add something in common/ that manages common tasks for disk images (size of a block device, handling .zero, ...); but for now the duplication is fine.> + /* The command could set $disk to a regular file or a block device > + * (or a symlink to either), so we must check that here. > + */ > + if (S_ISBLK (statbuf.st_mode)) { > + h->size = block_device_size (h->fd); > + if (h->size == -1) > + goto error; > + } > + else /* Regular file. */ > + h->size = statbuf.st_size; > + nbdkit_debug ("tmpdisk: requested_size = %" PRIi64 ", size = %" PRIi64, > + requested_size, h->size); > + > /* We don't need the disk to appear in the filesystem since we hold > * a file descriptor and access it through that, so unlink the disk. > * This also ensures it is always cleaned up. > */ > unlink (disk); > + rmdir (dir);If the user creates a symlink, we don't remove the original - but that's not our problem. I think this works just fine as written.> +++ b/tests/test-tmpdisk-command.sh> +source ./functions.sh > +set -e > +set -x > + > +requires nbdsh -c 'exit (not h.supports_uri ())' > + > +# - If multiple parameters appear, last one is used. > +# - Test quoting. > +# - size=0 because we ignore it in the command itself. > +nbdkit -f -v -U - tmpdisk 0 a=2 a=1 b=1024 c="a ' b ' c" \ > + command=' > +set -x > +set -e > +if [ $a -ne 1 ]; then exit 1; fi > +if [ "$c" != "a '\'' b '\'' c" ]; then exit 1; fi > +truncate -s $b "$disk" > +' \ > + --run ' > +nbdsh -u "$uri" -c "assert h.get_size() == 1024" > +'Nice. LGTM; the patch is now ready to go. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Apparently Analagous Threads
- [PATCH nbdkit v2] tmpdisk: Generalize the tmpdisk plugin
- [PATCH nbdkit 0/2] Generalize the tmpdisk plugin.
- [PATCH nbdkit] New tmpdisk plugin.
- [PATCH nbdkit v3] New tmpdisk plugin.
- [PATCH nbdkit v3] tmpdisk: Pass any parameters as shell variables to the command.