On 10/28/22 11:08 AM, Derrick Stolee wrote:
}
diff --git a/t/helper/test-http-server.c b/t/helper/test-http-server.c
@@ -0,0 +1,1134 @@
+#include "config.h"
+#include "run-command.h"
+#include "strbuf.h"
+#include "string-list.h"
+#include "trace2.h"
+#include "version.h"
+#include "dir.h"
+#include "date.h"
+
+#define TR2_CAT "test-http-server"
+
+static const char *pid_file;
+static int verbose;
+static int reuseaddr;
+
+static const char test_http_auth_usage[] =
+"http-server [--verbose]\n"
+" [--timeout=<n>] [--init-timeout=<n>] [--max-connections=<n>]\n"
+" [--reuseaddr] [--pid-file=<file>]\n"
+" [--listen=<host_or_ipaddr>]* [--port=<n>]\n"
+" [--anonymous-allowed]\n"
+" [--auth=<scheme>[:<params>] [--auth-token=<scheme>:<token>]]*\n"
+;
These are a lot of options to implement all at once. They are probably
simple enough, but depending on the implementation and tests, it might
be helpful to split this patch into smaller ones that introduce these
options along with the tests that exercise each. That will help
verify that they are being tested properly instead of needing to track
back and forth across the patch for each one.
how many of these options were inherited from test-gvfs-protocol or
from upstream git-daemon? If most came from git-daemon, it's probably
easier to see that this was a cut-n-paste from it if it comes over in
one commit, since all of the OPT_ processing, usage(), and static global
state vars will come over together I would think -- rather than to build
up the arg parsing bit by bit. More on this in a minute...
+
+/* Timeout, and initial timeout */
+static unsigned int timeout;
+static unsigned int init_timeout;
+
+static void logreport(const char *label, const char *err, va_list params)
+{
+ struct strbuf msg = STRBUF_INIT;
+
+ strbuf_addf(&msg, "[%"PRIuMAX"] %s: ", (uintmax_t)getpid(), label);
+ strbuf_vaddf(&msg, err, params);
+ strbuf_addch(&msg, '\n');
+
+ fwrite(msg.buf, sizeof(char), msg.len, stderr);
+ fflush(stderr);
+
+ strbuf_release(&msg);
+}
+
+__attribute__((format (printf, 1, 2)))
+static void logerror(const char *err, ...)
+{
+ va_list params;
+ va_start(params, err);
+ logreport("error", err, params);
+ va_end(params);
+}
+
+__attribute__((format (printf, 1, 2)))
+static void loginfo(const char *err, ...)
+{
+ va_list params;
+ if (!verbose)
+ return;
+ va_start(params, err);
+ logreport("info", err, params);
+ va_end(params);
+}
...Maybe it would be easier to see/diff this large new test server
if we copied `daemon.c` into this source file in 1 commit and then
converted it to what you have now in 1 commit -- so that only new
code shows up here. For example, all of the above logreport, logerror,
and loginfo routines would show up as new in the copy commit, but not
in the edit commit. However, that may lead to too much noise when
you actually get into the meat of the auth changes, maybe.
I wonder how much of this we need or is just a nice thing. I would
err on the side of making things as simple as possible, but being
able to debug this test server may be important based on your
experience.
i'd vote to keep it.
[...]
+static void kill_some_child(void)
+static void check_dead_children(void)
These technically sound methods have unfortunate names.
Using something like "connection" over "child" might
alleviate some of the horror. (I initially wanted to
suggest "subprocess" but you compare live_children to
max_connections in the next method, so connection seemed
appropriate.)
These names were inherited from `daemon.c` IIRC. I wouldn't change
them since it'll just introduce noise when diffing. Especially,
if we do the copy commit first.
[...]
+static struct strvec cld_argv = STRVEC_INIT;
+static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)
+{
+ struct child_process cld = CHILD_PROCESS_INIT;
+
+ if (max_connections && live_children >= max_connections) {
+ kill_some_child();
+ sleep(1); /* give it some time to die */
+ check_dead_children();
+ if (live_children >= max_connections) {
+ close(incoming);
+ logerror("Too many children, dropping connection");
+ return;
+ }
+ }
Do we anticipate exercising concurrent requests in our
tests? Perhaps it's not worth putting a cap on the
connection count so we can keep the test helpers simple.
again, this code was inherited from `daemon.c`, so we could leave it.
[...]
+ mod = xmalloc(sizeof(struct auth_module));
+ mod->scheme = xstrdup(p[0]->buf);
+ mod->challenge_params = p[1] ? xstrdup(p[1]->buf) : NULL;
Here, you xstrdup() into a 'const char *', but you are really
passing ownership so it shouldn't be conts.
There is a strbuf_detach() that will let you steal the buffer from the
strbuf if that would help.
[...]
This was a lot to read, and the interesting bits are all mixed in
with the http server code, which is less interesting to what we
are trying to accomplish. It would be beneficial to split this
into one or two patches before we actually introduce the tests.
agreed. it is big, but it does make sense. perhaps doing the
copy daemon.c commit and then see how this commit diffs from it
would make it more manageable. (not sure, but worth a try.)
[...]
From what I read, I don't think there is much to change in
the end result of the code, but it definitely was hard to read
the important things when surrounded by many lines of
boilerplate.
agreed. i think the end result is good.
Thanks
Jeff
