Re: [PATCH 09/27] upload-pack: make check_non_tip() clean things up error
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 10, 2016, 20:25 UTC
- 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"?
Show 7 quoted lines
> 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"?
Show 54 quoted lines
> 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;