Re: [PATCH] config: use a static lock_file struct
- Date: Sat, 2 Sep 2017 04:44:51 -0400
- From: Jeff King <peff@xxxxxxxx>
- Subject: Re: [PATCH] config: use a static lock_file struct
On Wed, Aug 30, 2017 at 09:07:53AM +0200, Michael Haggerty wrote:
> > So it's probably safer to just let tempfile.c handle the whole lifetime,
> > and have it put all live tempfiles on the heap, and free them when
> > they're deactivated. That means our creation signature becomes more
> > like:
> > struct tempfile *create_tempfile(const char *path);
> > and delete_tempfile() actually calls free() on it (though we'd probably
> > want to skip the free() from a signal handler for the usual reasons).
> I agree that the latter would be a nice, and relatively safe, design. It
> would involve some fairly intrusive changes to client code, though.
> I think it would be possible to implement the new API while leaving the
> old one intact, to avoid having to rewrite all clients at once, and
> potentially to allow clients to avoid a malloc if they already have a
> convenient place to embed a `struct tempfile` (except that now they'd be
> able to free it when done). For example, `create_tempfile(tempfile,
> path)` and its friends could accept NULL as the first argument, in which
> case it would malloc a `struct tempfile` itself, and mark it as being
> owned by the tempfile module. Such objects would be freed when
> deactivated. But if the caller passes in a non-NULL `tempfile` argument,
> the old behavior would be retained.
There are actually very few callers of create_tempfile (I count two).
Everybody just uses lock_file(). So I think we could get away with just
modifying the internals of the lock struct to hold a pointer, and then
teaching commit_lock_file() et al to reset it to NULL after performing
an operation that frees the tempfile. We could even modify
rename_tempfile() and friends to take a pointer-to-pointer and NULL it
itself. That would make it harder to get wrong.
In the long term I don't think there's a huge value to being able to
place a "struct tempfile" inside another struct, as opposed to holding a
pointer. It saves a malloc, but at the cost of opening up confusion