On 2022-10-28 12:14, Jeff Hostetler wrote:
>
>
> 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...
>
Only --anonymous-allowed, --auth and --auth-token are added over git-daemon.
>
>>> +
>>> +/* 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 take from git-daemon and the test-gvfs-protocol helper from microsoft/git
fork, but then also delete lots of not required pieces too just as much as
I have added. Copying git-daemon.c, to then delete, and then add feels like
lots of noise.
>> 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.
Indeed. These functions are untouched from daemon.c. I do plan to split
this mega-patch up however in to a single 'add the boilerplate' based on
git-daemon patch, then add the extra pieces like HTTP request parsing and
the auth pieces in a v3.
> [...]
>>> +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.
Will update in v3 to drop the const.
> [...]
>> 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
>
>
