From: Eric Sunshine Date: Mon, 16 May 2016 03:44:43 GMT Subject: Re: [PATCH v2 63/94] builtin/apply: make apply_all_patches() return -1 on error Message-ID: In-Reply-To: <20160511131745.2914-64-chriscool@tuxfamily.org> On Wed, May 11, 2016 at 9:17 AM, Christian Couder wrote: > To finish libifying the apply functionality, apply_all_patches() should not > die() or exit() in case of error, but return -1. > > While doing that we must take care that file descriptors are properly closed > and, if needed, reset a sensible value. > > Signed-off-by: Christian Couder > --- > diff --git a/builtin/apply.c b/builtin/apply.c > @@ -4613,9 +4613,10 @@ static int apply_all_patches(struct apply_state *state, > } > > if (state->update_index) { > - if (write_locked_index(&the_index, state->lock_file, COMMIT_LOCK)) > - die(_("Unable to write new index file")); > + res = write_locked_index(&the_index, state->lock_file, COMMIT_LOCK); > state->newfd = -1; Does write_locked_index() unconditionally close the file descriptor even when an error occurs? If not, then isn't this potentially leaking 'newfd'? (My very cursory read of write_locked_index() seems to reveal that the file descriptor may indeed remain open upon index write failure.) > + if (res) > + return error(_("Unable to write new index file")); > } > > return !!errs;