Re: [PATCH v6 02/39] api-lockfile: revise and expand the documentation

[Michael Haggerty <mhagger@alum.mit.edu>]

On 09/30/2014 07:39 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> I was being repetitive because I didn't want the docs to depend on the
>> user remembering what the "bss" section is (which, technically, is also
>> not part of the C standard). I think a better way would be to just not
>> mention "bss section" at all and reword the rest. Maybe something like
>>
>>   The caller:
>>
>>   * Allocates a variable `struct lock_file lock`, initialized to zeros.
>>     Because the `lock_file` structure is used in an `atexit(3)` handler,
>>     its storage has to remain valid throughout the life of the program;
>>     e.g., it should be a static variable or space allocated on the heap.
>>
>> Better?
> 
> Somewhat.  I like that you droped "BSS", though.
> 
> 	Allocates a 'struct lock_file' either as a static variable
> 	or on the heap, initialized to zeros.  Once you use the
> 	structure to call hold_lock_file_* family of functions, it
> 	belongs to the lockfile subsystem and its storage must
> 	remain valid throughout the life of the program (i.e. you
> 	cannot use an on-stack variable to hold this structure).

OK, I'll use that.

>>> It feels a bit conflicting between "must not be freed or ALTERED"
>>> and "it may be REUSED".  Drop "or altered" to reduce confusion?  And
>>> this repeats the third sentence I suggested to remove from the first
>>> paragraph (above 'see below' refers here).
>>
>> The purpose of "or altered" is to make sure that users don't imagine
>> that they should memset() the structure to zeros or something before
>> reusing it (especially since the "caller" instructions earlier say that
>> the object should be "initialized to zeros").
>>
>> Would it help if I change
>>
>>     s/altered/altered by the caller/
>>
>> ?
> 
> The fundamental rule is that callers outside the lockfile system must
> not touch individual members of "struct lock_file" that is "live".
> They must not free, they must not alter, they must not do anything
> other than calling the lockfile API functions.
> 
> While it is natural that the readers would understand such a rule
> must be followed for a lockfile that is in either the initialized,
> locked, closed-but-not-committed state, I agree that it is not just
> possible but likely that people misunderstand and think that once a
> lockfile is committed or rolled back it no longer has to follow that
> rule.  We would want to make sure readers do not fall into such a
> misunderstanding.
> 
> I dunno.  Your "if you memset it to NULs, you will break the linked
> list of the lock and the whole lockfile system and the element
> cannot even be reused." may be the most important thing you may have
> wanted to say, but it is not conveyed by that change at all, at
> least to me.
> 
> A small voice in the back of my skull keeps telling me that a rule
> that is hard to document and explain is a rule that does not make
> sense.  Is it possible to allow commit and rollback to safely remove
> the structure from the atexit(3) list (aka "no longer owned by the
> lockfile subsystem")?

Certainly it is possible. One would probably make lock_file an opaque
data structure, always allocated on the heap within the lockfile
subsystem, and maybe with a free list to reduce memory allocations.

But it would be a lot of work and I don't see a commensurate payoff.
There are not *that* many callers of the lockfile API.

>>> Is it allowed to tell the name of this lock_file to other people and
>>> let them read from it?  The answer is yes but it is not obvious from
>>> this description.
>>>
>>> Also mention how the above interact with reopen_lock_file() here?
>>
>> I'll take a stab at it, though TBH I haven't really studied the use case
>> for reopen_lock_file(). You might be better able to provide insight into
>> this topic.
> 
> You would want to be able to do this:
> 
>  - hold a lock on a file (say, the index);
> 
>  - update it in preparation to commit it;
> 
>  - write it out and make sure the contents is on the disk platter,
>    in preparation to call another program and allow it (and nobody
>    else) to inspect the contents you wrote, while still holding the
>    lock yourself.  In our set of API functions, close_lock_file is
>    what lets us do this.
> 
>  - further update it, write it out and commit.  We need to read it
>    first, open(2) it to write, write(2), and commit_lock_file().
> 
> The set of API functions you described in the document, there is no
> way to say "I already have a lock on that file, just let me open(2)
> for writing because I have further updates" and that is where reopen
> comes in.

Thanks for the clarification. My upcoming reroll will document
reopen_lock_file().

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu