[Libguestfs] [nbdkit PATCH v2 2/2] sh: Add exit status triggers for nbdkit_{shutdown, disconnect}
Richard W.M. Jones
rjones at redhat.com
Tue Nov 15 16:07:33 UTC 2022
On Wed, Nov 09, 2022 at 02:43:53PM -0600, Eric Blake wrote:
> 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
Isn't S<> needed here?
> +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;
> }
I agree with Laszlo that this is a bit strange and it's hard to know
if all callers are prepared for this.
It's a case of reusing the same enum for two distinct domains: domain
(A) is the error code returned from the script (enum exit_code), and
domain (B) is the subsequent action of callers of handle_script_error.
I think at a minimum it needs more comment at the top about exactly
which subset of exit_code (A) that the function can return (B), and
what each case means, plus careful inspection of each call site. Or a
second enum.
I'm fine whatever you want to do about it though.
Reviewed-by: Richard W.M. Jones <rjones at redhat.com>
Rich.
> /* 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 at redhat.com
> https://listman.redhat.com/mailman/listinfo/libguestfs
--
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
More information about the Libguestfs
mailing list