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

Some feature additions to the shell script plugin allow us to test the
export flags field reasonably easily.

Rich.

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.
---
 plugins/sh/nbdkit-sh-plugin.pod | 17 ++++++++++
 plugins/sh/sh.c                 | 55 ++++++++++++++++++++++++++++++++-
 2 files changed, 71 insertions(+), 1 deletion(-)

diff --git a/plugins/sh/nbdkit-sh-plugin.pod b/plugins/sh/nbdkit-sh-plugin.pod
index 371de5f..0644c36 100644
--- a/plugins/sh/nbdkit-sh-plugin.pod
+++ b/plugins/sh/nbdkit-sh-plugin.pod
@@ -6,6 +6,10 @@ nbdkit-sh-plugin - nbdkit shell, script or executable plugin
 
  nbdkit sh /path/to/script [arguments...]
 
+ nbdkit sh script=- [arguments...] <<'EOF'
+ ... shell script here ...
+ EOF
+
 =head1 DESCRIPTION
 
 C<nbdkit-sh-plugin> allows you to write plugins for L<nbdkit(1)>
using
@@ -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
+
 =head1 WRITING AN NBDKIT SH PLUGIN
 
 For an example plugin written in Bash, see:
diff --git a/plugins/sh/sh.c b/plugins/sh/sh.c
index 368f1eb..fcd839b 100644
--- a/plugins/sh/sh.c
+++ b/plugins/sh/sh.c
@@ -39,6 +39,7 @@
 #include <string.h>
 #include <unistd.h>
 #include <errno.h>
+#include <sys/stat.h>
 
 #include <nbdkit-plugin.h>
 
@@ -115,6 +116,50 @@ sh_dump_plugin (void)
   }
 }
 
+/* This implements the "inline script" feature (script=-).  Read
stdin
+ * into a temporary file and return the name of the file which the
+ * caller must free.  For convenience we put the temporary file into
+ * tmpdir but that's an implementation detail.
+ */
+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) {
+    nbdkit_error ("asprintf: %m");
+    goto err;
+  }
+
+  if (system (cmd) != 0) {
+    nbdkit_error ("sh: failed to copy inline script to temporary
file");
+    goto err;
+  }
+
+  if (chmod (filename, 0500) == -1) {
+    nbdkit_error ("chmod: %s: %m", filename);
+    goto err;
+  }
+
+  free (cmd);
+  return filename;
+
+ err:
+  free (filename);
+  free (cmd);
+  return NULL;
+}
+
 static int
 sh_config (const char *key, const char *value)
 {
@@ -124,7 +169,15 @@ sh_config (const char *key, const char *value)
       nbdkit_error ("the first parameter must be
script=/path/to/script");
       return -1;
     }
-    script = nbdkit_realpath (value);
+
+    /* If the script name is not "-" then it's expected to be a
+     * filename, otherwise it's an inline script which must be read
+     * into a temporary file.  Either way we want an absolute path.
+     */
+    if (strcmp (value, "-") != 0)
+      script = nbdkit_realpath (value);
+    else
+      script = inline_script ();
     if (script == NULL)
       return -1;
 
-- 
2.19.2

This allows us to add the following callbacks:

 - can_zero     returns boolean
 - can_fua      should print "none", "emulate" or
"native"

Furthermore the following callbacks are modified in a backwards
compatible manner:

 - pwrite       adding flags parameter
 - trim         adding flags parameter

This change is not backwards compatible:

 - zero         may_trim parameter replaced by flags parameter
---
 plugins/sh/nbdkit-sh-plugin.pod | 41 +++++++++++---
 plugins/sh/sh.c                 | 99 +++++++++++++++++++++++++++------
 2 files changed, 115 insertions(+), 25 deletions(-)

diff --git a/plugins/sh/nbdkit-sh-plugin.pod b/plugins/sh/nbdkit-sh-plugin.pod
index 0644c36..ebd3cfb 100644
--- a/plugins/sh/nbdkit-sh-plugin.pod
+++ b/plugins/sh/nbdkit-sh-plugin.pod
@@ -196,17 +196,21 @@ This method is required.
 
 =item C<can_trim>
 
+=item C<can_zero>
+
 Unlike in other languages, you B<must> provide the C<can_*> methods
 otherwise they are assumed to all return false and your C<pwrite>,
-C<flush> and C<trim> methods will never be called.  The reason for
-this is obscure: In other languages we can detect if (eg) a C<pwrite>
-method is defined and synthesize an appropriate response if no actual
-C<can_write> method is defined.  However detecting if methods are
-present without running them is not possible with this plugin.
+C<flush>, C<trim> and C<zero> methods will never be called. 
The
+reason for this is obscure: In other languages we can detect if (eg) a
+C<pwrite> method is defined and synthesize an appropriate response if
+no actual C<can_write> method is defined.  However detecting if
+methods are present without running them is not possible with this
+plugin.
 
  /path/to/script can_write <handle>
  /path/to/script can_flush <handle>
  /path/to/script can_trim <handle>
+ /path/to/script can_zero <handle>
 
 The script should exit with code C<0> for true or code C<3> for
false.
 
@@ -216,6 +220,15 @@ The script should exit with code C<0> for true or
code C<3> for false.
 
 The script should exit with code C<0> for true or code C<3> for
false.
 
+=item C<can_fua>
+
+ /path/to/script can_fua <handle>
+
+This controls Forced Unit Access (FUA) behaviour of the core server.
+Unlike the other C<can_*> callbacks, this one is I<not> a boolean. 
It
+must print either "none", "emulate" or "native". 
The meaning of these
+is described in L<nbdkit-plugin(3)>.
+
 =item C<pread>
 
  /path/to/script pread <handle> <count> <offset>
@@ -227,10 +240,13 @@ This method is required.
 
 =item C<pwrite>
 
- /path/to/script pwrite <handle> <count> <offset>
+ /path/to/script pwrite <handle> <count> <offset>
<flags>
 
 The script should read the binary data to be written from stdin.
 
+The C<flags> parameter can be an empty string or
C<"fua">.  In future
+a comma-separated list of flags may be present.
+
 Unlike in other languages, if you provide a C<pwrite> method you
 B<must> also provide a C<can_write> method which exits with code
C<0>
 (true).
@@ -245,16 +261,23 @@ B<must> also provide a C<can_flush> method
which exits with code C<0>
 
 =item C<trim>
 
- /path/to/script trim <handle> <count> <offset>
+ /path/to/script trim <handle> <count> <offset> <flags>
+
+The C<flags> parameter can be an empty string or
C<"fua">.  In future
+a comma-separated list of flags may be present.
 
 Unlike in other languages, if you provide a C<trim> method you
B<must>
 also provide a C<can_trim> method which exits with code C<0>
(true).
 
 =item C<zero>
 
- /path/to/script zero <handle> <count> <offset>
<may_trim>
+ /path/to/script zero <handle> <count> <offset> <flags>
 
-C<may_trim> will be either C<true> or C<false>.
+The C<flags> parameter can be an empty string or
C<"fua">.  In future
+a comma-separated list of flags may be present.
+
+Unlike in other languages, if you provide a C<zero> method you
B<must>
+also provide a C<can_zero> method which exits with code C<0>
(true).
 
 =back
 
diff --git a/plugins/sh/sh.c b/plugins/sh/sh.c
index fcd839b..58693fb 100644
--- a/plugins/sh/sh.c
+++ b/plugins/sh/sh.c
@@ -41,6 +41,8 @@
 #include <errno.h>
 #include <sys/stat.h>
 
+#define NBDKIT_API_VERSION 2
+
 #include <nbdkit-plugin.h>
 
 #include "call.h"
@@ -360,7 +362,8 @@ sh_get_size (void *handle)
 }
 
 static int
-sh_pread (void *handle, void *buf, uint32_t count, uint64_t offset)
+sh_pread (void *handle, void *buf, uint32_t count, uint64_t offset,
+          uint32_t flags)
 {
   char *h = handle;
   char cbuf[32], obuf[32];
@@ -404,12 +407,14 @@ sh_pread (void *handle, void *buf, uint32_t count,
uint64_t offset)
 }
 
 static int
-sh_pwrite (void *handle, const void *buf,
-                   uint32_t count, uint64_t offset)
+sh_pwrite (void *handle, const void *buf, uint32_t count, uint64_t offset,
+           uint32_t flags)
 {
   char *h = handle;
   char cbuf[32], obuf[32];
-  const char *args[] = { script, "pwrite", h, cbuf, obuf, NULL };
+  /* In future, comma-separated list of flags. */
+  const char *sflags = flags & NBDKIT_FLAG_FUA ? "fua" :
"";
+  const char *args[] = { script, "pwrite", h, cbuf, obuf, sflags,
NULL };
 
   snprintf (cbuf, sizeof cbuf, "%" PRIu32, count);
   snprintf (obuf, sizeof obuf, "%" PRIu64, offset);
@@ -467,12 +472,6 @@ sh_can_flush (void *handle)
   return boolean_method (handle, "can_flush");
 }
 
-static int
-sh_can_trim (void *handle)
-{
-  return boolean_method (handle, "can_trim");
-}
-
 static int
 sh_is_rotational (void *handle)
 {
@@ -480,7 +479,70 @@ sh_is_rotational (void *handle)
 }
 
 static int
-sh_flush (void *handle)
+sh_can_trim (void *handle)
+{
+  return boolean_method (handle, "can_trim");
+}
+
+static int
+sh_can_zero (void *handle)
+{
+  return boolean_method (handle, "can_zero");
+}
+
+static int
+sh_can_fua (void *handle)
+{
+  char *h = handle;
+  const char *args[] = { script, "can_fua", h, NULL };
+  char *s = NULL;
+  size_t slen;
+  int r;
+
+  switch (call_read (&s, &slen, args)) {
+  case OK:
+    if (slen > 0 && s[slen-1] == '\n')
+      s[slen-1] = '\0';
+    if (strcmp (s, "none") == 0)
+      r = NBDKIT_FUA_NONE;
+    else if (strcmp (s, "emulate") == 0)
+      r = NBDKIT_FUA_EMULATE;
+    else if (strcmp (s, "native") == 0)
+      r = NBDKIT_FUA_NATIVE;
+    else {
+      nbdkit_error ("%s: could not parse output from can_fua method:
%s",
+                    script, s);
+      free (s);
+      return -1;
+    }
+    free (s);
+    return r;
+
+  case MISSING:
+    free (s);
+    /* NBDKIT_FUA_EMULATE means that nbdkit will call .flush.  However
+     * we cannot know if that callback exists, so the safest default
+     * is to return NBDKIT_FUA_NONE.
+     */
+    return NBDKIT_FUA_NONE;
+
+  case ERROR:
+    free (s);
+    return -1;
+
+  case RET_FALSE:
+    free (s);
+    nbdkit_error ("%s: %s method returned unexpected code (3/false)",
+                  script, "can_fua");
+    errno = EIO;
+    return -1;
+
+  default: abort ();
+  }
+}
+
+static int
+sh_flush (void *handle, uint32_t flags)
 {
   char *h = handle;
   const char *args[] = { script, "flush", h, NULL };
@@ -507,11 +569,13 @@ sh_flush (void *handle)
 }
 
 static int
-sh_trim (void *handle, uint32_t count, uint64_t offset)
+sh_trim (void *handle, uint32_t count, uint64_t offset, uint32_t flags)
 {
   char *h = handle;
   char cbuf[32], obuf[32];
-  const char *args[] = { script, "trim", h, cbuf, obuf, NULL };
+  /* In future, comma-separated list of flags. */
+  const char *sflags = flags & NBDKIT_FLAG_FUA ? "fua" :
"";
+  const char *args[] = { script, "trim", h, cbuf, obuf, sflags, NULL
};
 
   snprintf (cbuf, sizeof cbuf, "%" PRIu32, count);
   snprintf (obuf, sizeof obuf, "%" PRIu64, offset);
@@ -538,12 +602,13 @@ sh_trim (void *handle, uint32_t count, uint64_t offset)
 }
 
 static int
-sh_zero (void *handle, uint32_t count, uint64_t offset, int may_trim)
+sh_zero (void *handle, uint32_t count, uint64_t offset, uint32_t flags)
 {
   char *h = handle;
   char cbuf[32], obuf[32];
-  const char *args[] = { script, "zero", h, cbuf, obuf,
-                         may_trim ? "true" : "false", NULL
};
+  /* In future, comma-separated list of flags. */
+  const char *sflags = flags & NBDKIT_FLAG_FUA ? "fua" :
"";
+  const char *args[] = { script, "zero", h, cbuf, obuf, sflags, NULL
};
 
   snprintf (cbuf, sizeof cbuf, "%" PRIu32, count);
   snprintf (obuf, sizeof obuf, "%" PRIu64, offset);
@@ -596,6 +661,8 @@ static struct nbdkit_plugin plugin = {
   .can_flush         = sh_can_flush,
   .is_rotational     = sh_is_rotational,
   .can_trim          = sh_can_trim,
+  .can_zero          = sh_can_zero,
+  .can_fua           = sh_can_fua,
 
   .pread             = sh_pread,
   .pwrite            = sh_pwrite,
-- 
2.19.2

---
 tests/Makefile.am    |   3 +
 tests/test-eflags.sh | 222 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 225 insertions(+)

diff --git a/tests/Makefile.am b/tests/Makefile.am
index a01c47c..173d622 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -282,6 +282,9 @@ test_oldstyle_LDADD = libtest.la $(LIBGUESTFS_LIBS)
 
 endif HAVE_LIBGUESTFS
 
+# Test export flags.
+TESTS += test-eflags.sh
+
 # common disk image shared with several tests
 if HAVE_GUESTFISH
 check_DATA += disk
diff --git a/tests/test-eflags.sh b/tests/test-eflags.sh
new file mode 100755
index 0000000..96ec311
--- /dev/null
+++ b/tests/test-eflags.sh
@@ -0,0 +1,222 @@
+#!/usr/bin/env bash
+# nbdkit
+# Copyright (C) 2018 Red Hat Inc.
+# All rights reserved.
+#
+# 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.
+
+# Test export flags.
+#
+# Run nbdkit with various can_* callbacks defined and with or without
+# the -r flag, and check that nbdkit constructs the export flags
+# controlling READ_ONLY, ROTATIONAL, SEND_TRIM, etc. as expected.
+#
+# We use the oldstyle (-o) protocol here because it's simpler to read
+# out the eflags.  We use the shell plugin because it gives maximum
+# control over the can_* callbacks (at least, max without having to
+# write a C plugin).
+
+source ./functions.sh
+set -e
+
+if ! socat -h >/dev/null; then
+    echo "$0: 'socat' command not available"
+    exit 77
+fi
+
+# Check 'od' command exists and can do endian swaps (which the FreeBSD
+# version cannot).
+if ! od --endian=big </dev/null >/dev/null; then
+    echo "$0: 'od' command not available or doesn't support
endian swaps"
+    exit 77
+fi
+
+files="eflags.out"
+rm -f $files
+cleanup_fn rm -f $files
+
+# The export flags.
+# See also src/protocol.h
+HAS_FLAGS=$((         1 << 0 ))
+READ_ONLY=$((         1 << 1 ))
+SEND_FLUSH=$((        1 << 2 ))
+SEND_FUA=$((          1 << 3 ))
+ROTATIONAL=$((        1 << 4 ))
+SEND_TRIM=$((         1 << 5 ))
+SEND_WRITE_ZEROES=$(( 1 << 6 ))
+
+all_flags="HAS_FLAGS READ_ONLY SEND_FLUSH SEND_FUA ROTATIONAL SEND_TRIM
SEND_WRITE_ZEROES"
+
+do_nbdkit ()
+{
+    nbdkit -v -U - -o "$@" sh script=- --run '
+        socat -b 28 unix-connect:$unixsocket \
+            exec:"dd bs=1 skip=26 count=2 of=eflags.out status=none"
+    '
+
+    # By adding 0 we convert the string to an integer (removing
+    # whitespace).
+    eflags=$((0 + $(od --endian=big -An -N2 -d < eflags.out)))
+    printf "eflags 0x%04x" $eflags
+    for f in $all_flags; do
+        [ $(( eflags & ${!f} )) -ne 0 ] && echo -n " $f"
+    done
+    echo
+}
+
+fail ()
+{
+    echo "$@"
+    exit 1
+}
+
+# no -r
+# can_write=false
+
+do_nbdkit <<'EOF'
+case "$1" in
+     get_size) echo 1M ;;
+     *) exit 2 ;;
+esac
+EOF
+
+[ $eflags -eq $(( HAS_FLAGS|READ_ONLY )) ] ||
+    fail "expected HAS_FLAGS|READ_ONLY"
+
+# -r
+# can_write=false
+
+do_nbdkit -r <<'EOF'
+case "$1" in
+     get_size) echo 1M ;;
+     *) exit 2 ;;
+esac
+EOF
+
+[ $eflags -eq $(( HAS_FLAGS|READ_ONLY )) ] ||
+    fail "expected HAS_FLAGS|READ_ONLY"
+
+# no -r
+# can_write=true
+#
+# NBD_FLAG_SEND_WRITE_ZEROES is always set on writable connections
+# even if can_zero returns false, because nbdkit reckons it can
+# emulate zeroing using pwrite.
+
+do_nbdkit <<'EOF'
+case "$1" in
+     get_size) echo 1M ;;
+     can_write) exit 0 ;;
+     *) exit 2 ;;
+esac
+EOF
+
+[ $eflags -eq $(( HAS_FLAGS|SEND_WRITE_ZEROES )) ] ||
+    fail "expected HAS_FLAGS|SEND_WRITE_ZEROES"
+
+# -r
+# can_write=true
+#
+# The -r flag overrides the plugin so this behaves as if can_write is
+# false.
+
+do_nbdkit -r <<'EOF'
+case "$1" in
+     get_size) echo 1M ;;
+     can_write) exit 0 ;;
+     *) exit 2 ;;
+esac
+EOF
+
+[ $eflags -eq $(( HAS_FLAGS|READ_ONLY )) ] ||
+    fail "expected HAS_FLAGS|READ_ONLY"
+
+# can_write=false
+# can_trim=true
+# can_zero=true
+#
+# If writing is not possible then trim and zero are always disabled.
+
+do_nbdkit <<'EOF'
+case "$1" in
+     get_size) echo 1M ;;
+     can_write) exit 3 ;;
+     can_trim) exit 0 ;;
+     can_zero) exit 0 ;;
+     *) exit 2 ;;
+esac
+EOF
+
+[ $eflags -eq $(( HAS_FLAGS|READ_ONLY )) ] ||
+    fail "expected HAS_FLAGS|READ_ONLY"
+
+# can_write=true
+# can_trim=true
+
+do_nbdkit <<'EOF'
+case "$1" in
+     get_size) echo 1M ;;
+     can_write) exit 0 ;;
+     can_trim) exit 0 ;;
+     *) exit 2 ;;
+esac
+EOF
+
+[ $eflags -eq $(( HAS_FLAGS|SEND_TRIM|SEND_WRITE_ZEROES )) ] ||
+    fail "expected HAS_FLAGS|SEND_TRIM|SEND_WRITE_ZEROES"
+
+# can_write=true
+# is_rotational=true
+
+do_nbdkit <<'EOF'
+case "$1" in
+     get_size) echo 1M ;;
+     can_write) exit 0 ;;
+     is_rotational) exit 0 ;;
+     *) exit 2 ;;
+esac
+EOF
+
+[ $eflags -eq $(( HAS_FLAGS|ROTATIONAL|SEND_WRITE_ZEROES )) ] ||
+    fail "expected HAS_FLAGS|ROTATIONAL|SEND_WRITE_ZEROES"
+
+# can_write=true
+# can_fua=native
+
+do_nbdkit <<'EOF'
+case "$1" in
+     get_size) echo 1M ;;
+     can_write) exit 0 ;;
+     can_fua) echo "native" ;;
+     *) exit 2 ;;
+esac
+EOF
+
+[ $eflags -eq $(( HAS_FLAGS|SEND_FUA|SEND_WRITE_ZEROES )) ] ||
+    fail "expected HAS_FLAGS|SEND_FUA|SEND_WRITE_ZEROES"
-- 
2.19.2

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 12/14/18 4:16 PM, Richard W.M. Jones wrote:
> This allows us to add the following callbacks:
> 
>   - can_zero     returns boolean
>   - can_fua      should print "none", "emulate" or
"native"
> 
> Furthermore the following callbacks are modified in a backwards
> compatible manner:
> 
>   - pwrite       adding flags parameter
>   - trim         adding flags parameter
> 
> This change is not backwards compatible:
> 
>   - zero         may_trim parameter replaced by flags parameter
> ---
>   plugins/sh/nbdkit-sh-plugin.pod | 41 +++++++++++---
>   plugins/sh/sh.c                 | 99 +++++++++++++++++++++++++++------
>   2 files changed, 115 insertions(+), 25 deletions(-)
> 
>   
> +=item C<can_fua>
> +
> + /path/to/script can_fua <handle>
> +
> +This controls Forced Unit Access (FUA) behaviour of the core server.
> +Unlike the other C<can_*> callbacks, this one is I<not> a
boolean.  It
> +must print either "none", "emulate" or
"native".  The meaning of these
Worth adding 'to stdout'?
> +is described in L<nbdkit-plugin(3)>.
> +
>   =item C<pread>
>   
>    /path/to/script pread <handle> <count> <offset>
> @@ -227,10 +240,13 @@ This method is required.
>   
>   =item C<pwrite>
>   
> - /path/to/script pwrite <handle> <count> <offset>
> + /path/to/script pwrite <handle> <count> <offset>
<flags>
>   
>   The script should read the binary data to be written from stdin.
>   
> +The C<flags> parameter can be an empty string or
C<"fua">.  In future
s/In future/In the future,/
> +a comma-separated list of flags may be present.
Is the empty string "" always provided as an argument, or will the 
argument be omitted in that case?
> +
>   Unlike in other languages, if you provide a C<pwrite> method you
>   B<must> also provide a C<can_write> method which exits with
code C<0>
>   (true).
> @@ -245,16 +261,23 @@ B<must> also provide a C<can_flush>
method which exits with code C<0>
>   
>   =item C<trim>
>   
> - /path/to/script trim <handle> <count> <offset>
> + /path/to/script trim <handle> <count> <offset>
<flags>
> +
> +The C<flags> parameter can be an empty string or
C<"fua">.  In future
Copy-and-paste "In future"
> +a comma-separated list of flags may be present.
>   
>   Unlike in other languages, if you provide a C<trim> method you
B<must>
>   also provide a C<can_trim> method which exits with code C<0>
(true).
>   
>   =item C<zero>
>   
> - /path/to/script zero <handle> <count> <offset>
<may_trim>
> + /path/to/script zero <handle> <count> <offset>
<flags>
>   
> -C<may_trim> will be either C<true> or C<false>.
> +The C<flags> parameter can be an empty string or
C<"fua">.  In future
> +a comma-separated list of flags may be present.
This one is wrong, since you have both fua and may_trim now.
> +
> +  case RET_FALSE:
> +    free (s);
> +    nbdkit_error ("%s: %s method returned unexpected code
(3/false)",
> +                  script, "can_fua");
Could we be nice and treat this as "none"?
> @@ -538,12 +602,13 @@ sh_trim (void *handle, uint32_t count, uint64_t
offset)
>   }
>   
>   static int
> -sh_zero (void *handle, uint32_t count, uint64_t offset, int may_trim)
> +sh_zero (void *handle, uint32_t count, uint64_t offset, uint32_t flags)
>   {
>     char *h = handle;
>     char cbuf[32], obuf[32];
> -  const char *args[] = { script, "zero", h, cbuf, obuf,
> -                         may_trim ? "true" : "false",
NULL };
> +  /* In future, comma-separated list of flags. */
While I commented about 'in future' being too terse in the .pod, I
don't
mind it being compressed in the .c file.   On the other hand, the 
comment here is wrong, as sh_zero needs to handle both fua and may_trim 
flags.
> +  const char *sflags = flags & NBDKIT_FLAG_FUA ? "fua" :
"";
and thus this computation for sflags is incomplete.

Otherwise, looking good.

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

On 12/14/18 4:16 PM, Richard W.M. Jones wrote:
> ---
>   tests/Makefile.am    |   3 +
>   tests/test-eflags.sh | 222 +++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 225 insertions(+)
> 
> +# Test export flags.
> +#
> +# Run nbdkit with various can_* callbacks defined and with or without
> +# the -r flag, and check that nbdkit constructs the export flags
> +# controlling READ_ONLY, ROTATIONAL, SEND_TRIM, etc. as expected.
> +#
> +# We use the oldstyle (-o) protocol here because it's simpler to read
> +# out the eflags.
Indeed - the client has to talk in newstyle, but not in oldstyle :)
>    We use the shell plugin because it gives maximum
> +# control over the can_* callbacks (at least, max without having to
> +# write a C plugin).
> +
> +source ./functions.sh
> +set -e
> +
> +if ! socat -h >/dev/null; then
> +    echo "$0: 'socat' command not available"
> +    exit 77
> +fi
> +
> +# Check 'od' command exists and can do endian swaps (which the
FreeBSD
> +# version cannot).
Do we need endian swaps, or can we just read one byte at a time and then 
reconstruct things by hand (after all, we know the bytes are in network 
order, so we don't have to worry about whether we are running on a big- 
or little-endian machine to do the integer math to build up the 16-bit 
value from those two bytes).
> +if ! od --endian=big </dev/null >/dev/null; then
> +    echo "$0: 'od' command not available or doesn't
support endian swaps"
> +    exit 77
> +fi
> +
> +files="eflags.out"
> +rm -f $files
> +cleanup_fn rm -f $files
> +
> +# The export flags.
> +# See also src/protocol.h
Comment may need tweaking if I finish my patches to move src/proto* to 
common/proto/.  We'll see what lands first :)
> +HAS_FLAGS=$((         1 << 0 ))
> +READ_ONLY=$((         1 << 1 ))
> +SEND_FLUSH=$((        1 << 2 ))
> +SEND_FUA=$((          1 << 3 ))
> +ROTATIONAL=$((        1 << 4 ))
> +SEND_TRIM=$((         1 << 5 ))
> +SEND_WRITE_ZEROES=$(( 1 << 6 ))
> +
> +all_flags="HAS_FLAGS READ_ONLY SEND_FLUSH SEND_FUA ROTATIONAL
SEND_TRIM SEND_WRITE_ZEROES"
Well, that's all the flags we currently drive, although more flags exist 
(and we may eventually patch nbdkit to drive those, as well).
> +
> +do_nbdkit ()
> +{
> +    nbdkit -v -U - -o "$@" sh script=- --run '
> +        socat -b 28 unix-connect:$unixsocket \
> +            exec:"dd bs=1 skip=26 count=2 of=eflags.out
status=none"
dd can portably do byteswaps, with conv=swab, but then you have to know 
if you are big- or little-endian to know whether to use it.
> +    '
> +
> +    # By adding 0 we convert the string to an integer (removing
> +    # whitespace).
> +    eflags=$((0 + $(od --endian=big -An -N2 -d < eflags.out)))
So back to my earlier thought of just reading 1 byte at a time and 
building up the value ourselves:

eflags=$(( ($(od -An -N1     -tu1 eflags.out) << 8) |
             $(od -An -N1 -j1 -tu1 eflags.out)         ))
> +    printf "eflags 0x%04x" $eflags
> +    for f in $all_flags; do
> +        [ $(( eflags & ${!f} )) -ne 0 ] && echo -n "
$f"
> +    done
> +    echo
> +}
> +
> +
> +# no -r
> +# can_write=true
> +#
> +# NBD_FLAG_SEND_WRITE_ZEROES is always set on writable connections
> +# even if can_zero returns false, because nbdkit reckons it can
> +# emulate zeroing using pwrite.
And we have the nozero filter if we want things otherwise (if you want 
to use that in this test).

Otherwise, this looks sane.

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