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
_______________________________________________
Libguestfs mailing list
[email protected]
https://listman.redhat.com/mailman/listinfo/libguestfs
