3:19 a.m.
At the moment nbdkit_nanosleep gives an incorrect error message if it
aborts early. Even in the case when the server is not actually
shutting down it will say:
$ nbdkit --filter=delay null delay-close=3 --run 'nbdinfo --size $uri; nbdinfo
--size $uri'
0
nbdkit: null[1]: error: aborting sleep to shut down
0
nbdkit: null[2]: error: aborting sleep to shut down
This commit changes the error so we only talk about shut down when the
server is actually shutting down, and use a different message in other
cases.
---
server/public.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/server/public.c b/server/public.c
index 4870e2d3..d9ed0d9c 100644
--- a/server/public.c
+++ b/server/public.c
@@ -728,7 +728,10 @@ nbdkit_nanosleep (unsigned sec, unsigned nsec)
(conn && conn->nworkers > 0 && connection_get_status ()
< 1) ||
(conn && (fds[2].revents & (POLLRDHUP | POLLHUP | POLLERR |
POLLNVAL))));
- nbdkit_error ("aborting sleep to shut down");
+ if (quit)
+ nbdkit_error ("aborting sleep because of server shut down");
+ else
+ nbdkit_error ("aborting sleep because of connection close or error");
errno = ESHUTDOWN;
return -1;
--
2.32.0
3:19 a.m.
New subject: [PATCH nbdkit 2/3] delay: Fix delay-close
See comments in the code for how this has been fixed.
This only delays clients which use NBD_CMD_DISC (libnbd
nbd_shutdown(3)). Clients which drop the connection obviously cannot
be delayed. For example:
$ nbdkit --filter=delay null delay-close=3 \
--run 'time nbdsh -u $uri -c "h.shutdown()"
time nbdsh -u $uri -c "pass"'
real 0m3.061s # Client used shutdown, was delayed
user 0m0.028s
sys 0m0.030s
real 0m0.058s # Client disconnected, was not delayed
user 0m0.029s
sys 0m0.027s
Reported-by: Ming Xie
Fixes: commit de8dcd3a34a38b088a0f9a6f8ca754702ad1f598
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1991652
---
filters/delay/nbdkit-delay-filter.pod | 13 +++++++--
filters/delay/delay.c | 42 +++++++++++++++++++--------
2 files changed, 40 insertions(+), 15 deletions(-)
diff --git a/filters/delay/nbdkit-delay-filter.pod
b/filters/delay/nbdkit-delay-filter.pod
index 11ae544b..bfc90dae 100644
--- a/filters/delay/nbdkit-delay-filter.pod
+++ b/filters/delay/nbdkit-delay-filter.pod
@@ -117,15 +117,22 @@ the plugin.
=item B<delay-open=>NNB<ms>
+(nbdkit E<ge> 1.28)
+
+Delay open (client connection) by C<SECS> seconds or C<NN>
+milliseconds.
+
=item B<delay-close=>SECS
=item B<delay-close=>NNB<ms>
(nbdkit E<ge> 1.28)
-Delay open and close operations by C<SECS> seconds or C<NN>
-milliseconds. Open corresponds to client connection. Close may not
-be visible to clients if they abruptly disconnect.
+Delay close (client disconnection) by C<SECS> seconds of C<NN>
+milliseconds. This will also cause server shutdown to be delayed.
+This only affects clients that gracefully disconnect (using
+C<NBD_CMD_DISC> / libnbd function L<nbd_shutdown(3)>). Clients that
+abruptly close the socket cannot be delayed.
=back
diff --git a/filters/delay/delay.c b/filters/delay/delay.c
index df3729a7..e0b32f46 100644
--- a/filters/delay/delay.c
+++ b/filters/delay/delay.c
@@ -39,6 +39,7 @@
#include <stdbool.h>
#include <errno.h>
#include <limits.h>
+#include <time.h>
#include <nbdkit-filter.h>
@@ -134,12 +135,6 @@ open_delay (int *err)
return delay (delay_open_ms, err);
}
-static int
-close_delay (int *err)
-{
- return delay (delay_close_ms, err);
-}
-
/* Called for each key=value passed on the command line. */
static int
delay_config (nbdkit_next_config *next, nbdkit_backend *nxdata,
@@ -253,13 +248,36 @@ delay_open (nbdkit_next_open *next, nbdkit_context *nxdata,
return NBDKIT_HANDLE_NOT_NEEDED;
}
-/* Close connection. */
-static void
-delay_close (void *handle)
+/* Close connection.
+ *
+ * We cannot call nbdkit_nanosleep here because the socket may have
+ * been closed and that function will abort and return immediately.
+ * However we want for force a sleep (even if the server is shutting
+ * down) so use regular nanosleep instead.
+ *
+ * We cannot use the .close callback because that happens after the
+ * socket has closed, thus not delaying the client. By using
+ * .finalize we can delay well-behaved clients (those that use
+ * NBD_CMD_DISC). We cannot delay clients that drop the connection.
+ */
+static int
+delay_finalize (nbdkit_next *next, void *handle)
{
- int err;
+ const unsigned ms = delay_close_ms;
- close_delay (&err);
+ if (ms > 0) {
+ struct timespec ts;
+
+ ts.tv_sec = ms / 1000;
+ ts.tv_nsec = (ms % 1000) * 1000000;
+ /* If nanosleep fails we don't really want to interrupt the chain
+ * of finalize calls through the other filters, so ignore any
+ * error here.
+ */
+ nanosleep (&ts, NULL);
+ }
+
+ return next->finalize (next);
}
/* Read data. */
@@ -340,7 +358,7 @@ static struct nbdkit_filter filter = {
.config_help = delay_config_help,
.can_fast_zero = delay_can_fast_zero,
.open = delay_open,
- .close = delay_close,
+ .finalize = delay_finalize,
.pread = delay_pread,
.pwrite = delay_pwrite,
.zero = delay_zero,
--
2.32.0
9:25 a.m.
New subject: [PATCH nbdkit 2/3] delay: Fix delay-close
On Tue, Aug 10, 2021 at 09:19:14AM +0100, Richard W.M. Jones wrote:
See comments in the code for how this has been fixed.
This only delays clients which use NBD_CMD_DISC (libnbd
nbd_shutdown(3)). Clients which drop the connection obviously cannot
be delayed. For example:
$ nbdkit --filter=delay null delay-close=3 \
--run 'time nbdsh -u $uri -c "h.shutdown()"
time nbdsh -u $uri -c "pass"'
real 0m3.061s # Client used shutdown, was delayed
user 0m0.028s
sys 0m0.030s
real 0m0.058s # Client disconnected, was not delayed
user 0m0.029s
sys 0m0.027s
Reported-by: Ming Xie
Fixes: commit de8dcd3a34a38b088a0f9a6f8ca754702ad1f598
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1991652
---
filters/delay/nbdkit-delay-filter.pod | 13 +++++++--
filters/delay/delay.c | 42 +++++++++++++++++++--------
2 files changed, 40 insertions(+), 15 deletions(-)
diff --git a/filters/delay/nbdkit-delay-filter.pod
b/filters/delay/nbdkit-delay-filter.pod
index 11ae544b..bfc90dae 100644
--- a/filters/delay/nbdkit-delay-filter.pod
+++ b/filters/delay/nbdkit-delay-filter.pod
@@ -117,15 +117,22 @@ the plugin.
=item B<delay-open=>NNB<ms>
+(nbdkit E<ge> 1.28)
+
+Delay open (client connection) by C<SECS> seconds or C<NN>
+milliseconds.
+
=item B<delay-close=>SECS
=item B<delay-close=>NNB<ms>
(nbdkit E<ge> 1.28)
-Delay open and close operations by C<SECS> seconds or C<NN>
-milliseconds. Open corresponds to client connection. Close may not
-be visible to clients if they abruptly disconnect.
+Delay close (client disconnection) by C<SECS> seconds of C<NN>
or
+milliseconds. This will also cause server shutdown to be delayed.
+This only affects clients that gracefully disconnect (using
+C<NBD_CMD_DISC> / libnbd function L<nbd_shutdown(3)>). Clients that
+abruptly close the socket cannot be delayed.
=back
+/* Close connection.
+ *
+ * We cannot call nbdkit_nanosleep here because the socket may have
+ * been closed and that function will abort and return immediately.
+ * However we want for force a sleep (even if the server is shutting
s/for /to /
+ * down) so use regular nanosleep instead.
+ *
+ * We cannot use the .close callback because that happens after the
+ * socket has closed, thus not delaying the client. By using
+ * .finalize we can delay well-behaved clients (those that use
+ * NBD_CMD_DISC). We cannot delay clients that drop the connection.
+ */
+static int
+delay_finalize (nbdkit_next *next, void *handle)
{
With those typos fixed, series looks good to me.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
3:43 p.m.
New subject: [PATCH nbdkit 2/3] delay: Fix delay-close
On Tue, Aug 10, 2021 at 09:25:28AM -0500, Eric Blake wrote:
On Tue, Aug 10, 2021 at 09:19:14AM +0100, Richard W.M. Jones wrote:
> See comments in the code for how this has been fixed.
>
> This only delays clients which use NBD_CMD_DISC (libnbd
> nbd_shutdown(3)). Clients which drop the connection obviously cannot
> be delayed. For example:
I also noticed that you added another commit 58187831a not posted on
list at the time to make delay parsing more robust. However, it uses
sscanf() to parse an integer, which is risky on untrusted data (such
as the command-line input from the user).
First, the code paths that don't use sscanf:
$ ./nbdkit -f memory 1 --filter=delay delay-read=1oops
nbdkit: error: delay-read: could not parse number: "1oops": trailing garbage
Good - this is what we want to tell the user.
$ ./nbdkit -f memory 1 --filter=delay delay-read=oops
nbdkit: error: delay-read: empty string where we expected a number
So-so - we correctly give the user an error, but the error message is
poor quality: "oops" is not an empty string (this code path did not
use sscanf)
Now for the use of sscanf:
$ ./nbdkit -f memory 1 --filter=delay delay-read=oopsms
nbdkit: error: cannot parse delay-read in milliseconds parameter: oopsms
Good - we detected a bug, although the message is worded differently
now.
$ ./nbdkit -f memory 1 --filter=delay delay-read=1oopsms
Oops - our use of sscanf didn't check for trailing garbage, and this
is behaving as delay-read=1ms.
$ ./nbdkit -fv memory 1 --filter=delay delay-read=999999999999999999999ms
Using gdb, I see that in glibc this results in the same as
delay-read=4294967295, but that behavior is unspecified by POSIX and
may result in other values on other platforms. Better would be
detecting overflow, but sscanf() cannot detect numeric overflow.
Detecting trailing garbage could be done with sscanf(value, "%ums%n",
r, &n) == 1 followed by checking that n consumed strlen(value) bytes,
but detecting overflow really needs strtol() rather than sscanf.
We have other filters and plugins that use sscanf. As long as their
inputs come from stable sources (such as scanning kernel /proc files)
or don't parse numbers, that is safe; but in general, use of sscanf to
parse user-provided data is risky.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
3:50 p.m.
New subject: [PATCH nbdkit 2/3] delay: Fix delay-close
On Thu, Aug 12, 2021 at 03:43:56PM -0500, Eric Blake wrote:
$ ./nbdkit -f memory 1 --filter=delay delay-read=1oopsms
Oops - our use of sscanf didn't check for trailing garbage, and this
is behaving as delay-read=1ms.
$ ./nbdkit -fv memory 1 --filter=delay delay-read=999999999999999999999ms
Using gdb, I see that in glibc this results in the same as
delay-read=4294967295, but that behavior is unspecified by POSIX and
may result in other values on other platforms. Better would be
detecting overflow, but sscanf() cannot detect numeric overflow.
Detecting trailing garbage could be done with sscanf(value, "%ums%n",
r, &n) == 1 followed by checking that n consumed strlen(value) bytes,
but detecting overflow really needs strtol() rather than sscanf.
We have other filters and plugins that use sscanf. As long as their
inputs come from stable sources (such as scanning kernel /proc files)
or don't parse numbers, that is safe; but in general, use of sscanf to
parse user-provided data is risky.
I'm wondering if we want to add
int nbdkit_parse_int_suffix (const char *what, const char *str, char **suff, int *r);
and friends, which sets *suff to the first unparsed byte in str on a
successful integer prefix parse.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
4:21 p.m.
New subject: [PATCH nbdkit 2/3] delay: Fix delay-close
On Thu, Aug 12, 2021 at 03:50:34PM -0500, Eric Blake wrote:
On Thu, Aug 12, 2021 at 03:43:56PM -0500, Eric Blake wrote:
>
> $ ./nbdkit -f memory 1 --filter=delay delay-read=1oopsms
>
> Oops - our use of sscanf didn't check for trailing garbage, and this
> is behaving as delay-read=1ms.
>
> $ ./nbdkit -fv memory 1 --filter=delay delay-read=999999999999999999999ms
>
> Using gdb, I see that in glibc this results in the same as
> delay-read=4294967295, but that behavior is unspecified by POSIX and
> may result in other values on other platforms. Better would be
> detecting overflow, but sscanf() cannot detect numeric overflow.
>
> Detecting trailing garbage could be done with sscanf(value, "%ums%n",
> r, &n) == 1 followed by checking that n consumed strlen(value) bytes,
> but detecting overflow really needs strtol() rather than sscanf.
>
> We have other filters and plugins that use sscanf. As long as their
> inputs come from stable sources (such as scanning kernel /proc files)
> or don't parse numbers, that is safe; but in general, use of sscanf to
> parse user-provided data is risky.
I'm wondering if we want to add
int nbdkit_parse_int_suffix (const char *what, const char *str, char **suff, int *r);
and friends, which sets *suff to the first unparsed byte in str on a
successful integer prefix parse.
string + length parsing would also work in this case.
I don't have any particular preference.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines. Supports shell scripting,
bindings from many languages. http://libguestfs.org
4:20 p.m.
New subject: [PATCH nbdkit 2/3] delay: Fix delay-close
On Thu, Aug 12, 2021 at 03:43:56PM -0500, Eric Blake wrote:
On Tue, Aug 10, 2021 at 09:25:28AM -0500, Eric Blake wrote:
> On Tue, Aug 10, 2021 at 09:19:14AM +0100, Richard W.M. Jones wrote:
> > See comments in the code for how this has been fixed.
> >
> > This only delays clients which use NBD_CMD_DISC (libnbd
> > nbd_shutdown(3)). Clients which drop the connection obviously cannot
> > be delayed. For example:
I also noticed that you added another commit 58187831a not posted on
list at the time to make delay parsing more robust. However, it uses
sscanf() to parse an integer, which is risky on untrusted data (such
as the command-line input from the user).
First, the code paths that don't use sscanf:
$ ./nbdkit -f memory 1 --filter=delay delay-read=1oops
nbdkit: error: delay-read: could not parse number: "1oops": trailing garbage
Good - this is what we want to tell the user.
$ ./nbdkit -f memory 1 --filter=delay delay-read=oops
nbdkit: error: delay-read: empty string where we expected a number
So-so - we correctly give the user an error, but the error message is
poor quality: "oops" is not an empty string (this code path did not
use sscanf)
Now for the use of sscanf:
$ ./nbdkit -f memory 1 --filter=delay delay-read=oopsms
nbdkit: error: cannot parse delay-read in milliseconds parameter: oopsms
Good - we detected a bug, although the message is worded differently
now.
$ ./nbdkit -f memory 1 --filter=delay delay-read=1oopsms
Oops - our use of sscanf didn't check for trailing garbage, and this
is behaving as delay-read=1ms.
$ ./nbdkit -fv memory 1 --filter=delay delay-read=999999999999999999999ms
Using gdb, I see that in glibc this results in the same as
delay-read=4294967295, but that behavior is unspecified by POSIX and
may result in other values on other platforms. Better would be
detecting overflow, but sscanf() cannot detect numeric overflow.
Detecting trailing garbage could be done with sscanf(value, "%ums%n",
r, &n) == 1 followed by checking that n consumed strlen(value) bytes,
but detecting overflow really needs strtol() rather than sscanf.
We have other filters and plugins that use sscanf. As long as their
inputs come from stable sources (such as scanning kernel /proc files)
or don't parse numbers, that is safe; but in general, use of sscanf to
parse user-provided data is risky.
Agreed .. (although I really wish POSIX would make sscanf more robust).
The reason for still using sscanf in the NNms case was only because to
use nbdkit_parse_* functions would either involve copying the string
(possible, but tedious), or we'd have to add alternate functions that
allow parsing string + length.
Also I hope we trust the command line. We might make that clearer in
the documentation however.
Rich.
--
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
3:19 a.m.
New subject: [PATCH nbdkit 3/3] delay: Test delay-open and delay-close
---
tests/Makefile.am | 12 +++++++--
tests/test-delay-close.sh | 54 +++++++++++++++++++++++++++++++++++++++
tests/test-delay-open.sh | 49 +++++++++++++++++++++++++++++++++++
3 files changed, 113 insertions(+), 2 deletions(-)
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 04858849..344bb697 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -1427,8 +1427,16 @@ EXTRA_DIST += \
$(NULL)
# delay filter tests.
-TESTS += test-delay-shutdown.sh
-EXTRA_DIST += test-delay-shutdown.sh
+TESTS += \
+ test-delay-close.sh \
+ test-delay-open.sh \
+ test-delay-shutdown.sh \
+ $(NULL)
+EXTRA_DIST += \
+ test-delay-close.sh \
+ test-delay-open.sh \
+ test-delay-shutdown.sh \
+ $(NULL)
LIBNBD_TESTS += test-delay
test_delay_SOURCES = test-delay.c
diff --git a/tests/test-delay-close.sh b/tests/test-delay-close.sh
new file mode 100755
index 00000000..1de305f5
--- /dev/null
+++ b/tests/test-delay-close.sh
@@ -0,0 +1,54 @@
+#!/usr/bin/env bash
+# nbdkit
+# Copyright (C) 2018-2021 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.
+
+source ./functions.sh
+set -e
+set -x
+
+requires_run
+requires_plugin null
+requires_filter delay
+requires nbdsh --version
+
+# Test delay-close with a well-behaved client.
+
+nbdkit -U - null --filter=delay delay-close=3 \
+ --run '
+start_t=$SECONDS
+nbdsh -u "$uri" -c "h.shutdown()"
+end_t=$SECONDS
+
+if [ $((end_t - start_t)) -lt 3 ]; then
+ echo "$0: delay filter failed: delay-close=3 caused delay < 3 seconds"
+ exit 1
+fi
+'
diff --git a/tests/test-delay-open.sh b/tests/test-delay-open.sh
new file mode 100755
index 00000000..2a74e44c
--- /dev/null
+++ b/tests/test-delay-open.sh
@@ -0,0 +1,49 @@
+#!/usr/bin/env bash
+# nbdkit
+# Copyright (C) 2018-2021 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.
+
+source ./functions.sh
+set -e
+set -x
+
+requires_run
+requires_plugin null
+requires_filter delay
+requires nbdinfo --version
+
+start_t=$SECONDS
+nbdkit -U - null --filter=delay delay-open=3 --run 'nbdinfo "$uri"'
+end_t=$SECONDS
+
+if [ $((end_t - start_t)) -lt 3 ]; then
+ echo "$0: delay filter failed: delay-open=3 caused delay < 3 seconds"
+ exit 1
+fi
--
2.32.0
1200
days inactive
1202
days old
7 comments
2 participants
participants (2)
-
Eric Blake -
Richard W.M. Jones