8:21 p.m.
In commit c306fa93ab and neighbors (v1.15.1), a concerted effort went
into caching the results of .can_FOO callbacks, with commit messages
demonstrating that a plugin with a slow callback should not have that
delay magnified multiple times. But nothing was added to the
testsuite at the time, and with the sh and eval plugins, we still have
two scenarios where we did not take advantage of the nbdkit cache
because we directly called things ourselves (one pre-existing since
v1.13.9, another introduced in v1.15.1 right after the cleanups). Fix
those spots by reworking the handle we pass to nbdkit, then enhance
test-eflags.sh to ensure we don't regress again.
Note that nbdkit does not yet cache .thread_model; that will be
addressed in the next patch. Furthermore, we end up duplicating the
caching that nbdkit itself does, because it would be a layering
violation for us to have enough information to call into
backend_can_flush().
Fixes: 05c5eed6f2
Fixes: 8490694d08
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
plugins/sh/methods.c | 102 +++++++++++++++++++++++++++----------------
tests/test-eflags.sh | 44 +++++++++++++++++--
2 files changed, 105 insertions(+), 41 deletions(-)
diff --git a/plugins/sh/methods.c b/plugins/sh/methods.c
index 56e2d410..cce143de 100644
--- a/plugins/sh/methods.c
+++ b/plugins/sh/methods.c
@@ -1,5 +1,5 @@
/* nbdkit
- * Copyright (C) 2018-2019 Red Hat Inc.
+ * Copyright (C) 2018-2020 Red Hat Inc.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are
@@ -193,29 +193,42 @@ sh_preconnect (int readonly)
}
}
+struct sh_handle {
+ int can_flush;
+ int can_zero;
+ char *h;
+};
+
void *
sh_open (int readonly)
{
const char *method = "open";
const char *script = get_script (method);
- char *h = NULL;
size_t hlen;
const char *args[] =
{ script, method,
readonly ? "true" : "false",
nbdkit_export_name () ? : "",
NULL };
+ struct sh_handle *h = malloc (sizeof *h);
+
+ if (!h) {
+ nbdkit_error ("malloc: %m");
+ return NULL;
+ }
+ h->can_flush = -1;
+ h->can_zero = -1;
/* We store the string returned by open in the handle. */
- switch (call_read (&h, &hlen, args)) {
+ switch (call_read (&h->h, &hlen, args)) {
case OK:
/* Remove final newline if present. */
- if (hlen > 0 && h[hlen-1] == '\n') {
- h[hlen-1] = '\0';
+ if (hlen > 0 && h->h[hlen-1] == '\n') {
+ h->h[hlen-1] = '\0';
hlen--;
}
if (hlen > 0)
- nbdkit_debug ("sh: handle: %s", h);
+ nbdkit_debug ("sh: handle: %s", h->h);
return h;
case MISSING:
@@ -223,17 +236,19 @@ sh_open (int readonly)
* missing then we return "" as the handle. Allocate a new string
* for it because we don't know what call_read returned here.
*/
- free (h);
- h = strdup ("");
- if (h == NULL)
+ free (h->h);
+ h->h = strdup ("");
+ if (h->h == NULL)
nbdkit_error ("strdup: %m");
return h;
case ERROR:
+ free (h->h);
free (h);
return NULL;
case RET_FALSE:
+ free (h->h);
free (h);
nbdkit_error ("%s: %s method returned unexpected code (3/false)",
script, method);
@@ -249,14 +264,15 @@ sh_close (void *handle)
{
const char *method = "close";
const char *script = get_script (method);
- char *h = handle;
- const char *args[] = { script, method, h, NULL };
+ struct sh_handle *h = handle;
+ const char *args[] = { script, method, h->h, NULL };
switch (call (args)) {
case OK:
case MISSING:
case ERROR:
case RET_FALSE:
+ free (h->h);
free (h);
return;
default: abort ();
@@ -268,8 +284,8 @@ sh_get_size (void *handle)
{
const char *method = "get_size";
const char *script = get_script (method);
- char *h = handle;
- const char *args[] = { script, method, h, NULL };
+ struct sh_handle *h = handle;
+ const char *args[] = { script, method, h->h, NULL };
CLEANUP_FREE char *s = NULL;
size_t slen;
int64_t r;
@@ -307,9 +323,9 @@ sh_pread (void *handle, void *buf, uint32_t count, uint64_t offset,
{
const char *method = "pread";
const char *script = get_script (method);
- char *h = handle;
+ struct sh_handle *h = handle;
char cbuf[32], obuf[32];
- const char *args[] = { script, method, h, cbuf, obuf, NULL };
+ const char *args[] = { script, method, h->h, cbuf, obuf, NULL };
CLEANUP_FREE char *data = NULL;
size_t len;
@@ -395,9 +411,9 @@ sh_pwrite (void *handle, const void *buf, uint32_t count, uint64_t
offset,
{
const char *method = "pwrite";
const char *script = get_script (method);
- char *h = handle;
+ struct sh_handle *h = handle;
char cbuf[32], obuf[32], fbuf[32];
- const char *args[] = { script, method, h, cbuf, obuf, fbuf, NULL };
+ const char *args[] = { script, method, h->h, cbuf, obuf, fbuf, NULL };
snprintf (cbuf, sizeof cbuf, "%" PRIu32, count);
snprintf (obuf, sizeof obuf, "%" PRIu64, offset);
@@ -429,8 +445,8 @@ static int
boolean_method (const char *script, const char *method,
void *handle, int def)
{
- char *h = handle;
- const char *args[] = { script, method, h, NULL };
+ struct sh_handle *h = handle;
+ const char *args[] = { script, method, h->h, NULL };
switch (call (args)) {
case OK: /* true */
@@ -457,8 +473,14 @@ int
sh_can_flush (void *handle)
{
const char *method = "can_flush";
- const char *script = get_script (method);
- return boolean_method (script, method, handle, 0);
+ const char *script;
+ struct sh_handle *h = handle;
+
+ if (h->can_flush >= 0)
+ return h->can_flush;
+
+ script = get_script (method);
+ return h->can_flush = boolean_method (script, method, handle, 0);
}
int
@@ -481,8 +503,14 @@ int
sh_can_zero (void *handle)
{
const char *method = "can_zero";
- const char *script = get_script (method);
- return boolean_method (script, method, handle, 0);
+ const char *script;
+ struct sh_handle *h = handle;
+
+ if (h->can_zero >= 0)
+ return h->can_zero;
+
+ script = get_script (method);
+ return h->can_zero = boolean_method (script, method, handle, 0);
}
int
@@ -507,8 +535,8 @@ sh_can_fua (void *handle)
{
const char *method = "can_fua";
const char *script = get_script (method);
- char *h = handle;
- const char *args[] = { script, method, h, NULL };
+ struct sh_handle *h = handle;
+ const char *args[] = { script, method, h->h, NULL };
CLEANUP_FREE char *s = NULL;
size_t slen;
int r;
@@ -562,8 +590,8 @@ sh_can_cache (void *handle)
{
const char *method = "can_cache";
const char *script = get_script (method);
- char *h = handle;
- const char *args[] = { script, method, h, NULL };
+ struct sh_handle *h = handle;
+ const char *args[] = { script, method, h->h, NULL };
CLEANUP_FREE char *s = NULL;
size_t slen;
int r;
@@ -627,8 +655,8 @@ sh_flush (void *handle, uint32_t flags)
{
const char *method = "flush";
const char *script = get_script (method);
- char *h = handle;
- const char *args[] = { script, method, h, NULL };
+ struct sh_handle *h = handle;
+ const char *args[] = { script, method, h->h, NULL };
switch (call (args)) {
case OK:
@@ -656,9 +684,9 @@ sh_trim (void *handle, uint32_t count, uint64_t offset, uint32_t
flags)
{
const char *method = "trim";
const char *script = get_script (method);
- char *h = handle;
+ struct sh_handle *h = handle;
char cbuf[32], obuf[32], fbuf[32];
- const char *args[] = { script, method, h, cbuf, obuf, fbuf, NULL };
+ const char *args[] = { script, method, h->h, cbuf, obuf, fbuf, NULL };
snprintf (cbuf, sizeof cbuf, "%" PRIu32, count);
snprintf (obuf, sizeof obuf, "%" PRIu64, offset);
@@ -690,9 +718,9 @@ sh_zero (void *handle, uint32_t count, uint64_t offset, uint32_t
flags)
{
const char *method = "zero";
const char *script = get_script (method);
- char *h = handle;
+ struct sh_handle *h = handle;
char cbuf[32], obuf[32], fbuf[32];
- const char *args[] = { script, method, h, cbuf, obuf, fbuf, NULL };
+ const char *args[] = { script, method, h->h, cbuf, obuf, fbuf, NULL };
snprintf (cbuf, sizeof cbuf, "%" PRIu32, count);
snprintf (obuf, sizeof obuf, "%" PRIu64, offset);
@@ -795,9 +823,9 @@ sh_extents (void *handle, uint32_t count, uint64_t offset, uint32_t
flags,
{
const char *method = "extents";
const char *script = get_script (method);
- char *h = handle;
+ struct sh_handle *h = handle;
char cbuf[32], obuf[32], fbuf[32];
- const char *args[] = { script, method, h, cbuf, obuf, fbuf, NULL };
+ const char *args[] = { script, method, h->h, cbuf, obuf, fbuf, NULL };
CLEANUP_FREE char *s = NULL;
size_t slen;
int r;
@@ -840,9 +868,9 @@ sh_cache (void *handle, uint32_t count, uint64_t offset, uint32_t
flags)
{
const char *method = "cache";
const char *script = get_script (method);
- char *h = handle;
+ struct sh_handle *h = handle;
char cbuf[32], obuf[32];
- const char *args[] = { script, method, h, cbuf, obuf, NULL };
+ const char *args[] = { script, method, h->h, cbuf, obuf, NULL };
snprintf (cbuf, sizeof cbuf, "%" PRIu32, count);
snprintf (obuf, sizeof obuf, "%" PRIu64, offset);
diff --git a/tests/test-eflags.sh b/tests/test-eflags.sh
index 9b3a6a3a..3228bbf6 100755
--- a/tests/test-eflags.sh
+++ b/tests/test-eflags.sh
@@ -1,6 +1,6 @@
#!/usr/bin/env bash
# nbdkit
-# Copyright (C) 2018-2019 Red Hat Inc.
+# Copyright (C) 2018-2020 Red Hat Inc.
#
# Redistribution and use in source and binary forms, with or without
# modification, are permitted provided that the following conditions are
@@ -50,7 +50,7 @@ if ! qemu-nbd --help | grep -sq -- --list; then
exit 77
fi
-files="eflags.out"
+files="eflags.out eflags.err"
late_args=
rm -f $files
cleanup_fn rm -f $files
@@ -72,12 +72,29 @@ SEND_FAST_ZERO=$(( 1 << 11 ))
do_nbdkit ()
{
- nbdkit -v -U - "$@" sh - $late_args --run 'qemu-nbd --list -k
$unixsocket' |
- grep -E "flags: 0x" | grep -Eoi '0x[a-f0-9]+' > eflags.out
+ # Prepend a check for internal caching to the script on stdin.
+ { printf %s '
+ if test $1 = thread_model; then
+ :
+ elif test -f $tmpdir/seen_$1; then
+ echo "repeat call to $1"
>>'"$PWD/eflags.err"'
+ else
+ touch $tmpdir/seen_$1
+ fi
+ '; cat; } | nbdkit -v -U - "$@" sh - $late_args \
+ --run 'qemu-nbd --list -k $unixsocket' |
+ grep -E "flags: 0x" | grep -Eoi '0x[a-f0-9]+' >eflags.out
2>eflags.err
printf eflags=; cat eflags.out
# Convert hex flags to decimal and assign it to $eflags.
eflags=$(printf "%d" $(cat eflags.out))
+
+ # See if nbdkit failed to cache a callback.
+ if test -s eflags.err; then
+ echo "error: nbdkit did not cache callbacks properly"
+ cat eflags.err
+ exit 1
+ fi
}
fail ()
@@ -327,6 +344,25 @@ EOF
[ $eflags -eq $(( HAS_FLAGS|READ_ONLY|SEND_DF )) ] ||
fail "$LINENO: expected HAS_FLAGS|READ_ONLY|SEND_DF"
+#----------------------------------------------------------------------
+# -r
+# can_write=true
+# can_flush=true
+#
+# Setting read-only does not ignore can_flush.
+
+do_nbdkit -r <<'EOF'
+case "$1" in
+ get_size) echo 1M ;;
+ can_write) exit 0 ;;
+ can_flush) exit 0 ;;
+ *) exit 2 ;;
+esac
+EOF
+
+[ $eflags -eq $(( HAS_FLAGS|READ_ONLY|SEND_FLUSH|SEND_DF )) ] ||
+ fail "$LINENO: expected HAS_FLAGS|READ_ONLY|SEND_FLUSH|SEND_DF"
+
#----------------------------------------------------------------------
# can_write=true
# can_flush=true
--
2.25.1