Re: [PATCH v2 6/6] t5556-http-auth: add test for HTTP auth hdr logic

[Derrick Stolee <derrickstolee@github.com>]

On 11/1/22 7:14 PM, Matthew John Cheetham wrote:
> On 2022-10-28 12:14, Jeff Hostetler wrote:
>> On 10/28/22 11:08 AM, Derrick Stolee wrote:

>>>> +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.

If these are copied from daemon.c, it may be worth trying
to lib-ify these data structures and code so they can be
shared across the two places. That can also come up as a
cleanup later, too.

For now, don't bother changing the names since they exist
somewhere else.
 
>> [...]
>>>> +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.

I wonder how much could be extracted from daemon.c using a
copy into a 'daemon-lib.c' with methods defined in 'daemon-lib.h'
then consumed from this file instead. Not sure it's worth the
churn to daemon.c, though.

Thanks,
-Stolee