Re: [PATCH] config: use a static lock_file struct

On Wed, Aug 30, 2017 at 09:07:53AM +0200, Michael Haggerty wrote:

quoted

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
about lifetimes.

-Peff