On Tue, Jun 06, 2023 at 06:09:09PM +0200, Laszlo Ersek wrote:
> 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().
It's indeed a copy of test-curl-header-script.c. --find-copies-harder
thinks it's:
--- a/tests/test-curl-cookie-script.c
+++ b/tests/test-curl-head-forbidden.c
which makes sense as test-curl-header-script.c and
test-curl-cookie-script.c were from a common source originally.
> > @@ -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.
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.
> > +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").
OK.
> > + 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#issuecomment-1577786849
> > */
> > 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.
OK.
> > 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 <ler...@redhat.com>
Thanks,
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines. Supports shell scripting,
bindings from many languages. http://libguestfs.org
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
