On 11/09/22 21:43, Eric Blake wrote:

We are about to define specific meaning to more return status values. To make it easier to probe if nbdkit is new enough to honor such definitions (and thus whether it makes sense to write a script that tries to utilize such special return values), it is worth advertising the max known status supported by the given build of the sh/eval plugin. --- plugins/eval/nbdkit-eval-plugin.pod | 3 +- plugins/sh/nbdkit-sh-plugin.pod | 5 ++++ tests/Makefile.am | 2 ++ plugins/sh/call.h | 6 ++-- plugins/sh/methods.c | 6 +++- tests/test-eval-dump-plugin.sh | 43 +++++++++++++++++++++++++++++ 6 files changed, 61 insertions(+), 4 deletions(-) create mode 100755 tests/test-eval-dump-plugin.sh

diff --git a/plugins/eval/nbdkit-eval-plugin.pod b/plugins/eval/nbdkit-eval-plugin.pod index fa9784ae..0c10ae43 100644 --- a/plugins/eval/nbdkit-eval-plugin.pod +++ b/plugins/eval/nbdkit-eval-plugin.pod @@ -146,7 +146,8 @@ The script fragment behaves the same way as the corresponding method in L<nbdkit-sh-plugin(1)>. In particular, parameters are identical, C<$tmpdir> is present and used in the same way, the exit code must be one of the valid exit codes described in that manual page, and error -handling works the same way too. +handling works the same way too. Likewise, B<nbdkit --dump-plugin +eval> includes a line for B<max_known_status=> in nbdkit E<ge> 1.34.

Note that a C<config> callback will only handle keys not recognized as callback names; when picking key=value pairs that you want your script diff --git a/plugins/sh/nbdkit-sh-plugin.pod b/plugins/sh/nbdkit-sh-plugin.pod index 2a55fdc9..1c539599 100644 --- a/plugins/sh/nbdkit-sh-plugin.pod +++ b/plugins/sh/nbdkit-sh-plugin.pod @@ -129,6 +129,11 @@ These exit codes are reserved for future use.

=back

+In nbdkit E<gt> 1.34, it is possible to probe whether additional exit +codes have been assigned meaning, by looking for the line +B<max_known_status=> in the output of B<nbdkit --dump-plugin sh>. If +this line is not present, exit codes 4 and above behave like status 1. + =head2 Temporary directory

A fresh script is invoked for each method call (ie. scripts are diff --git a/tests/Makefile.am b/tests/Makefile.am index d59b797c..a4e93686 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-dump-plugin.sh \ $(NULL) EXTRA_DIST += \ test-eval.sh \ test-eval-file.sh \ test-eval-exports.sh \ test-eval-cache.sh \ + test-eval-dump-plugin.sh \ $(NULL)

# file plugin test. diff --git a/plugins/sh/call.h b/plugins/sh/call.h index 76de23a3..fab77a3c 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,9 @@ 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 */ + /* Adjust methods.c:sh_dump_plugin when defining new codes */

We could introduce an "out-of-band" value for terminating the enum list (such as SH_EXIT_CODE_MAX); it would never be handled as an actual exit code, but could be referenced in sh_dump_plugin(). New exit codes would be inserted just before it. But if this "pattern" is not used in nbdkit already, then the current comment will work perfectly well. (I'm mentioning it because it's extensively used e.g. in edk2.)

+ /* 4-7 is reserved since 1.8; handle like ERROR for now */ } exit_code;

extern exit_code call (const char **argv) diff --git a/plugins/sh/methods.c b/plugins/sh/methods.c index 5c1ee5f8..4a75a3a0 100644 --- a/plugins/sh/methods.c +++ b/plugins/sh/methods.c @@ -1,5 +1,5 @@ /* nbdkit - * Copyright (C) 2018-2021 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 @@ -58,6 +58,10 @@ sh_dump_plugin (void) const char *args[] = { script, method, NULL }; CLEANUP_FREE_STRING string o = empty_vector;

+ /* Dump information about the sh/eval features */ + printf ("max_known_status=%d\n", RET_FALSE); + + /* Dump any additional information from the script */

Tricky comment; the "--dump-plugin" option with the "dump_plugin=" operand at the end of the test made me do a double-take :) The documentation covers it though: https://libguestfs.org/nbdkit-sh-plugin.3.html#Methods https://libguestfs.org/nbdkit-plugin.3.html#dump_plugin Reviewed-by: Laszlo Ersek <lersek@redhat.com> Laszlo

if (script) { /* Call dump_plugin method. */ switch (call_read (&o, args)) { diff --git a/tests/test-eval-dump-plugin.sh b/tests/test-eval-dump-plugin.sh new file mode 100755 index 00000000..ba25efbc --- /dev/null +++ b/tests/test-eval-dump-plugin.sh @@ -0,0 +1,43 @@ +#!/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 --dump-plugin operation using eval + +source ./functions.sh +set -e +set -x + +requires_plugin eval + +nbdkit --dump-plugin eval | grep '^max_known_status=' + +nbdkit --dump-plugin eval dump_plugin='echo extra=stuff' | grep '^extra=stuff$'

On 11/09/22 21:43, 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.

Per perlpod(1), the canonical Formatting Code for this would be Z<>, as in (untested): =item Z<>6 However, I can't see a single instance of Z<> in the v2v projects, so it could cause compatibility problems. I guess let's stick with S<> then.

--- 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;

The user-facing documentation and the internal comment on DISC_FORCE both state that stderr and the retval are irrelevant, due to the client never getting a response after a forcible disconnect. Therefore, I *think* we could simplify the code here, like this: case DISC_FORCE: /* hoisted to match enum order */ /* nbdkit_disconnect() argument simplified */ /* stderr processing entirely skipped, as it is irrelevant anyway */ nbdkit_disconnect (true); return OK; /* note: will never reach the client anyway */ case DISC_SOFT_OK: /* unchanged */ nbdkit_disconnect (false); return OK; case DISC_SOFT_ERR: /* nbdkit_disconnect() argument simplified */ nbdkit_disconnect (false); err = ESHUTDOWN; break; Benefits I see from this: - small runtime benefit of eliminating the stderr processing code - *much* easier to read for a human (IMO) Of course it all hinges on whether we are indeed allowed to return "OK" after DISC_FORCE! (We might as well return MISSING or RET_FALSE as well -- basically any return status that is not an error report, hence does not require the setting of "errno", from parsing stderr, or from an appropriate default.) This is a really complex refactoring, I think as-is it is entirely correct wrt. observable behavior, I just find it a little bit hard to read (due to the fallthrough from DISC_FORCE), and a tiny bit runtime-wasteful (same reason). NB I've only reviewed one call site thus far (from call()).

+ + 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; }

Hmm OK I guess this is where my theory on DISC_FORCE may be flawed. If DISC_FORCE is supposed to call string_reset() here, then we can't return OK for DISC_FORCE from handle_script_error(). But is it really a problem if we propagate "data read from stdout" after a forcible disconnect?

/* 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"

I'm mostly just skimming the test case; but: should we emit this complaint to the test case's stderr?

+ exit 1 + fi +} +serve + +# Noisy status 0 (OK) succeeds, despite text to stderr +nbdsh -u "nbd+unix:///0?socket=$sock" -c - <<\EOF

What's the deal with <<\EOF, why not just <<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

Whoa, complicated; many-many cases :) It does look correct to me. With the proposed updates, or without: Reviewed-by: Laszlo Ersek <lersek@redhat.com> Laszlo

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@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@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