Re: [PATCH] Close transport helper on protocol error

[Junio C Hamano <gitster@pobox.com>]

thibault.jamet@gmail.com writes:

> Subject: Re: [PATCH] Close transport helper on protocol error

Perhaps

    Subject: transport: close helper on protocol error

> +static int disconnect_helper(struct transport *transport);
> +
> +static int disconnect_helper_data(struct helper_data *data);

Even after reading it twice, disconnect_helper_data() does not ring
the "this is to disconnect the helper process, based on what is
contained in a helper_data instance" bell, which you wanted to ring.
It sounds like it is trying to disconnect "helper_data" from
something unsaid.

I think the root cause of this awkwardness is because this split of
the function into two is suboptimal.  There is only one existing
caller of disconnect_helper() and it passes transport->data (and the
"data" is of type helper_data).  As it is a file-scope static
function, why not just change the type of the parameter from the
whole transport to just helper_data, without introducing the new
function to hold the bulk of the original code?

> +static int release_helper(struct transport *transport);
> +
>  static struct child_process *get_helper(struct transport *transport)
>  {
>  	struct helper_data *data = transport->data;
> @@ -155,8 +161,10 @@ static struct child_process *get_helper(struct transport *transport)
>  	while (1) {
>  		const char *capname, *arg;
>  		int mandatory = 0;
> -		if (recvline(data, &buf))
> +		if (recvline(data, &buf)){
> +			release_helper(transport);
>  			exit(128);
> +		}

This, together with other exit(128) we see in this patch now have
release_helepr() in front of it, which is in line with what the log
message claims that the patch does.

I however wonder if we want to do a bit more, perhaps with atexit().
I am not hinting-suggesting to do so (as you said, if the init
process ought to take care of the zombies, the patch under review
might already be unneeded, and atexit() makes things even worse),
but having trouble to convince that this patch stops at the right
place.

Thanks.