Re: [Libguestfs] [PATCH nbdkit 2/2] curl: Fallback to GET if HEAD not supported

only superficial comments: On 6/6/23 13:22, Richard W.M. Jones wrote:

> diff --git a/tests/test-curl-head-forbidden.c b/tests/test-curl-head-forbidden.c > new file mode 100644 > index 000000000..16b1f0533 > --- /dev/null > +++ b/tests/test-curl-head-forbidden.c > @@ -0,0 +1,134 @@ > +/* nbdkit > + * Copyright Red Hat > + * > + * 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. > + */ > + > +/* Test curl plugin against a simulated webserver which responds with > + * 403 Forbidden to HEAD requests, but allows the GET method. > + */ > + > +#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 <errno.h> > +#include <sys/stat.h> > + > +#include <libnbd.h> > + > +#include "cleanup.h" > +#include "web-server.h" > + > +#include "test.h" > + > +static char buf[1024]; > + > +int > +main (int argc, char *argv[]) > +{ > + struct stat statbuf; > + const char *sockpath; > + struct nbd_handle *nbd; > + CLEANUP_FREE char *usp_param = NULL; > + int64_t size; > + > +#ifndef HAVE_CURLOPT_UNIX_SOCKET_PATH > + fprintf (stderr, "%s: curl does not support CURLOPT_UNIX_SOCKET_PATH\n", > + program_name); > + exit (77); > +#endif > + > + if (stat ("disk", &statbuf) == -1) { > + if (errno == ENOENT) { > + fprintf (stderr, "%s: test skipped because \"disk\" is missing\n", > + program_name); > + exit (77); > + } > + perror ("disk"); > + exit (EXIT_FAILURE); > + } > + > + sockpath = web_server ("disk", NULL, true); > + if (sockpath == NULL) { > + fprintf (stderr, "%s: could not start web server thread\n", program_name); > + exit (EXIT_FAILURE); > + } > + > + nbd = nbd_create (); > + if (nbd == NULL) { > + fprintf (stderr, "%s\n", nbd_get_error ()); > + exit (EXIT_FAILURE); > + } > + > + /* Start nbdkit. */ > + if (asprintf (&usp_param, "unix-socket-path=%s", sockpath) == -1) { > + perror ("asprintf"); > + exit (EXIT_FAILURE); > + } > + if (nbd_connect_command (nbd, > + (char *[]) { > + "nbdkit", "-s", "--exit-with-parent", "-v", > + "curl", > + "-D", "curl.verbose=1", > + "http://localhost/disk", > + usp_param, /* unix-socket-path=... */ > + NULL }) == -1) { > + fprintf (stderr, "%s\n", nbd_get_error ()); > + exit (EXIT_FAILURE); > + } > + > + /* Check the size is expected. */ > + size = nbd_get_size (nbd); > + if (size == -1) { > + fprintf (stderr, "%s\n", nbd_get_error ()); > + exit (EXIT_FAILURE); > + } > + if (size != statbuf.st_size) { > + fprintf (stderr, "%s: incorrect export size, " > + "expected: %" PRIu64 " actual: %" PRIi64 "\n", > + program_name, > + (uint64_t) statbuf.st_size, size); > + exit (EXIT_FAILURE); > + } > + > + /* Make a request. */ > + if (nbd_pread (nbd, buf, sizeof buf, 0, 0) == -1) { > + fprintf (stderr, "%s\n", nbd_get_error ()); > + exit (EXIT_FAILURE); > + } > + > + nbd_close (nbd); > + exit (EXIT_SUCCESS); > +} I take it this is a small customization (more precisely, lightly modified copy) of another (existent) test. The --find-copies-harder git formatting option is good for presenting new code in such terms, i.e. as a copy of another file + diffs on top. I figure the only relevant difference would be the last, "true", argument, for web_server().

> @@ -108,6 +112,7 @@ web_server (const char *filename, check_request_t _check_request) > ignore_sigpipe (); > > check_request = _check_request; > + head_fails_with_403 = _head_fails_with_403; > > /* Open the file. */ > fd = open (filename, O_RDONLY|O_CLOEXEC); > @@ -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.

> +static void > +xwrite_allow_epipe (int s, const char *buf, size_t len) > +{ > + ssize_t r; > + > + while (len > 0) { > + r = write (s, buf, len); > + if (r == -1) { > + if (errno != EPIPE) { > + perror ("write"); > + exit (EXIT_FAILURE); > + } > + else > + return; > + } exit() doesn't return, so I suggest removing the "else" (i.e., unnesting the "return").

> + buf += r; > + len -= r; > + } > +} > + > static void > xpread (char *buf, size_t count, off_t offset) > { > diff --git a/tests/web-server.h b/tests/web-server.h > index 36f0ea1e1..0961f4c5d 100644 > --- a/tests/web-server.h > +++ b/tests/web-server.h > @@ -43,6 +43,8 @@ > #ifndef NBDKIT_WEB_SERVER_H > #define NBDKIT_WEB_SERVER_H > > +#include <stdbool.h> > + > /* Starts a web server in a background thread. The web server will > * serve 'filename' (only) - the URL in requests is ignored. > * > @@ -57,10 +59,15 @@ > * The optional check_request function is called when the request is > * received (note: not in the main thread) and can be used to perform > * checks for example that particular headers were sent. > + * > + * If head_fails_with_403 == true then we simulate a server that > + * responds to GET but fails HEAD with 403 errors, see: > + * https://github.com/kubevirt/containerized-data-importer/issues/2737#issue... > */ > typedef void (*check_request_t) (const char *request); > extern const char *web_server (const char *filename, > - check_request_t check_request) > + check_request_t check_request, > + bool head_fails_with_403) > __attribute__ ((__nonnull__ (1))); > > #endif /* NBDKIT_WEB_SERVER_H */ I'd prefer the introduction of this new parameter to be a separate patch. That would isolate much the boilerplate from the relevant feature addition.

> diff --git a/.gitignore b/.gitignore > index e62c23d8b..49af3998f 100644 > --- a/.gitignore > +++ b/.gitignore > @@ -122,6 +122,7 @@ plugins/*/*.3 > /tests/test-curl > /tests/test-curl-cookie-script > /tests/test-curl-header-script > +/tests/test-curl-head-forbidden > /tests/test-data > /tests/test-delay > /tests/test-exit-with-parent Reviewed-by: Laszlo Ersek <lersek(a)redhat.com&gt;