[Libguestfs] [libnbd PATCH 2/2] tests: Test server response to oversize requests
Laszlo Ersek
lersek at redhat.com
Fri Nov 11 12:17:01 UTC 2022
On 11/10/22 00:26, Eric Blake wrote:
> Improve some existing tests to cover client safety valves for oversize
> requests, and add a new test that tests libnbd behavior on server
> errors to the same sort of requests. The new test requires a parallel
> patch posted to nbdkit to teach 'nbdkit eval' how to forcefully
> disconnect the server on particarly oversize write requests, emulating
s/particarly/partially/ (I believe)
> qemu-nbd behavior.
> ---
> tests/Makefile.am | 15 +++-
> tests/errors-client-oversize.c | 18 +++-
> tests/errors-server-oversize.c | 151 ++++++++++++++++++++++++++++++++
> tests/errors-server-unaligned.c | 2 +
> .gitignore | 1 +
> 5 files changed, 185 insertions(+), 2 deletions(-)
> create mode 100644 tests/errors-server-oversize.c
>
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index 70083e5c..0b9c454e 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -177,6 +177,7 @@ check_PROGRAMS += \
> errors-multiple-disconnects \
> errors-server-invalid-offset \
> errors-client-oversize \
> + errors-server-oversize \
> errors-client-unadvertised-cmd \
> errors-server-unadvertised-cmd \
> errors-client-unaligned \
> @@ -247,6 +248,7 @@ TESTS += \
> errors-multiple-disconnects \
> errors-server-invalid-offset \
> errors-client-oversize \
> + errors-server-oversize \
> errors-client-unadvertised-cmd \
> errors-server-unadvertised-cmd \
> errors-client-unaligned \
> @@ -343,9 +345,20 @@ errors_multiple_disconnects_LDADD = $(top_builddir)/lib/libnbd.la
> errors_server_invalid_offset_SOURCES = errors-server-invalid-offset.c
> errors_server_invalid_offset_LDADD = $(top_builddir)/lib/libnbd.la
>
> -errors_client_oversize_SOURCES = errors-client-oversize.c
> +errors_client_oversize_SOURCES = \
> + errors-client-oversize.c \
> + requires.c \
> + requires.h \
> + $(NULL)
> errors_client_oversize_LDADD = $(top_builddir)/lib/libnbd.la
>
> +errors_server_oversize_SOURCES = \
> + errors-server-oversize.c \
> + requires.c \
> + requires.h \
> + $(NULL)
> +errors_server_oversize_LDADD = $(top_builddir)/lib/libnbd.la
> +
> errors_client_unadvertised_cmd_SOURCES = errors-client-unadvertised-cmd.c \
> requires.c requires.h
> errors_client_unadvertised_cmd_LDADD = $(top_builddir)/lib/libnbd.la
> diff --git a/tests/errors-client-oversize.c b/tests/errors-client-oversize.c
> index 7e5b5421..739b41e9 100644
> --- a/tests/errors-client-oversize.c
> +++ b/tests/errors-client-oversize.c
> @@ -30,6 +30,7 @@
> #include <sys/stat.h>
>
> #include <libnbd.h>
> +#include "requires.h"
>
> #define MAXSIZE 68157440 /* 65M, oversize on purpose */
>
> @@ -62,10 +63,15 @@ main (int argc, char *argv[])
> {
> struct nbd_handle *nbd;
> const char *cmd[] = {
> - "nbdkit", "-s", "-v", "--exit-with-parent", "memory", "68157440", NULL,
> + "nbdkit", "-s", "-v", "--exit-with-parent",
> + "memory", "68157440",
> + "--filter=blocksize-policy", "blocksize-maximum=32M",
> + "blocksize-error-policy=error",
> + NULL
> };
>
> progname = argv[0];
> + requires ("nbdkit --version --filter=blocksize-policy null");
>
> nbd = nbd_create ();
> if (nbd == NULL) {
> @@ -87,6 +93,7 @@ main (int argc, char *argv[])
> exit (EXIT_FAILURE);
> }
> check (ERANGE, "nbd_pread: ");
> +
> if (nbd_aio_pwrite (nbd, buf, MAXSIZE, 0,
> NBD_NULL_COMPLETION, 0) != -1) {
> fprintf (stderr, "%s: test failed: "
> @@ -96,6 +103,15 @@ main (int argc, char *argv[])
> }
> check (ERANGE, "nbd_aio_pwrite: ");
>
> + if (nbd_aio_pwrite (nbd, buf, 33*1024*1024, 0,
> + NBD_NULL_COMPLETION, 0) != -1) {
> + fprintf (stderr, "%s: test failed: "
> + "nbd_aio_pwrite did not fail with oversize request\n",
> + argv[0]);
> + exit (EXIT_FAILURE);
> + }
> + check (ERANGE, "nbd_aio_pwrite: ");
> +
> nbd_close (nbd);
> exit (EXIT_SUCCESS);
> }
> diff --git a/tests/errors-server-oversize.c b/tests/errors-server-oversize.c
> new file mode 100644
> index 00000000..e55aafc3
> --- /dev/null
> +++ b/tests/errors-server-oversize.c
> @@ -0,0 +1,151 @@
> +/* NBD client library in userspace
> + * Copyright (C) 2013-2022 Red Hat Inc.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +/* Deliberately provoke some errors and check the error messages from
> + * nbd_get_error etc look reasonable.
> + */
> +
> +#include <config.h>
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <errno.h>
> +#include <unistd.h>
> +#include <inttypes.h>
> +#include <sys/stat.h>
> +
> +#include <libnbd.h>
> +#include "requires.h"
> +
> +#define MAXSIZE 68157440 /* 65M, oversize on purpose */
> +
> +static char *progname;
> +static char buf[MAXSIZE];
> +
> +static void
> +check (int experr, const char *prefix)
> +{
> + const char *msg = nbd_get_error ();
> + int errnum = nbd_get_errno ();
> +
> + fprintf (stderr, "error: \"%s\"\n", msg);
> + fprintf (stderr, "errno: %d (%s)\n", errnum, strerror (errnum));
> + if (strncmp (msg, prefix, strlen (prefix)) != 0) {
> + fprintf (stderr, "%s: test failed: missing context prefix: %s\n",
> + progname, msg);
> + exit (EXIT_FAILURE);
> + }
> + if (errnum != experr) {
> + fprintf (stderr, "%s: test failed: "
> + "expected errno = %d (%s), but got %d\n",
> + progname, experr, strerror (experr), errnum);
> + exit (EXIT_FAILURE);
> + }
> +}
> +
> +static void
> +check_server_fail (struct nbd_handle *h, int64_t cookie,
> + const char *cmd, int experr)
> +{
> + int r;
> +
> + if (cookie == -1) {
> + fprintf (stderr, "%s: test failed: %s not sent to server\n",
> + progname, cmd);
> + exit (EXIT_FAILURE);
> + }
> +
> + while ((r = nbd_aio_command_completed (h, cookie)) == 0) {
> + if (nbd_poll (h, -1) == -1) {
> + fprintf (stderr, "%s: test failed: poll failed while awaiting %s: %s\n",
> + progname, cmd, nbd_get_error ());
> + exit (EXIT_FAILURE);
> + }
> + }
> +
> + if (r != -1) {
> + fprintf (stderr, "%s: test failed: %s did not fail at server\n",
> + progname, cmd);
> + exit (EXIT_FAILURE);
> + }
> + check (experr, "nbd_aio_command_completed: ");
> +}
> +
> +int
> +main (int argc, char *argv[])
> +{
> + struct nbd_handle *nbd;
> + const char *cmd[] = {
> + "nbdkit", "-s", "-v", "--exit-with-parent", "eval",
> + "get_size= echo 68157440",
> + "block_size= echo 1 512 16M",
> + "pread= echo EIO >&2; exit 1",
> + "pwrite= if test $3 -gt $((32*1024*1024)); then\n"
> + " exit 6\n" /* Hard disconnect */
> + " elif test $3 -gt $((16*1024*1024)); then\n"
> + " echo EOVERFLOW >&2; exit 1\n"
> + " fi\n"
> + " cat >/dev/null",
> + NULL,
> + };
> + uint32_t strict;
> +
> + progname = argv[0];
> + requires ("nbdkit --dump-plugin eval | grep ^max_known_status=");
> +
> + nbd = nbd_create ();
> + if (nbd == NULL) {
> + fprintf (stderr, "%s\n", nbd_get_error ());
> + exit (EXIT_FAILURE);
> + }
> +
> + /* Connect to the server. */
> + if (nbd_connect_command (nbd, (char **) cmd) == -1) {
> + fprintf (stderr, "%s: %s\n", argv[0], nbd_get_error ());
> + exit (EXIT_FAILURE);
> + }
> +
> + /* Check the advertised max sizes. */
> + printf ("server block size maximum: %" PRIi64 "\n",
> + nbd_get_block_size (nbd, LIBNBD_SIZE_MAXIMUM));
> + printf ("libnbd payload size maximum: %" PRIi64 "\n",
> + nbd_get_block_size (nbd, LIBNBD_SIZE_PAYLOAD));
> +
> + /* Disable client-side safety check */
> + strict = nbd_get_strict_mode (nbd) & ~LIBNBD_STRICT_PAYLOAD;
The current macros are (without the first patch applied):
#define LIBNBD_STRICT_COMMANDS 0x01
#define LIBNBD_STRICT_FLAGS 0x02
#define LIBNBD_STRICT_BOUNDS 0x04
#define LIBNBD_STRICT_ZERO_SIZE 0x08
#define LIBNBD_STRICT_ALIGN 0x10
#define LIBNBD_STRICT_MASK 0x1f
The 0x prefix *permits* the compiler to consider unsigned integer types
when picking the size (type) for the integer constant, but the prefix
does not *force* an unsigned type. And, because these values are really
small, they all have type "int". In turn, when we apply the bitwise
negation operator, we flip the sign bit. Not necessarily a problem on
the platforms we care about, but not nice.
There are two ways to remove this concern. One is the following:
strict = nbd_get_strict_mode (nbd) & ~(sometype)LIBNBD_STRICT_PAYLOAD;
where "sometype" is an unsigned integer type having at least the
conversion rank of "int" (so that it would not be promoted to "int"),
*and* large enough to accommodate all uint32_t values. C99 permits
"unsigned int" to be just 16 bits, so using "unsigned int" for
"sometype" is only safe if we are sure that on our platforms, "unsigned
int" will always be able to represent "uint32_t". Otherwise we need a
larger type, such as "unsigned long" (which is guaranteed by C99 even to
hold a uint32_t).
The other way is more generic: change the generator to output all these
constants with a "u" suffix, such as:
#define LIBNBD_STRICT_COMMANDS 0x01u
#define LIBNBD_STRICT_FLAGS 0x02u
#define LIBNBD_STRICT_BOUNDS 0x04u
#define LIBNBD_STRICT_ZERO_SIZE 0x08u
#define LIBNBD_STRICT_ALIGN 0x10u
#define LIBNBD_STRICT_MASK 0x1fu
This is better because the compiler will pick "unsigned int" for them
(once we reach huge values, the compiler will pick "unsigned long" then
"unsigned long long"), the bitwise negation will be applied to unsigned
integers, and then the bitwise "and" will be between a uint32_t and some
unsigned integer type (and at that one that's not going to be promoted
to "int", having a rank already at least as large).
Then in case "uint32_t" fits in an "int", it's going to be promoted to
"int", and then the bit-and is between int and unsigned int --> the int
is converted to unsigned int, done. Otherwise, we have two unsigned
integers, the one with the smaller rank is converted to the type of the
other.
Long story short: probably totally harmless in practice on our platforms
(the code happens to do what's expected due to two's complement
representation of signed integers), but it's general hygiene to avoid
bit manipulations on signed integers (some of those happen to be
undefined behavior even). It's good to be aware IMO that the generator
currently produces LIBNBD_STRICT_* values that have type "int", and not
signed int.
Feel free to ignore this comment if you think it's needlessly cautious.
(I think it'd be best to keep the patch as-is, and then modify the
generator in a separate patch.)
...We could also consider the C99 standard macro UINT32_C(...):
#define LIBNBD_STRICT_COMMANDS UINT32_C(0x01)
#define LIBNBD_STRICT_FLAGS UINT32_C(0x02)
#define LIBNBD_STRICT_BOUNDS UINT32_C(0x04)
#define LIBNBD_STRICT_ZERO_SIZE UINT32_C(0x08)
#define LIBNBD_STRICT_ALIGN UINT32_C(0x10)
#define LIBNBD_STRICT_MASK UINT32_C(0x1f)
which creates values of uint_least32_t -- but that type can (in theory)
still be promoted to "int", if "int" is large enough. So I think the "u"
suffix is best.
> + if (nbd_set_strict_mode (nbd, strict) == -1) {
> + fprintf (stderr, "%s\n", nbd_get_error ());
> + exit (EXIT_FAILURE);
> + }
> +
> + /* Handle graceful server rejection of oversize request */
> + check_server_fail (nbd,
> + nbd_aio_pwrite (nbd, buf, 17*1024*1024, 0,
> + NBD_NULL_COMPLETION, 0),
> + "17M nbd_aio_pwrite", EINVAL);
> +
> + /* Handle abrupt server rejection of oversize request */
> + check_server_fail (nbd,
> + nbd_aio_pwrite (nbd, buf, 33*1024*1024, 0,
> + NBD_NULL_COMPLETION, 0),
> + "33M nbd_aio_pwrite", ENOTCONN);
> +
> + nbd_close (nbd);
> + exit (EXIT_SUCCESS);
> +}
> diff --git a/tests/errors-server-unaligned.c b/tests/errors-server-unaligned.c
> index ac374976..6dbd6e29 100644
> --- a/tests/errors-server-unaligned.c
> +++ b/tests/errors-server-unaligned.c
> @@ -123,6 +123,8 @@ main (int argc, char *argv[])
> nbd_get_block_size (nbd, LIBNBD_SIZE_PREFERRED));
> printf ("server block size maximum: %" PRIi64 "\n",
> nbd_get_block_size (nbd, LIBNBD_SIZE_MAXIMUM));
> + printf ("libnbd payload size maximum: %" PRIi64 "\n",
> + nbd_get_block_size (nbd, LIBNBD_SIZE_PAYLOAD));
> fflush (stdout);
>
> /* Send an unaligned read, server-side */
> diff --git a/.gitignore b/.gitignore
> index fe929d6d..f4273713 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -215,6 +215,7 @@ Makefile.in
> /tests/errors-poll-no-fd
> /tests/errors-pread-structured
> /tests/errors-server-invalid-offset
> +/tests/errors-server-oversize
> /tests/errors-server-unadvertised-cmd
> /tests/errors-server-unaligned
> /tests/errors-server-unknown-flags
>
Looks good to me otherwise:
Reviewed-by: Laszlo Ersek <lersek at redhat.com>
Laszlo
More information about the Libguestfs
mailing list