Eric Blake
2022-Nov-09 20:43 UTC
[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
Eric Blake
2022-Nov-09 21:25 UTC
[Libguestfs] [nbdkit PATCH v2 2/2] sh: Add exit status triggers for nbdkit_{shutdown, disconnect}
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. > ---> +=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.Oops, the use of S<> in this paragraph is not necessary. I've adjusted it locally. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Laszlo Ersek
2022-Nov-10 15:08 UTC
[Libguestfs] [nbdkit PATCH v2 2/2] sh: Add exit status triggers for nbdkit_{shutdown, disconnect}
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 - <<\EOFWhat'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 at redhat.com> Laszlo
Richard W.M. Jones
2022-Nov-15 16:07 UTC
[Libguestfs] [nbdkit PATCH v2 2/2] sh: Add exit status triggers for nbdkit_{shutdown, disconnect}
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 8Isn'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