Re: [PATCH v2 63/94] builtin/apply: make apply_all_patches() return -1 on error

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Jun 8, 2016 at 12:37 PM, Christian Couder
<christian.couder@xxxxxxxxx> wrote:
> On Mon, May 16, 2016 at 5:44 AM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
>> On Wed, May 11, 2016 at 9:17 AM, Christian Couder
>> <christian.couder@xxxxxxxxx> wrote:
>>>         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.)
>
> You are right, it is leaking newfd if write_locked_index() fails.
> The solution to that is to call `rollback_lock_file(state->lock_file)`
> and the following patch was supposed to do that:
>
> [PATCH v2 82/94] apply: roll back index lock file in case of error
>
> but it would do that only if `state->newfd >= 0` so we should set
> state->newfd to -1 only if write_locked_index() succeeds.
>
> I will fix this.
>
> I am also going to add a comment to this patch saying that this patch
> needs a following patch to call rollback_lock_file(state->lock_file)
> in case of errors.
>
> Or if you prefer, I can squash the patch that call
> rollback_lock_file(state->lock_file) in case of errors into this
> patch.

Squashing may indeed be preferable over leaving it in a "broken" state
until the next patch, though I haven't thought too hard about it.
Alternately, can the two patches somehow be swapped?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]