Re: [PATCH v2 05/25] sequencer: allow the sequencer to take custody of malloc()ed data

["Jakub Narębski" <jnareb@gmail.com>]

W dniu 06.10.2016 o 21:23, Junio C Hamano pisze:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
>> If you prefer to accept such sloppy work, I will change it of course,
>> feeling dirty that it has my name on it.
> 
> I do want neither leaks nor sloppyness, and I thought that was
> understood by everybody, hence I thought the last round made it
> clear that _entrust() must not exist.

Also, the *_entrust() mechanism in v1 was quite sequencer-specific,
rather than be a generic mechanism.

> 
> Let's try again.
> 
> We manage lifetime of a field in a structure in one of three ways in
> our codebase [*1*].
> 
>  * A field can always point at a borrowed piece of memory that the
>    destruction of the containing structure or re-assignment to the
>    field do not have to free the current value of the field.  This
>    is a minority, simply because it is rare for a field to be always
>    able to borrow from others.  The destruction of the containing
>    structure or re-assignment to the field needs to do nothing
>    special.
> 
>  * A field can own the piece of memory that it points at.  The
>    destruction of the containing structure or re-assignment to the
>    field needs to free the current value of the field [*2*].  A
>    field that can be assigned a value from the configuration (which
>    is typically xstrdup()'ed) is an example of such a field, and if
>    a command line option also can assign to the field, we'd need to
>    xstrdup() it, even though we know we could borrow from argv[],
>    because cleaning-up needs to be able to free(3) it.
> 
>  * A field can sometimes own and sometimes borrow the memory, and it
>    is accompanied by another field to tell which case it is, so that
>    cleaning-up can tell when it needs to be free(3)d.  This is a
>    minority case, and we generally avoid it especially in modern
>    code for small allocation, as it makes the lifetime rule more
>    complex than it is worth.

Great writeup!

> The _entrust() thing may have been an excellent fourth option if it
> were cost-free.  xstrdup() that the second approach necessitates for
> literal strings (like part of argv[]) would become unnecessary and
> we can treat as if all the fields in a structure were all borrowing
> the memory from elsewhere (in fact, _entrust() creates the missing
> owners of pieces of memory that need to be freed later); essentially
> it turns a "mixed ownership" case into the first approach.
> 
> But it is not cost-free.  The mechanism needs to allocate memory to
> become the missing owner and keep track of which pieces of memory
> need to be freed later, and the resulting code does not become any
> easier to follow.  The programmer still needs to know which ones to
> _entrust() just like the programmer in the model #2 needs to make
> sure xstrdup() is done for appropriate pieces of memory--the only
> difference is that an _entrust() programmer needs to mark the pieces
> of memory to be freed, while a #2 programmer needs to xstrdup() the
> pieces of memory that are being "borrowed".
> 
> So let's not add the fourth way to our codebase.  "mixed ownership"
> case should be turned into "field owns the memory", i.e. approach #2.

On the other hand the _entrust() mechanism might be a good solution
if the amount of memory was large, for example order of magnitude more
than what would be needed to keep ownership info *and* borrowing would
not be possible for some reason.

But the _entrust() mechanism reminds me on one hand side of memory
debuggers like dmalloc, Electric Fence (eFence), or valgrind; combined
a bit with garbage collection.  Namely, when we are in a library call,
record all allocations (perhaps by redirecting alloc functions), then
free those resources at the exit of library call.
 
> 
> [Footnotes]
> 
> *1* It is normal for different fields in the same structure follow
>     different lifetime rules.
> 
> *2* Unless leaks are allowed, that is.  I think we have instances
>     where "git cmd --opt=A --opt=B" allocates and stores a new piece
>     of memory that is computed based on A and store it to a field,
>     and then overwrites the field with another allocation of a value
>     based on B without freeing old value, saying "the caller can
>     avoid passing the same thing twice, and such a leak is miniscule
>     anyway".  That is not nice, and we've been reducing them over
>     time.
>