Libguestfs - Dec 2018 - Re: [PATCH nbdkit 1/3] sh: Implement inline scripts.

On 12/14/18 4:16 PM, Richard W.M. Jones wrote:
> This implements something like a readonly 1MB disk reading as zeroes:
> 
> nbdkit sh script=- <<'EOF'
>    case "$1" in
>      get_size) echo 1M ;;
>      pread) dd if=/dev/zero count=$3 iflag=count_bytes ;;
>      *) exit 2 ;;
>    esac
> EOF
> 
> Use of "-" is analogous to reading passwords from stdin.
> ---
> @@ -26,6 +30,19 @@ like this:
>   You may have to add further C<key=value> arguments to the command
>   line.
>   
> +=head2 Inline shell scripts
> +
> +It is also possible to write a shell script plugin "inline"
using C<->
> +as the name of the script, like this:
> +
> + nbdkit sh script=- <<'EOF'
> +   case "$1" in
> +     get_size) echo 1M ;;
> +     pread) dd if=/dev/zero count=$3 iflag=count_bytes ;;
> +     *) exit 2 ;;
> +   esac
> + EOF
> +
Worth a specific mention that 'script=' is required, since otherwise the
'-' looks like an option to nbdkit proper rather than a script
parameter?

Probably not worth a mention that a local file named '-' can still be 
invoked via [script=]./- (there, script is optional because the leading 
./ avoids the leading -).
> +static char *
> +inline_script (void)
> +{
> +  const char scriptname[] = "inline-script.sh";
> +  char *filename = NULL;
> +  char *cmd = NULL;
> +
> +  if (asprintf (&filename, "%s/%s", tmpdir, scriptname) ==
-1) {
> +    nbdkit_error ("asprintf: %m");
> +    goto err;
> +  }
> +
> +  /* Safe because both the tmpdir and script name are controlled by us
> +   * and don't contain characters that need quoting.
> +   */
> +  if (asprintf (&cmd, "cat > %s", filename) == -1) {
Useful comment :) And I verified it is true.

Looks good.

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

On Fri, Dec 14, 2018 at 04:31:32PM -0600, Eric Blake
wrote:
> On 12/14/18 4:16 PM, Richard W.M. Jones wrote:
> >This implements something like a readonly 1MB disk reading as zeroes:
> >
> >nbdkit sh script=- <<'EOF'
> >   case "$1" in
> >     get_size) echo 1M ;;
> >     pread) dd if=/dev/zero count=$3 iflag=count_bytes ;;
> >     *) exit 2 ;;
> >   esac
> >EOF
> >
> >Use of "-" is analogous to reading passwords from stdin.
> >---
> 
> >@@ -26,6 +30,19 @@ like this:
> >  You may have to add further C<key=value> arguments to the
command
> >  line.
> >+=head2 Inline shell scripts
> >+
> >+It is also possible to write a shell script plugin "inline"
using C<->
> >+as the name of the script, like this:
> >+
> >+ nbdkit sh script=- <<'EOF'
> >+   case "$1" in
> >+     get_size) echo 1M ;;
> >+     pread) dd if=/dev/zero count=$3 iflag=count_bytes ;;
> >+     *) exit 2 ;;
> >+   esac
> >+ EOF
> >+
> 
> Worth a specific mention that 'script=' is required, since
otherwise
> the '-' looks like an option to nbdkit proper rather than a script
> parameter?
Actually I tried using just "-" (for the first time, I didn't try
it
before) and it works.  So the above can be written:

 nbdkit sh - <<'EOF'
   case "$1" in
     get_size) echo 1M ;;
     pread) dd if=/dev/zero count=$3 iflag=count_bytes ;;
     *) exit 2 ;;
   esac
 EOF

At least on Linux.  I wonder if that also works on FreeBSD etc and if
it depends on an implementation detail of the parser.  I'll have to do
some more investigations.

Rich.

> Probably not worth a mention that a local file named '-' can still
> be invoked via [script=]./- (there, script is optional because the
> leading ./ avoids the leading -).
> 
> >+static char *
> >+inline_script (void)
> >+{
> >+  const char scriptname[] = "inline-script.sh";
> >+  char *filename = NULL;
> >+  char *cmd = NULL;
> >+
> >+  if (asprintf (&filename, "%s/%s", tmpdir, scriptname)
== -1) {
> >+    nbdkit_error ("asprintf: %m");
> >+    goto err;
> >+  }
> >+
> >+  /* Safe because both the tmpdir and script name are controlled by us
> >+   * and don't contain characters that need quoting.
> >+   */
> >+  if (asprintf (&cmd, "cat > %s", filename) == -1) {
> 
> Useful comment :) And I verified it is true.
> 
> Looks good.
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org
-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

On Sat, Dec 15, 2018 at 07:22:41AM +0000, Richard W.M. Jones
wrote:
> On Fri, Dec 14, 2018 at 04:31:32PM -0600, Eric Blake wrote:
> > On 12/14/18 4:16 PM, Richard W.M. Jones wrote:
> > >This implements something like a readonly 1MB disk reading as
zeroes:
> > >
> > >nbdkit sh script=- <<'EOF'
> > >   case "$1" in
> > >     get_size) echo 1M ;;
> > >     pread) dd if=/dev/zero count=$3 iflag=count_bytes ;;
> > >     *) exit 2 ;;
> > >   esac
> > >EOF
> > >
> > >Use of "-" is analogous to reading passwords from stdin.
> > >---
> > 
> > >@@ -26,6 +30,19 @@ like this:
> > >  You may have to add further C<key=value> arguments to the
command
> > >  line.
> > >+=head2 Inline shell scripts
> > >+
> > >+It is also possible to write a shell script plugin
"inline" using C<->
> > >+as the name of the script, like this:
> > >+
> > >+ nbdkit sh script=- <<'EOF'
> > >+   case "$1" in
> > >+     get_size) echo 1M ;;
> > >+     pread) dd if=/dev/zero count=$3 iflag=count_bytes ;;
> > >+     *) exit 2 ;;
> > >+   esac
> > >+ EOF
> > >+
> > 
> > Worth a specific mention that 'script=' is required, since
otherwise
> > the '-' looks like an option to nbdkit proper rather than a
script
> > parameter?
> 
> Actually I tried using just "-" (for the first time, I didn't
try it
> before) and it works.  So the above can be written:
> 
>  nbdkit sh - <<'EOF'
>    case "$1" in
>      get_size) echo 1M ;;
>      pread) dd if=/dev/zero count=$3 iflag=count_bytes ;;
>      *) exit 2 ;;
>    esac
>  EOF
> 
> At least on Linux.  I wonder if that also works on FreeBSD etc and if
> it depends on an implementation detail of the parser.  I'll have to do
> some more investigations.
This works on FreeBSD, and looking at the way that coreutils cat(1) is
implemented (as an example of presumably portable code) it also simply
assumes that "-" is parsed as a non-option.

I'm going to rework this, along with the other review comments you
made.

Thanks,

Rich.
> Rich.
> 
> 
> > Probably not worth a mention that a local file named '-' can
still
> > be invoked via [script=]./- (there, script is optional because the
> > leading ./ avoids the leading -).
> > 
> > >+static char *
> > >+inline_script (void)
> > >+{
> > >+  const char scriptname[] = "inline-script.sh";
> > >+  char *filename = NULL;
> > >+  char *cmd = NULL;
> > >+
> > >+  if (asprintf (&filename, "%s/%s", tmpdir,
scriptname) == -1) {
> > >+    nbdkit_error ("asprintf: %m");
> > >+    goto err;
> > >+  }
> > >+
> > >+  /* Safe because both the tmpdir and script name are controlled
by us
> > >+   * and don't contain characters that need quoting.
> > >+   */
> > >+  if (asprintf (&cmd, "cat > %s", filename) ==
-1) {
> > 
> > Useful comment :) And I verified it is true.
> > 
> > Looks good.
> > 
> > -- 
> > Eric Blake, Principal Software Engineer
> > Red Hat, Inc.           +1-919-301-3266
> > Virtualization:  qemu.org | libvirt.org
> 
> -- 
> Richard Jones, Virtualization Group, Red Hat
http://people.redhat.com/~rjones
> Read my programming and virtualization blog: http://rwmj.wordpress.com
> virt-top is 'top' for virtual machines.  Tiny program with many
> powerful monitoring features, net stats, disk stats, logging, etc.
> http://people.redhat.com/~rjones/virt-top
-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/