On 10/15/21 16:17, Richard W.M. Jones wrote:
Test the new nbdkit-retry-request-filter with the curl plugin. I
have
modified our test webserver so it provides a magical "/mirror" path
which acts somewhat like a download mirror with occasional broken
redirects.
Fixes:
https://bugzilla.redhat.com/show_bug.cgi?id=2013000
---
tests/Makefile.am | 24 ++++
tests/test-retry-request-mirror.c | 130 ++++++++++++++++++++
tests/web-server.c | 197 +++++++++++++++++++++++++++---
.gitignore | 1 +
4 files changed, 337 insertions(+), 15 deletions(-)
I've mostly only skimmed this, for it being a test; it looks OK to me.
I have some superficial comments / questions:
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 9a1485a6c..b8a3a654c 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -1701,6 +1701,30 @@ EXTRA_DIST += \
test-retry-request.sh \
$(NULL)
+LIBNBD_TESTS += test-retry-request-mirror
+
+test_retry_request_mirror_SOURCES = \
+ test-retry-request-mirror.c \
+ web-server.c \
+ web-server.h \
+ test.h \
+ $(NULL)
+test_retry_request_mirror_CPPFLAGS = \
+ -I$(top_srcdir)/common/utils
+test_retry_request_mirror_CFLAGS = \
+ $(WARNINGS_CFLAGS) \
+ $(PTHREAD_CFLAGS) \
+ $(LIBNBD_CFLAGS) \
+ $(NULL)
+test_retry_request_mirror_LDFLAGS = \
+ $(top_builddir)/common/utils/libutils.la \
+ $(PTHREAD_LIBS) \
+ $(NULL)
+test_retry_request_mirror_LDADD = \
+ libtest.la \
+ $(LIBNBD_LIBS) \
+ $(NULL)
+
# swab filter test.
TESTS += \
test-swab-8.sh \
diff --git a/tests/test-retry-request-mirror.c b/tests/test-retry-request-mirror.c
new file mode 100644
index 000000000..861e2e0e5
--- /dev/null
+++ b/tests/test-retry-request-mirror.c
@@ -0,0 +1,130 @@
+/* nbdkit
+ * Copyright (C) 2013-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.
+ */
+
+/* This is a test of recovering from broken redirects to a mirror
+ * service. See the following bug for background:
+ *
https://bugzilla.redhat.com/show_bug.cgi?id=2013000
+ */
+
+#include <config.h>
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <stdbool.h>
+#include <stdint.h>
+#include <inttypes.h>
+#include <string.h>
+#include <unistd.h>
+
+#include <libnbd.h>
+
+#include "cleanup.h"
+#include "web-server.h"
+
+#include "test.h"
+
+int
+main (int argc, char *argv[])
+{
+ const char *sockpath;
+ struct nbd_handle *nbd;
+ CLEANUP_FREE char *usp_param = NULL;
+ char buf[512];
+ int state, i;
+
+#ifndef HAVE_CURLOPT_UNIX_SOCKET_PATH
+ fprintf (stderr, "%s: curl does not support CURLOPT_UNIX_SOCKET_PATH\n",
+ argv[0]);
+ exit (77);
+#endif
+
+ sockpath = web_server ("disk", NULL);
+ if (sockpath == NULL) {
+ fprintf (stderr, "%s: could not start web server thread\n", argv[0]);
+ exit (EXIT_FAILURE);
+ }
+
+ /* Start nbdkit. */
+ if (asprintf (&usp_param, "unix-socket-path=%s", sockpath) == -1) {
+ perror ("asprintf");
+ exit (EXIT_FAILURE);
Is exit() safe here (with the web server running in another thread)?
Furthermore, should we do anything for (e.g.) deleting the pathname of
the unix domain socket? (I don't think it matters much; I'm asking this
is mostly for my own education.) And then that would apply more or less
to every error exit after this point.
+ }
+ if (test_start_nbdkit ("--filter=retry-request",
+ "curl", usp_param,
"http://localhost/mirror",
+ "retry-request-delay=1",
+ NULL) == -1)
+ exit (EXIT_FAILURE);
+
+ nbd = nbd_create ();
+ if (nbd == NULL) {
+ nbd_error:
+ fprintf (stderr, "%s: %s\n", argv[0], nbd_get_error ());
+ exit (EXIT_FAILURE);
+ }
Can we put this block of code at the very end of the function, and here,
just say "goto nbd_error"?
Because right now we have gotos jumping backward, which I find awkward
even for (otherwise justified) "algorithmic reasons", and very awkward
for a usual error path.
... Either way:
Acked-by: Laszlo Ersek <lersek(a)redhat.com>
Thanks!
Laszlo
+
+ if (nbd_connect_unix (nbd, sock /* NBD socket */) == -1)
+ goto nbd_error;
+
+ /* The way the test works is we fetch the magic "/mirror" path (see
+ * web-server.c). This redirects to /mirror1, /mirror2, /mirror3
+ * round robin on each request. /mirror1 returns all 1's, /mirror2
+ * returns all 2's, and /mirror3 returns a 404 error. The 404 error
+ * should be transparently skipped by the filter, so we should see
+ * alternating 1's and 2's buffers.
+ */
+ if (nbd_pread (nbd, buf, sizeof buf, 0, 0) == -1)
+ goto nbd_error;
+
+ if (buf[0] == 1 && buf[1] == 1 && buf[511] == 1)
+ state = 2;
+ else if (buf[0] == 2 && buf[1] == 2 && buf[511] == 2)
+ state = 1;
+ else {
+ fprintf (stderr, "%s: unexpected start state\n", argv[0]);
+ exit (EXIT_FAILURE);
+ }
+
+ for (i = 0; i < 10; ++i) {
+ if (nbd_pread (nbd, buf, sizeof buf, 0, 0) == -1)
+ goto nbd_error;
+ if (buf[0] != state || buf[1] != state || buf[511] != state) {
+ fprintf (stderr, "%s: unexpected state\n", argv[0]);
+ exit (EXIT_FAILURE);
+ }
+ state++;
+ if (state == 3)
+ state = 1;
+ }
+
+ nbd_close (nbd);
+ exit (EXIT_SUCCESS);
+}
diff --git a/tests/web-server.c b/tests/web-server.c
index ac44d16b2..07a2ec398 100644
--- a/tests/web-server.c
+++ b/tests/web-server.c
@@ -57,6 +57,8 @@
#define SOCK_CLOEXEC 0
#endif
+enum method { HEAD, GET };
+
static char tmpdir[] = "/tmp/wsXXXXXX";
static char sockpath[] = "............./sock";
static int listen_sock = -1;
@@ -67,7 +69,12 @@ static check_request_t check_request;
static void *start_web_server (void *arg);
static void handle_requests (int s);
-static void handle_request (int s, bool headers_only);
+static void handle_file_request (int s, enum method method);
+static void handle_mirror_redirect_request (int s);
+static void handle_mirror_data_request (int s, enum method method, char byte);
+static void send_404_not_found (int s);
+static void send_405_method_not_allowed (int s);
+static void send_500_internal_server_error (int s);
static void xwrite (int s, const char *buf, size_t len);
static void xpread (char *buf, size_t count, off_t offset);
@@ -177,12 +184,15 @@ start_web_server (void *arg)
static void
handle_requests (int s)
{
- size_t r, n, sz;
bool eof = false;
fprintf (stderr, "web server: accepted connection\n");
while (!eof) {
+ size_t r, n, sz;
+ enum method method;
+ char path[128];
+
/* Read request until we see "\r\n\r\n" (end of headers) or EOF. */
n = 0;
for (;;) {
@@ -214,30 +224,73 @@ handle_requests (int s)
/* Call the optional user function to check the request. */
if (check_request) check_request (request);
- /* HEAD or GET request? */
- if (strncmp (request, "HEAD ", 5) == 0)
- handle_request (s, true);
- else if (strncmp (request, "GET ", 4) == 0)
- handle_request (s, false);
+ /* Get the method and path fields from the first line. */
+ if (strncmp (request, "HEAD ", 5) == 0) {
+ method = HEAD;
+ n = strcspn (&request[5], " \n\t");
+ if (n >= sizeof path) {
+ send_500_internal_server_error (s);
+ eof = true;
+ break;
+ }
+ memcpy (path, &request[5], n);
+ path[n] = '\0';
+ }
+ else if (strncmp (request, "GET ", 4) == 0) {
+ method = GET;
+ n = strcspn (&request[4], " \n\t");
+ if (n >= sizeof path) {
+ send_500_internal_server_error (s);
+ eof = true;
+ break;
+ }
+ memcpy (path, &request[4], n);
+ path[n] = '\0';
+ }
else {
- /* Return 405 Method Not Allowed. */
- const char response[] =
- "HTTP/1.1 405 Method Not Allowed\r\n"
- "Content-Length: 0\r\n"
- "Connection: close\r\n"
- "\r\n";
- xwrite (s, response, strlen (response));
+ send_405_method_not_allowed (s);
eof = true;
break;
}
+
+ fprintf (stderr, "web server: requested path: %s\n", path);
+
+ /* For testing retry-request + curl:
+ * /mirror redirects round-robin to /mirror1, /mirror2, /mirror3
+ * /mirror1 returns a file of \x01 bytes
+ * /mirror2 returns a file of \x02 bytes
+ * /mirror3 returns 404 errors
+ * Anything else returns a 500 error
+ */
+ if (strcmp (path, "/mirror") == 0)
+ handle_mirror_redirect_request (s);
+ else if (strcmp (path, "/mirror1") == 0)
+ handle_mirror_data_request (s, method, 1);
+ else if (strcmp (path, "/mirror2") == 0)
+ handle_mirror_data_request (s, method, 2);
+ else if (strcmp (path, "/mirror3") == 0) {
+ send_404_not_found (s);
+ eof = true;
+ }
+ else if (strncmp (path, "/mirror", 7) == 0) {
+ send_500_internal_server_error (s);
+ eof = true;
+ }
+
+ /* Otherwise it's a regular file request. 'path' is ignored, we
+ * only serve a single file passed to web_server().
+ */
+ else
+ handle_file_request (s, method);
}
close (s);
}
static void
-handle_request (int s, bool headers_only)
+handle_file_request (int s, enum method method)
{
+ const bool headers_only = method == HEAD;
uint64_t offset, length, end;
const char *p;
const char response1_ok[] = "HTTP/1.1 200 OK\r\n";
@@ -295,6 +348,120 @@ handle_request (int s, bool headers_only)
free (data);
}
+/* Request for /mirror */
+static void
+handle_mirror_redirect_request (int s)
+{
+ static char rr = '1'; /* round robin '1', '2',
'3' */
+ /* Note we send 302 (temporary redirect), same as Fedora's mirrorservice. */
+ const char found[] = "HTTP/1.1 302 Found\r\nContent-Length: 0\r\n";
+ char location[] = "Location: /mirrorX\r\n";
+ const char eol[] = "\r\n";
+
+ location[17] = rr;
+ rr++;
+ if (rr == '4')
+ rr = '1';
+
+ xwrite (s, found, strlen (found));
+ xwrite (s, location, strlen (location));
+ xwrite (s, eol, strlen (eol));
+}
+
+static void
+handle_mirror_data_request (int s, enum method method, char byte)
+{
+ const bool headers_only = method == HEAD;
+ uint64_t offset, length, end;
+ const char *p;
+ const char response1_ok[] = "HTTP/1.1 200 OK\r\n";
+ const char response1_partial[] = "HTTP/1.1 206 Partial Content\r\n";
+ const char response2[] =
+ "Accept-rANGES: bytes\r\n" /* See RHBZ#1837337 */
+ "Connection: keep-alive\r\n"
+ "Content-Type: application/octet-stream\r\n";
+ char response3[64];
+ const char response4[] = "\r\n";
+ char *data;
+
+ /* If there's no Range request header then send the full size as the
+ * content-length.
+ */
+ p = strcasestr (request, "\r\nRange: bytes=");
+ if (p == NULL) {
+ offset = 0;
+ length = statbuf.st_size;
+ xwrite (s, response1_ok, strlen (response1_ok));
+ }
+ else {
+ p += 15;
+ if (sscanf (p, "%" SCNu64 "-%" SCNu64, &offset, &end) !=
2) {
+ fprintf (stderr, "web server: could not parse "
+ "range request from curl client\n");
+ exit (EXIT_FAILURE);
+ }
+ /* Unclear but "Range: bytes=0-4" means bytes 0-3. '4' is the
+ * byte beyond the end of the range.
+ */
+ length = end - offset;
+ xwrite (s, response1_partial, strlen (response1_partial));
+ }
+
+ xwrite (s, response2, strlen (response2));
+ snprintf (response3, sizeof response3,
+ "Content-Length: %" PRIu64 "\r\n", length);
+ xwrite (s, response3, strlen (response3));
+ xwrite (s, response4, strlen (response4));
+
+ if (headers_only)
+ return;
+
+ /* Send the file content. */
+ data = malloc (length);
+ if (data == NULL) {
+ perror ("malloc");
+ exit (EXIT_FAILURE);
+ }
+
+ memset (data, byte, length);
+ xwrite (s, data, length);
+
+ free (data);
+}
+
+static void
+send_404_not_found (int s)
+{
+ const char response[] =
+ "HTTP/1.1 404 Not Found\r\n"
+ "Content-Length: 0\r\n"
+ "Connection: close\r\n"
+ "\r\n";
+ xwrite (s, response, strlen (response));
+}
+
+static void
+send_405_method_not_allowed (int s)
+{
+ const char response[] =
+ "HTTP/1.1 405 Method Not Allowed\r\n"
+ "Content-Length: 0\r\n"
+ "Connection: close\r\n"
+ "\r\n";
+ xwrite (s, response, strlen (response));
+}
+
+static void
+send_500_internal_server_error (int s)
+{
+ const char response[] =
+ "HTTP/1.1 500 Internal Server Error\r\n"
+ "Content-Length: 0\r\n"
+ "Connection: close\r\n"
+ "\r\n";
+ xwrite (s, response, strlen (response));
+}
+
static void
xwrite (int s, const char *buf, size_t len)
{
diff --git a/.gitignore b/.gitignore
index 847b72dd6..e7b07d510 100644
--- a/.gitignore
+++ b/.gitignore
@@ -155,6 +155,7 @@ plugins/*/*.3
/tests/test-python
/tests/test-random
/tests/test-readahead
+/tests/test-retry-request-mirror
/tests/test-ruby
/tests/test-S3/boto3/__pycache__/
/tests/test-shell