On 6/6/23 18:38, Richard W.M. Jones wrote:
On Tue, Jun 06, 2023 at 06:09:09PM +0200, Laszlo Ersek wrote:
On 6/6/23 13:22, Richard W.M. Jones wrote:
@@ -250,6 +255,11 @@ handle_requests (int s) } memcpy (path, &request[5], n); path[n] = '\0'; + if (head_fails_with_403) { + send_403_forbidden (s); + eof = true; + break; + }
I'm not a fan of the pre-existent pattern where we both set "eof = true" and break out of the loop. It would have to be cleaned up first, in a different patch, but I'm not sure if we care enough.
It's a test :-) However what do you suggest? Both statements are necessary because we have to break out of the inner for(;;) loop, run a tiny bit of common cleanup code (just a fprintf), and then break from the while(!eof) loop.
That argument is valid in case we are inside the inner ("for") loop, but here we are not (handle_requests() [tests/web-server.c]), as far as I can tell. The context with memcpy (path, &request[5], n); path[n] = '\0'; is outside (after) the "for" loop. The pre-patch code is misleading in this regard (it uses the same pattern of both eof=true plus break in three places). One option would be to use "eof=true" plus *continue*, rather than "break". That would: - prevent the rest of the *outer* (while) loop's body from running, just as the break does, - set "eof=true" consistently with the outer loop's design; IOW, rely on "eof" solely for exiting the outer loop, plus, after the "while" loop, "eof" would be true. Consider the following (pre-patch) cleanup (git diff --function-context):
diff --git a/tests/web-server.c b/tests/web-server.c index 9b37326c0540..7f9a15760a22 100644 --- a/tests/web-server.c +++ b/tests/web-server.c @@ -190,107 +190,107 @@ static void handle_requests (int s) { 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 (;;) { if (n >= sizeof request - 1 /* allow one byte for \0 */) { fprintf (stderr, "web server: request too long\n"); exit (EXIT_FAILURE); } sz = sizeof request - n - 1; r = read (s, &request[n], sz); if (r == -1) { perror ("read"); exit (EXIT_FAILURE); } if (r == 0) { eof = true; break; } n += r; request[n] = '\0'; if (strstr (request, "\r\n\r\n")) break; }
if (n == 0) continue;
fprintf (stderr, "web server: request:\n%s", request);
/* Call the optional user function to check the request. */ if (check_request) check_request (request);
/* 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; + continue; } 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; + continue; } memcpy (path, &request[4], n); path[n] = '\0'; } else { send_405_method_not_allowed (s); eof = true; - break; + continue; }
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);
fprintf (stderr, "web server: completed request\n"); }
fprintf (stderr, "web server: closing socket\n"); close (s); }
This would prevent the "web server: completed request" message from being logged, but that's already the case with: - the one continue statement we have in place anyway, - all the break statements the apove patch replaces. Those too prevent the "web server: completed request" message, so the new "continue" statements make no difference in that regard. If we wanted to print "web server: completed request" in any case, then we have at least two options. The ugly one (git diff --function-context --ignore-space-change):
diff --git a/tests/web-server.c b/tests/web-server.c index 9b37326c0540..7f7bd4424cd8 100644 --- a/tests/web-server.c +++ b/tests/web-server.c @@ -190,107 +190,109 @@ static void handle_requests (int s) { 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 (;;) { if (n >= sizeof request - 1 /* allow one byte for \0 */) { fprintf (stderr, "web server: request too long\n"); exit (EXIT_FAILURE); } sz = sizeof request - n - 1; r = read (s, &request[n], sz); if (r == -1) { perror ("read"); exit (EXIT_FAILURE); } if (r == 0) { eof = true; break; } n += r; request[n] = '\0'; if (strstr (request, "\r\n\r\n")) break; }
if (n == 0) continue;
fprintf (stderr, "web server: request:\n%s", request);
/* Call the optional user function to check the request. */ if (check_request) check_request (request);
/* 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; } + else { 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; } + else { memcpy (path, &request[4], n); path[n] = '\0'; } + } else { send_405_method_not_allowed (s); eof = true; - break; }
+ if (!eof) { 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); - + } fprintf (stderr, "web server: completed request\n"); }
fprintf (stderr, "web server: closing socket\n"); close (s); }
And the nice one (git diff --function-context --ignore-space-change):
diff --git a/tests/web-server.c b/tests/web-server.c index 9b37326c0540..3ea785f749af 100644 --- a/tests/web-server.c +++ b/tests/web-server.c @@ -186,111 +186,121 @@ start_web_server (void *arg) } }
-static void -handle_requests (int s) +/* Returns "true" iff we should continue serving requests. */ +static bool +handle_request (int s) { - bool eof = false; - - fprintf (stderr, "web server: accepted connection\n"); - - while (!eof) { - size_t r, n, sz; enum method method; + size_t n; char path[128];
- /* Read request until we see "\r\n\r\n" (end of headers) or EOF. */ - n = 0; - for (;;) { - if (n >= sizeof request - 1 /* allow one byte for \0 */) { - fprintf (stderr, "web server: request too long\n"); - exit (EXIT_FAILURE); - } - sz = sizeof request - n - 1; - r = read (s, &request[n], sz); - if (r == -1) { - perror ("read"); - exit (EXIT_FAILURE); - } - if (r == 0) { - eof = true; - break; - } - n += r; - request[n] = '\0'; - if (strstr (request, "\r\n\r\n")) - break; - } - - if (n == 0) - continue; - - fprintf (stderr, "web server: request:\n%s", request); - /* Call the optional user function to check the request. */ if (check_request) check_request (request);
/* 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; + return false; } 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; + return false; } memcpy (path, &request[4], n); path[n] = '\0'; } else { send_405_method_not_allowed (s); - eof = true; - break; + return false; }
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) + if (strcmp (path, "/mirror") == 0) { handle_mirror_redirect_request (s); - else if (strcmp (path, "/mirror1") == 0) + return true; + } + if (strcmp (path, "/mirror1") == 0) { handle_mirror_data_request (s, method, 1); - else if (strcmp (path, "/mirror2") == 0) + return true; + } + if (strcmp (path, "/mirror2") == 0) { handle_mirror_data_request (s, method, 2); - else if (strcmp (path, "/mirror3") == 0) { + return true; + } + if (strcmp (path, "/mirror3") == 0) { send_404_not_found (s); - eof = true; + return false; } - else if (strncmp (path, "/mirror", 7) == 0) { + if (strncmp (path, "/mirror", 7) == 0) { send_500_internal_server_error (s); - eof = true; + return false; }
/* 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); + return true; +}
+static void +handle_requests (int s) +{ + bool eof = false; + + fprintf (stderr, "web server: accepted connection\n"); + + while (!eof) { + size_t r, n, sz; + + /* Read request until we see "\r\n\r\n" (end of headers) or EOF. */ + n = 0; + for (;;) { + if (n >= sizeof request - 1 /* allow one byte for \0 */) { + fprintf (stderr, "web server: request too long\n"); + exit (EXIT_FAILURE); + } + sz = sizeof request - n - 1; + r = read (s, &request[n], sz); + if (r == -1) { + perror ("read"); + exit (EXIT_FAILURE); + } + if (r == 0) { + eof = true; + break; + } + n += r; + request[n] = '\0'; + if (strstr (request, "\r\n\r\n")) + break; + } + + if (n == 0) + continue; + + fprintf (stderr, "web server: request:\n%s", request); + eof = !handle_request (s); fprintf (stderr, "web server: completed request\n"); }
fprintf (stderr, "web server: closing socket\n"); close (s); }
(I've built and tested the last one.) But this is probably too much churn for a test. Laszlo