On 09/30/2014 07:39 PM, Junio C Hamano wrote: > Michael Haggerty <mhagger@xxxxxxxxxxxx> 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@xxxxxxxxxxxx -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html