Re: [PATCH 10/11] read-cache: don't leave dangling pointer in `do_write_index()`

From: Jeff King <peff@peff.net>
Date: Oct 2, 2017, 06:20 UTC
Message-ID: <20171002062044.j7xnmmqqiz5tzguu@sigill.intra.peff.net>
In-Reply-To: <20171002061529.xaiqccwjrthc74gf@sigill.intra.peff.net>

On Mon, Oct 02, 2017 at 02:15:30AM -0400, Jeff King wrote:

Show 22 quoted lines

> I'm not sure I follow here. It seems like write_locked_index() has three
> outcomes:
> 
>   - if there was an error, the lock is rolled back
> 
>   - if we were successful and the caller asked for CLOSE_LOCK, we remain
>     locked
> 
>   - if we were successful and the caller asked for COMMIT_LOCK, we
>     commit the lock
> 
> It sounds like you are considering the first outcome a bug, but I think
> it is intentional. Without that, every caller of write_locked_index()
> would need to release the lock themselves. That's especially cumbersome
> if they called with COMMIT_LOCK, as they otherwise can assume that
> write_locked_index() has released the lock one way or another.
> 
> So I actually think that just switching to a "struct tempfile **" here
> is a reasonable solution (I'd also be fine with doing this and then
> having do_write_locked_index() rollback the lock itself on error).
> 
> Or am I missing something?

Well, one thing I was certainly missing was reading your patch 11. :)

That fixes the COMMIT_LOCK case. We are still changing the behavior of CLOSE_LOCK on error, though. The existing callers all seem to die immediately so it wouldn't matter either way, but there could in theory be new ones in topics-in-flight.

The other thing I was missing is that we are absolutely inconsistent about this "close on error". It only happens for _one_ of the error returns of do_write_index(), and the others would have left the file open and not rolled-back.

So in retrospect, I think your approach is the right direction.

-Peff