Michael Haggerty [off-list ref] 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).
quoted
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")?
quoted
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.
On 09/30/2014 07:39 PM, Junio C Hamano wrote:
Michael Haggerty [off-list ref] writes:
quoted
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.
quoted
quoted
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.
quoted
quoted
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