From: Junio C Hamano <gitster@pobox.com>
Date: Fri, 10 Jun 2016 20:25:45 GMT
Subject: Re: [PATCH 09/27] upload-pack: make check_non_tip() clean things up error
Message-ID: <xmqqwplweqpi.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160610122714.3341-10-pclouds@gmail.com>

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> Subject: Re: [PATCH 09/27] upload-pack: make check_non_tip() clean things up error

"clean things up on error"?

> On error check_non_tip() will die and not closing file descriptors is no
> big deal. The next patch will split the majority of this function out
> for reuse in other cases, where die() may not be the only outcome. Same
> story for popping SIGPIPE out of the signal chain. So let's make sure we
> clean things up properly first.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

Makes me wonder if you can push into sigchain before the first
appearance of "goto error" so that in the error handling codepath
you can do sigchain_pop(), without adding sigchain_pop() before all
the "goto error"?

>  upload-pack.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/upload-pack.c b/upload-pack.c
> index 60f2e5e..1e8b025 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -494,8 +494,10 @@ static void check_non_tip(void)
>  		if (!is_our_ref(o))
>  			continue;
>  		memcpy(namebuf + 1, oid_to_hex(&o->oid), GIT_SHA1_HEXSZ);
> -		if (write_in_full(cmd.in, namebuf, 42) < 0)
> +		if (write_in_full(cmd.in, namebuf, 42) < 0) {
> +			sigchain_pop(SIGPIPE);
>  			goto error;
> +		}
>  	}
>  	namebuf[40] = '\n';
>  	for (i = 0; i < want_obj.nr; i++) {
> @@ -503,10 +505,13 @@ static void check_non_tip(void)
>  		if (is_our_ref(o))
>  			continue;
>  		memcpy(namebuf, oid_to_hex(&o->oid), GIT_SHA1_HEXSZ);
> -		if (write_in_full(cmd.in, namebuf, 41) < 0)
> +		if (write_in_full(cmd.in, namebuf, 41) < 0) {
> +			sigchain_pop(SIGPIPE);
>  			goto error;
> +		}
>  	}
>  	close(cmd.in);
> +	cmd.in = -1;
>  
>  	sigchain_pop(SIGPIPE);
>  
> @@ -518,6 +523,7 @@ static void check_non_tip(void)
>  	if (i)
>  		goto error;
>  	close(cmd.out);
> +	cmd.out = -1;
>  
>  	/*
>  	 * rev-list may have died by encountering a bad commit
> @@ -531,6 +537,11 @@ static void check_non_tip(void)
>  	return;
>  
>  error:
> +	if (cmd.in >= 0)
> +		close(cmd.in);
> +	if (cmd.out >= 0)
> +		close(cmd.out);
> +
>  	/* Pick one of them (we know there at least is one) */
>  	for (i = 0; i < want_obj.nr; i++) {
>  		o = want_obj.objects[i].item;