Re: [Libguestfs] [nbdkit PATCH] sh: Add exit status triggers for nbdkit_{shutdown, disconnect}

Make it possible for the sh and eval plugins to disconnect a client or shut down the entire nbdkit server by use of special return values. Prior to this patch we had reserved 4-7 for future use; this defines 4-6, and extends the set of reserved return values to 7-15. We figure it is unlikely that anyone is using status 8-15 with the intent that it behaves identically to status 1. For the testsuite, I only covered the eval plugin; but since it shares common code with the sh plugin, both styles should work. --- Finally got the testsuite additions for this in a state that I like. plugins/sh/nbdkit-sh-plugin.pod | 37 ++++++- tests/Makefile.am | 2 + plugins/sh/call.h | 9 +- plugins/sh/call.c | 85 +++++++-------- tests/test-eval-disconnect.sh | 185 ++++++++++++++++++++++++++++++++ 5 files changed, 268 insertions(+), 50 deletions(-) create mode 100755 tests/test-eval-disconnect.sh diff --git a/plugins/sh/nbdkit-sh-plugin.pod b/plugins/sh/nbdkit-sh-plugin.pod index 2a55fdc9..37139e1b 100644 --- a/plugins/sh/nbdkit-sh-plugin.pod +++ b/plugins/sh/nbdkit-sh-plugin.pod @@ -96,4 +96,4 @@ The script should exit with specific exit codes: The method was executed successfully. -=item 1 and 8-127 +=item 1 and 16-255 There was an error. The script may print on stderr an errno name, optionally followed by whitespace and a message, for example: @@ -123,9 +123,38 @@ The requested method is not supported by the script. For methods which return booleans, this code indicates false. -=item 4, 5, 6, 7 +=item S<4>

-These exit codes are reserved for future use. +Triggers a call to the C function C<nbdkit_shutdown>, which requests +an asynchronous exit of the nbdkit server (disconnecting all clients). +Since the client may get a response before shutdown is complete, +nbdkit then treats empty stderr as success for the current callback, +and parses non-empty stderr as if the script had exited with code +S<1>.

+ +=item S<5> + +Triggers a call to the C function C<nbdkit_disconnect> with C<force> +set to true, which requests an abrupt disconnect of the current +client. The contents of stderr are irrelevant with this status, since +the client will not get a response. + +=item S<6> + +Triggers a call to the C function C<nbdkit_disconnect> with C<force> +set to false, which requests a soft disconnect of the current client +(future client requests are rejected with C<ESHUTDOWN> without calling +into the plugin, but current requests may complete). Since the client +will likely get the response to this command, nbdkit then treats empty +stderr as success for the current callback, and parses non-empty +stderr as if the script had exited with code S<1>.

+ +=item 7-15 + +These exit codes are reserved for future use. Note that versions of +nbdkit E<lt> 1.34 documented that codes S<8> through S<15> behaved +like code S<1>; although it is unlikely that many scripts relied on +this similarity in practice. =back @@ -578,4 +607,4 @@ Richard W.M. Jones =head1 COPYRIGHT -Copyright (C) 2018-2020 Red Hat Inc. +Copyright (C) 2018-2022 Red Hat Inc. diff --git a/tests/Makefile.am b/tests/Makefile.am index d59b797c..3994fc6a 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -768,12 +768,14 @@ TESTS += \ test-eval-file.sh \ test-eval-exports.sh \ test-eval-cache.sh \ + test-eval-disconnect.sh \ $(NULL) EXTRA_DIST += \ test-eval.sh \ test-eval-file.sh \ test-eval-exports.sh \ test-eval-cache.sh \ + test-eval-disconnect.sh \ $(NULL) # file plugin test. diff --git a/plugins/sh/call.h b/plugins/sh/call.h index 76de23a3..44767e81 100644 --- a/plugins/sh/call.h +++ b/plugins/sh/call.h @@ -1,5 +1,5 @@ /* nbdkit - * Copyright (C) 2018 Red Hat Inc. + * Copyright (C) 2018-2022 Red Hat Inc. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions are @@ -51,7 +51,12 @@ typedef enum exit_code { OK = 0, ERROR = 1, /* all script error codes are mapped to this */ MISSING = 2, /* method missing */ - RET_FALSE = 3 /* script exited with code 3 meaning false */ + RET_FALSE = 3, /* script exited with code 3 meaning false */ + SHUTDOWN = 4, /* call nbdkit_shutdown() before parsing stderr */ + DISC_FORCE = 5, /* call nbdkit_disconnect(true) */ + DISC_SOFT = 6, /* call nbdkit_disconnect(false) */ + /* 7 is reserved since 1.8; handle like ERROR for now */ + /* 8-15 are reserved since 1.34; handle like ERROR for now */ } exit_code; extern exit_code call (const char **argv) diff --git a/plugins/sh/call.c b/plugins/sh/call.c index 2fa94d88..7d0f2b16 100644 --- a/plugins/sh/call.c +++ b/plugins/sh/call.c @@ -1,5 +1,5 @@ /* nbdkit - * Copyright (C) 2018-2020 Red Hat Inc. + * Copyright (C) 2018-2022 Red Hat Inc. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions are @@ -397,17 +397,47 @@ call3 (const char *wbuf, size_t wbuflen, /* sent to stdin (can be NULL) */ return ret; } -static void -handle_script_error (const char *argv0, string *ebuf) +/* Normalize return codes and parse error string. */ +static exit_code +handle_script_error (const char *argv0, string *ebuf, exit_code code) { int err; size_t skip = 0; char *p; - if (ebuf->len == 0) { + switch (code) { + case OK: + case MISSING: + case RET_FALSE: + /* Script successful. */ + return code; + + case SHUTDOWN: + /* Use trigger, then handle empty ebuf specially below */ + nbdkit_shutdown (); + break; + + case DISC_FORCE: + case DISC_SOFT: + /* Use trigger, then handle empty ebuf specially below */ + nbdkit_disconnect (code == DISC_FORCE); + break; + + case ERROR: + default: + /* Error even if ebuf is empty */ err = EIO; + goto parse;

+ } + + assert (code >= SHUTDOWN && code <= DISC_SOFT); + if (ebuf->len == 0) + return OK; + err = ESHUTDOWN; + + parse: + if (ebuf->len == 0) goto no_error_message; - } /* Recognize the errno values that match NBD protocol errors */ if (ascii_strncasecmp (ebuf->ptr, "EPERM", 5) == 0) { @@ -502,6 +532,7 @@ handle_script_error (const char *argv0, string *ebuf) /* Set errno. */ errno = err; + return ERROR; } /* Call the script with parameters. Don't write to stdin or read from @@ -516,19 +547,7 @@ call (const char **argv) CLEANUP_FREE_STRING string ebuf = empty_vector; r = call3 (NULL, 0, &rbuf, &ebuf, argv); - switch (r) { - case OK: - case MISSING: - case RET_FALSE: - /* Script successful. */ - return r; - - case ERROR: - default: - /* Error case. */ - handle_script_error (argv[0], &ebuf); - return ERROR; - } + return handle_script_error (argv[0], &ebuf, r); } /* Call the script with parameters. Read from stdout and return the @@ -541,20 +560,10 @@ call_read (string *rbuf, const char **argv) CLEANUP_FREE_STRING string ebuf = empty_vector; r = call3 (NULL, 0, rbuf, &ebuf, argv); - switch (r) { - case OK: - case MISSING: - case RET_FALSE: - /* Script successful. */ - return r; - - case ERROR: - default: - /* Error case. */ + r = handle_script_error (argv[0], &ebuf, r); + if (r == ERROR) string_reset (rbuf); - handle_script_error (argv[0], &ebuf); - return ERROR; - } + return r; } /* Call the script with parameters. Write to stdin of the script. @@ -568,17 +577,5 @@ call_write (const char *wbuf, size_t wbuflen, const char **argv) CLEANUP_FREE_STRING string ebuf = empty_vector; r = call3 (wbuf, wbuflen, &rbuf, &ebuf, argv); - switch (r) { - case OK: - case MISSING: - case RET_FALSE: - /* Script successful. */ - return r; - - case ERROR: - default: - /* Error case. */ - handle_script_error (argv[0], &ebuf); - return ERROR; - } + return handle_script_error (argv[0], &ebuf, r); } diff --git a/tests/test-eval-disconnect.sh b/tests/test-eval-disconnect.sh new file mode 100755 index 00000000..8427e6fd --- /dev/null +++ b/tests/test-eval-disconnect.sh @@ -0,0 +1,185 @@ +#!/usr/bin/env bash +# nbdkit +# Copyright (C) 2022 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. + +# Check shutdown/disconnect behavior triggered by special status values + +source ./functions.sh +set -e +set -x + +requires_plugin eval +requires_nbdsh_uri + +sock=$(mktemp -u /tmp/nbdkit-test-sock.XXXXXX) +files="eval-disconnect.pid $sock" +rm -f $files +cleanup_fn rm -f $files + +# Start nbdkit with a plugin that fails writes according to the export +# name which must be numeric: a positive value leaves stderr empty, a +# non-positive one outputs EPERM first. Serve multiple clients. +serve() +{ + rm -f $files + start_nbdkit -P eval-disconnect.pid -U $sock eval \ + get_size=' echo 1024 ' \ + open=' if test $3 -le 0; then \ + echo EPERM > $tmpdir/err; echo $((-$3)); \ + else \ + : > $tmpdir/err; echo $3; \ + fi ' \ + flush=' exit 0 ' \ + pwrite=' cat >/dev/null; cat $tmpdir/err >&2; exit $2 ' +} +serve + +# Noisy status 0 succeeds, despite text to stderr +nbdsh -u "nbd+unix:///0?socket=$sock" -c - <<\EOF +h.pwrite(b"1", 0) +h.flush() +h.shutdown() +EOF + +# Silent status 1 fails; nbdkit turns lack of error into EIO +nbdsh -u "nbd+unix:///1?socket=$sock" -c - <<\EOF +import errno +try: + h.pwrite(b"1", 0) + assert False +except nbd.Error as ex: + assert ex.errnum == errno.EIO +h.flush() +h.shutdown() +EOF + +# Noisy status 1 fails with supplied EPERM +nbdsh -u "nbd+unix:///-1?socket=$sock" -c - <<\EOF +import errno +try: + h.pwrite(b"1", 0) + assert False +except nbd.Error as ex: + assert ex.errnum == errno.EPERM +h.flush() +h.shutdown() +EOF + +# Silent status 5 kills socket; libnbd detects dead socket as ENOTCONN +nbdsh -u "nbd+unix:///5?socket=$sock" -c - <<\EOF +import errno +try: + h.pwrite(b"1", 0) + assert False +except nbd.Error as ex: + assert ex.errnum == errno.ENOTCONN +assert h.aio_is_ready() is False +EOF + +# Noisy status 5 kills socket; libnbd detects dead socket as ENOTCONN +nbdsh -u "nbd+unix:///-5?socket=$sock" -c - <<\EOF +import errno +try: + h.pwrite(b"1", 0) + assert False +except nbd.Error as ex: + assert ex.errnum == errno.ENOTCONN +assert h.aio_is_ready() is False +EOF + +# Silent status 6 succeeds, but next command fails with ESHUTDOWN +nbdsh -u "nbd+unix:///6?socket=$sock" -c - <<\EOF +import errno +h.pwrite(b"1", 0) +try: + h.flush() + assert False +except nbd.Error as ex: + assert ex.errnum == errno.ESHUTDOWN +h.shutdown() +EOF + +# Noisy status 6 fails with supplied EPERM, next command fails with ESHUTDOWN +nbdsh -u "nbd+unix:///-6?socket=$sock" -c - <<\EOF +import errno +try: + h.pwrite(b"1", 0) + assert False +except nbd.Error as ex: + assert ex.errnum == errno.EPERM +try: + h.flush() + assert False +except nbd.Error as ex: + assert ex.errnum == errno.ESHUTDOWN +h.shutdown() +EOF + +# Silent status 4 kills the server. It is racy whether client sees a reply +# of success or sees the connection killed with libnbd giving ENOTCONN +nbdsh -u "nbd+unix:///4?socket=$sock" -c - <<\EOF +import errno +try: + h.pwrite(b"1", 0) +except nbd.Error as ex: + assert ex.errnum == errno.ENOTCONN +EOF + +# Server should die shortly, if not already dead at this point. +for (( i=0; i < 5; i++ )); do + kill -s 0 "$(cat eval-disconnect.pid)" || break + sleep 1 +done +if [ $i = 5 ]; then + echo "nbdkit didn't exit fast enough" + exit 1 +fi + +# Restart server; noisy status 4 races between EPERM or ENOTCONN +serve +nbdsh -u "nbd+unix:///-4?socket=$sock" -c - <<\EOF +import errno +try: + h.pwrite(b"1", 0) + assert False +except nbd.Error as ex: + assert ex.errnum == errno.EPERM or ex.errnum == errno.ENOTCONN +EOF + +# Server should die shortly, if not already dead at this point. +for (( i=0; i < 5; i++ )); do + kill -s 0 "$(cat eval-disconnect.pid)" || break + sleep 1 +done +if [ $i = 5 ]; then + echo "nbdkit didn't exit fast enough" + exit 1 +fi