5:46 a.m.
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