On Tue, Nov 01, 2022 at 02:56:09PM -0500, Eric Blake wrote:
...
> +=item S<6>
> +
> +Triggers a call to the C function C<nbdkit_disconnect> with C<force>
> +set to false, which requests a soft disconnect of the current client
> +(future client requests are rejected with C<ESHUTDOWN> without calling
> +into the plugin, but current requests may complete). Since the client
> +will likely get the response to this command, nbdkit then treats empty
> +stderr as success for the current callback, and parses non-empty
> +stderr as if the script had exited with code S<1>.
OK .. seems like complicated behaviour, but I can sort of see the
reasoning behind it.
I do wonder if we just want to use separate codes for the "soft
disconnect + OK" and the "soft disconnect + error" cases. We have
reserved more so we won't run out :-)
> +=item 7-15
> +
> +These exit codes are reserved for future use. Note that versions of
> +nbdkit E<lt> 1.34 documented that codes S<8> through S<15> behaved
This is actually a case where S<> around the nbdkit E<lt> 1.34 makes
sense. But it should be removed around S<8> etc and in the next line
below.
> +like code S<1>; although it is unlikely that many scripts relied on
> +this similarity in practice.
[...]
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index d59b797c..3994fc6a 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -768,12 +768,14 @@ TESTS += \
> test-eval-file.sh \
> test-eval-exports.sh \
> test-eval-cache.sh \
> + test-eval-disconnect.sh \
> $(NULL)
> EXTRA_DIST += \
> test-eval.sh \
> test-eval-file.sh \
> test-eval-exports.sh \
> test-eval-cache.sh \
> + test-eval-disconnect.sh \
> $(NULL)
> # file plugin test.
> diff --git a/plugins/sh/call.h b/plugins/sh/call.h
> index 76de23a3..44767e81 100644
> --- a/plugins/sh/call.h
> +++ b/plugins/sh/call.h
> @@ -1,5 +1,5 @@
> /* nbdkit
> - * Copyright (C) 2018 Red Hat Inc.
> + * Copyright (C) 2018-2022 Red Hat Inc.
> *
> * Redistribution and use in source and binary forms, with or without
> * modification, are permitted provided that the following conditions are
> @@ -51,7 +51,12 @@ typedef enum exit_code {
> OK = 0,
> ERROR = 1, /* all script error codes are mapped to this */
> MISSING = 2, /* method missing */
> - RET_FALSE = 3 /* script exited with code 3 meaning false */
> + RET_FALSE = 3, /* script exited with code 3 meaning false */
> + SHUTDOWN = 4, /* call nbdkit_shutdown() before parsing stderr */
> + DISC_FORCE = 5, /* call nbdkit_disconnect(true) */
> + DISC_SOFT = 6, /* call nbdkit_disconnect(false) */
> + /* 7 is reserved since 1.8; handle like ERROR for now */
> + /* 8-15 are reserved since 1.34; handle like ERROR for now */
> } exit_code;
>
> extern exit_code call (const char **argv)
> diff --git a/plugins/sh/call.c b/plugins/sh/call.c
> index 2fa94d88..7d0f2b16 100644
> --- a/plugins/sh/call.c
> +++ b/plugins/sh/call.c
> @@ -1,5 +1,5 @@
> /* nbdkit
> - * Copyright (C) 2018-2020 Red Hat Inc.
> + * Copyright (C) 2018-2022 Red Hat Inc.
> *
> * Redistribution and use in source and binary forms, with or without
> * modification, are permitted provided that the following conditions are
> @@ -397,17 +397,47 @@ call3 (const char *wbuf, size_t wbuflen, /* sent to
> stdin (can be NULL) */
> return ret;
> }
>
> -static void
> -handle_script_error (const char *argv0, string *ebuf)
> +/* Normalize return codes and parse error string. */
> +static exit_code
> +handle_script_error (const char *argv0, string *ebuf, exit_code code)
> {
> int err;
> size_t skip = 0;
> char *p;
>
> - if (ebuf->len == 0) {
> + switch (code) {
> + case OK:
> + case MISSING:
> + case RET_FALSE:
> + /* Script successful. */
> + return code;
> +
> + case SHUTDOWN:
> + /* Use trigger, then handle empty ebuf specially below */
> + nbdkit_shutdown ();
> + break;
> +
> + case DISC_FORCE:
> + case DISC_SOFT:
> + /* Use trigger, then handle empty ebuf specially below */
> + nbdkit_disconnect (code == DISC_FORCE);
> + break;
> +
> + case ERROR:
> + default:
> + /* Error even if ebuf is empty */
> err = EIO;
> + goto parse;
> + }
> +
> + assert (code >= SHUTDOWN && code <= DISC_SOFT);
> + if (ebuf->len == 0)
> + return OK;
> + err = ESHUTDOWN;
> +
> + parse:
> + if (ebuf->len == 0)
> goto no_error_message;
> - }
I guess if we had the separate codes then we wouldn't need the goto?
> /* Recognize the errno values that match NBD protocol errors */
> if (ascii_strncasecmp (ebuf->ptr, "EPERM", 5) == 0) {
> @@ -502,6 +532,7 @@ handle_script_error (const char *argv0, string *ebuf)
>
> /* Set errno. */
> errno = err;
> + return ERROR;
> }
>
> /* Call the script with parameters. Don't write to stdin or read from
> @@ -516,19 +547,7 @@ call (const char **argv)
> CLEANUP_FREE_STRING string ebuf = empty_vector;
>
> r = call3 (NULL, 0, &rbuf, &ebuf, argv);
> - switch (r) {
> - case OK:
> - case MISSING:
> - case RET_FALSE:
> - /* Script successful. */
> - return r;
> -
> - case ERROR:
> - default:
> - /* Error case. */
> - handle_script_error (argv[0], &ebuf);
> - return ERROR;
> - }
> + return handle_script_error (argv[0], &ebuf, r);
> }
>
> /* Call the script with parameters. Read from stdout and return the
> @@ -541,20 +560,10 @@ call_read (string *rbuf, const char **argv)
> CLEANUP_FREE_STRING string ebuf = empty_vector;
>
> r = call3 (NULL, 0, rbuf, &ebuf, argv);
> - switch (r) {
> - case OK:
> - case MISSING:
> - case RET_FALSE:
> - /* Script successful. */
> - return r;
> -
> - case ERROR:
> - default:
> - /* Error case. */
> + r = handle_script_error (argv[0], &ebuf, r);
> + if (r == ERROR)
> string_reset (rbuf);
> - handle_script_error (argv[0], &ebuf);
> - return ERROR;
> - }
> + return r;
> }
>
> /* Call the script with parameters. Write to stdin of the script.
> @@ -568,17 +577,5 @@ call_write (const char *wbuf, size_t wbuflen, const char
> **argv)
> CLEANUP_FREE_STRING string ebuf = empty_vector;
>
> r = call3 (wbuf, wbuflen, &rbuf, &ebuf, argv);
> - switch (r) {
> - case OK:
> - case MISSING:
> - case RET_FALSE:
> - /* Script successful. */
> - return r;
> -
> - case ERROR:
> - default:
> - /* Error case. */
> - handle_script_error (argv[0], &ebuf);
> - return ERROR;
> - }
> + return handle_script_error (argv[0], &ebuf, r);
> }
> diff --git a/tests/test-eval-disconnect.sh b/tests/test-eval-disconnect.sh
> new file mode 100755
> index 00000000..8427e6fd
> --- /dev/null
> +++ b/tests/test-eval-disconnect.sh
> @@ -0,0 +1,185 @@
> +#!/usr/bin/env bash
> +# nbdkit
> +# Copyright (C) 2022 Red Hat Inc.
> +#
> +# Redistribution and use in source and binary forms, with or without
> +# modification, are permitted provided that the following conditions are
> +# met:
> +#
> +# * Redistributions of source code must retain the above copyright
> +# notice, this list of conditions and the following disclaimer.
> +#
> +# * Redistributions in binary form must reproduce the above copyright
> +# notice, this list of conditions and the following disclaimer in the
> +# documentation and/or other materials provided with the distribution.
> +#
> +# * Neither the name of Red Hat nor the names of its contributors may be
> +# used to endorse or promote products derived from this software without
> +# specific prior written permission.
> +#
> +# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS IS'' AND
> +# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
> +# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
> +# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
> +# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> +# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> +# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
> +# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
> +# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
> +# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
> +# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> +# SUCH DAMAGE.
> +
> +# Check shutdown/disconnect behavior triggered by special status values
> +
> +source ./functions.sh
> +set -e
> +set -x
> +
> +requires_plugin eval
> +requires_nbdsh_uri
> +
> +sock=$(mktemp -u /tmp/nbdkit-test-sock.XXXXXX)
> +files="eval-disconnect.pid $sock"
> +rm -f $files
> +cleanup_fn rm -f $files
> +
> +# Start nbdkit with a plugin that fails writes according to the export
> +# name which must be numeric: a positive value leaves stderr empty, a
> +# non-positive one outputs EPERM first. Serve multiple clients.
> +serve()
> +{
> + rm -f $files
> + start_nbdkit -P eval-disconnect.pid -U $sock eval \
> + get_size=' echo 1024 ' \
> + open=' if test $3 -le 0; then \
> + echo EPERM > $tmpdir/err; echo $((-$3)); \
> + else \
> + : > $tmpdir/err; echo $3; \
> + fi ' \
> + flush=' exit 0 ' \
> + pwrite=' cat >/dev/null; cat $tmpdir/err >&2; exit $2 '
> +}
> +serve
> +
> +# Noisy status 0 succeeds, despite text to stderr
> +nbdsh -u "nbd+unix:///0?socket=$sock" -c - <<\EOF
> +h.pwrite(b"1", 0)
> +h.flush()
> +h.shutdown()
> +EOF
> +
> +# Silent status 1 fails; nbdkit turns lack of error into EIO
> +nbdsh -u "nbd+unix:///1?socket=$sock" -c - <<\EOF
> +import errno
> +try:
> + h.pwrite(b"1", 0)
> + assert False
> +except nbd.Error as ex:
> + assert ex.errnum == errno.EIO
> +h.flush()
> +h.shutdown()
> +EOF
> +
> +# Noisy status 1 fails with supplied EPERM
> +nbdsh -u "nbd+unix:///-1?socket=$sock" -c - <<\EOF
> +import errno
> +try:
> + h.pwrite(b"1", 0)
> + assert False
> +except nbd.Error as ex:
> + assert ex.errnum == errno.EPERM
> +h.flush()
> +h.shutdown()
> +EOF
> +
> +# Silent status 5 kills socket; libnbd detects dead socket as ENOTCONN
> +nbdsh -u "nbd+unix:///5?socket=$sock" -c - <<\EOF
> +import errno
> +try:
> + h.pwrite(b"1", 0)
> + assert False
> +except nbd.Error as ex:
> + assert ex.errnum == errno.ENOTCONN
> +assert h.aio_is_ready() is False
> +EOF
> +
> +# Noisy status 5 kills socket; libnbd detects dead socket as ENOTCONN
> +nbdsh -u "nbd+unix:///-5?socket=$sock" -c - <<\EOF
> +import errno
> +try:
> + h.pwrite(b"1", 0)
> + assert False
> +except nbd.Error as ex:
> + assert ex.errnum == errno.ENOTCONN
> +assert h.aio_is_ready() is False
> +EOF
> +
> +# Silent status 6 succeeds, but next command fails with ESHUTDOWN
> +nbdsh -u "nbd+unix:///6?socket=$sock" -c - <<\EOF
> +import errno
> +h.pwrite(b"1", 0)
> +try:
> + h.flush()
> + assert False
> +except nbd.Error as ex:
> + assert ex.errnum == errno.ESHUTDOWN
> +h.shutdown()
> +EOF
> +
> +# Noisy status 6 fails with supplied EPERM, next command fails with ESHUTDOWN
> +nbdsh -u "nbd+unix:///-6?socket=$sock" -c - <<\EOF
> +import errno
> +try:
> + h.pwrite(b"1", 0)
> + assert False
> +except nbd.Error as ex:
> + assert ex.errnum == errno.EPERM
> +try:
> + h.flush()
> + assert False
> +except nbd.Error as ex:
> + assert ex.errnum == errno.ESHUTDOWN
> +h.shutdown()
> +EOF
> +
> +# Silent status 4 kills the server. It is racy whether client sees a reply
> +# of success or sees the connection killed with libnbd giving ENOTCONN
> +nbdsh -u "nbd+unix:///4?socket=$sock" -c - <<\EOF
> +import errno
> +try:
> + h.pwrite(b"1", 0)
> +except nbd.Error as ex:
> + assert ex.errnum == errno.ENOTCONN
> +EOF
> +
> +# Server should die shortly, if not already dead at this point.
> +for (( i=0; i < 5; i++ )); do
> + kill -s 0 "$(cat eval-disconnect.pid)" || break
> + sleep 1
> +done
> +if [ $i = 5 ]; then
> + echo "nbdkit didn't exit fast enough"
> + exit 1
> +fi
> +
> +# Restart server; noisy status 4 races between EPERM or ENOTCONN
> +serve
> +nbdsh -u "nbd+unix:///-4?socket=$sock" -c - <<\EOF
> +import errno
> +try:
> + h.pwrite(b"1", 0)
> + assert False
> +except nbd.Error as ex:
> + assert ex.errnum == errno.EPERM or ex.errnum == errno.ENOTCONN
> +EOF
> +
> +# Server should die shortly, if not already dead at this point.
> +for (( i=0; i < 5; i++ )); do
> + kill -s 0 "$(cat eval-disconnect.pid)" || break
> + sleep 1
> +done
> +if [ $i = 5 ]; then
> + echo "nbdkit didn't exit fast enough"
> + exit 1
> +fi
I'm not a big fan of these "omni-tests" (tests that test many related
or even unrelated features). But I guess it's fine unless you wanted
to split it.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
