Re: [Libguestfs] [nbdkit PATCH v2 2/2] 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-8, and extends the set of reserved return values to 9-15. We figure it is unlikely that anyone is using status 4-15 with the intent that it behaves identically to status 1; more importantly, the choice of soft disconnect with error for status 8 is the one least likely to break such an existing sh or eval script. For the testsuite, I only covered the eval plugin; but since it shares common code with the sh plugin, both styles should work. A note about the pod documentation: when the argument to =item would otherwise appear to be a single number, the use of S<> to mask it is necessary. --- plugins/sh/nbdkit-sh-plugin.pod | 41 +++++- tests/Makefile.am | 2 + plugins/sh/call.h | 7 +- plugins/sh/call.c | 92 ++++++------- plugins/sh/methods.c | 2 +- tests/test-eval-disconnect.sh | 236 ++++++++++++++++++++++++++++++++ 6 files changed, 324 insertions(+), 56 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 1c539599..2905eced 100644 --- a/plugins/sh/nbdkit-sh-plugin.pod +++ b/plugins/sh/nbdkit-sh-plugin.pod @@ -96,7 +96,7 @@ 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,42 @@ The requested method is not supported by the script. For methods which return booleans, this code indicates false. -=item 4, 5, 6, 7 +=item 4 and 5 -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). +The client will usually get a response before shutdown is complete +(although this is racy); so once the shutdown is requested, code 4 +then behaves like code 0 (stderr is ignored, and the server tries to +return success), and code 5 behaves like code 1 (the server tries to +return an error to the client parsed from stderr, although a missing +error defaults to C<ESHUTDOWN> instead of C<EIO>). + +=item S<6> + +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 7 and 8

+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, code 7 then behaves like +code 0 (stderr is ignored, and the server tries to return success), +and code 8 behaves like code 1 (the server tries to return an error to +the client parsed from stderr, although a missing error defaults to +C<ESHUTDOWN> instead of C<EIO>). + +=item 9-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 @@ -583,4 +616,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 a4e93686..b58d2d65 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -769,6 +769,7 @@ TESTS += \ test-eval-exports.sh \ test-eval-cache.sh \ test-eval-dump-plugin.sh \ + test-eval-disconnect.sh \ $(NULL) EXTRA_DIST += \ test-eval.sh \ @@ -776,6 +777,7 @@ EXTRA_DIST += \ test-eval-exports.sh \ test-eval-cache.sh \ test-eval-dump-plugin.sh \ + test-eval-disconnect.sh \ $(NULL) # file plugin test. diff --git a/plugins/sh/call.h b/plugins/sh/call.h index fab77a3c..57da7e98 100644 --- a/plugins/sh/call.h +++ b/plugins/sh/call.h @@ -52,8 +52,13 @@ typedef enum exit_code { ERROR = 1, /* all script error codes are mapped to this */ MISSING = 2, /* method missing */ RET_FALSE = 3, /* script exited with code 3 meaning false */ + SHUTDOWN_OK = 4, /* call nbdkit_shutdown(), then return OK */ + SHUTDOWN_ERR = 5, /* call nbdkit_shutdown(), then return ERROR */ + DISC_FORCE = 6, /* call nbdkit_disconnect(true); return irrelevant */ + DISC_SOFT_OK = 7, /* call nbdkit_disconnect(false), return OK */ + DISC_SOFT_ERR = 8, /* call nbdkit_disconnect(false), return ERROR */ /* Adjust methods.c:sh_dump_plugin when defining new codes */ - /* 4-7 is reserved since 1.8; handle like ERROR for now */ + /* 9-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..64f29079 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 @@ -44,6 +44,7 @@ #include <poll.h> #include <sys/types.h> #include <sys/wait.h> +#include <stdbool.h> #include <nbdkit-plugin.h> @@ -397,18 +398,48 @@ 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_OK: + nbdkit_shutdown (); + return OK; + + case SHUTDOWN_ERR: + nbdkit_shutdown (); + err = ESHUTDOWN; + break; + + case DISC_SOFT_OK: + nbdkit_disconnect (false); + return OK; + + case DISC_FORCE: + case DISC_SOFT_ERR: + nbdkit_disconnect (code == DISC_FORCE); + err = ESHUTDOWN; + break; + + case ERROR: + default: err = EIO; - goto no_error_message; + break; } + assert (ebuf->ptr); /* Even if empty, ebuf was NUL-terminated in call3 */ + /* Recognize the errno values that match NBD protocol errors */ if (ascii_strncasecmp (ebuf->ptr, "EPERM", 5) == 0) { err = EPERM; @@ -463,11 +494,6 @@ handle_script_error (const char *argv0, string *ebuf) err = EFBIG; skip = 5; } - /* Default to EIO. */ - else { - err = EIO; - skip = 0; - } if (skip && ebuf->ptr[skip]) { if (!ascii_isspace (ebuf->ptr[skip])) { @@ -495,13 +521,13 @@ handle_script_error (const char *argv0, string *ebuf) nbdkit_error ("%s: %s", argv0, &ebuf->ptr[skip]); } else { - no_error_message: nbdkit_error ("%s: script exited with error, " "but did not print an error message on stderr", argv0); } /* Set errno. */ errno = err; + return ERROR; }

/* Call the script with parameters. Don't write to stdin or read from @@ -516,19 +542,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 +555,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 +572,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/plugins/sh/methods.c b/plugins/sh/methods.c index 4a75a3a0..834393b2 100644 --- a/plugins/sh/methods.c +++ b/plugins/sh/methods.c @@ -59,7 +59,7 @@ sh_dump_plugin (void) CLEANUP_FREE_STRING string o = empty_vector; /* Dump information about the sh/eval features */ - printf ("max_known_status=%d\n", RET_FALSE); + printf ("max_known_status=%d\n", DISC_SOFT_ERR); /* Dump any additional information from the script */ if (script) { diff --git a/tests/test-eval-disconnect.sh b/tests/test-eval-disconnect.sh new file mode 100755 index 00000000..e7853f31 --- /dev/null +++ b/tests/test-eval-disconnect.sh @@ -0,0 +1,236 @@ +#!/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 ' +} +check_dead() +{ + # 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 +} +serve + +# Noisy status 0 (OK) 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 (ERROR) 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 (ERROR) 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 4 (SHUTDOWN_OK) kills the server. It is racy whether client +# sees success or if the connection is 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 +check_dead +serve + +# Noisy status 4 (SHUTDOWN_OK) kills the server. It is racy whether client +# sees success or if the connection is 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 +check_dead +serve + +# Silent status 5 (SHUTDOWN_ERR) kills the server. It is racy whether client +# sees implied ESHUTDOWN or if the connection dies with libnbd giving 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.ESHUTDOWN or ex.errnum == errno.ENOTCONN +EOF +check_dead +serve + +# Noisy status 5 (SHUTDOWN_ERR) kills the server. It is racy whether client +# sees EPERM or if the connection is killed with libnbd giving 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.EPERM or ex.errnum == errno.ENOTCONN +EOF +check_dead +serve + +# Silent status 6 (DISC_FORCE) kills socket; libnbd detects as ENOTCONN +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.ENOTCONN +assert h.aio_is_ready() is False +EOF + +# Noisy status 6 (DISC_FORCE) kills socket; libnbd detects as ENOTCONN +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.ENOTCONN +assert h.aio_is_ready() is False +EOF + +# Silent status 7 (DISC_SOFT_OK) succeeds, but next command gives ESHUTDOWN +nbdsh -u "nbd+unix:///7?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 7 (DISC_SOFT_OK) succeeds, but next command gives ESHUTDOWN +nbdsh -u "nbd+unix:///-7?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 + +# Silent status 8 (DISC_SOFT_ERR) fails with implied ESHUTDOWN, then next +# command gives ESHUTDOWN +nbdsh -u "nbd+unix:///8?socket=$sock" -c - <<\EOF +import errno +try: + h.pwrite(b"1", 0) + assert False +except nbd.Error as ex: + assert ex.errnum == errno.ESHUTDOWN +try: + h.flush() + assert False +except nbd.Error as ex: + assert ex.errnum == errno.ESHUTDOWN +h.shutdown() +EOF + +# Noisy status 8 (DISC_SOFT_ERR) fails with explicit EPERM, then next +# command gives ESHUTDOWN +nbdsh -u "nbd+unix:///-8?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 -- 2.38.1 _______________________________________________ Libguestfs mailing list Libguestfs(a)redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs