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