Re: [PATCH v3 00/25] Lockfile correctness and refactoring
On 04/15/2014 08:40 PM, Torsten Bögershausen wrote:
> refs.c:
> int close_ref(struct ref_lock *lock)
> {
> if (close_lock_file(lock->lk))
> return -1;
> lock->lock_fd = -1;
> return 0;
> }
>
> When the close() fails, fd is still >= 0, even if the file is closed.
>
> Could it be written like this ?
>
> int close_ref(struct ref_lock *lock)
> {
> return close_lock_file(lock->lk);
> }
It seem to me it would be better to set lock->lock_fd to -1 regardless
of whether close_lock_file() succeeds. Or maybe this field is not even
needed, and instead lock->lk->fd should be used.
This is a bit beyond the scope of this patch series, but I will put it
on my todo list.
> =
> lockfile.c, line 49
> * - filename holds the filename of the lockfile
>
> I think we have a strbuf here? (which is a good thing),
> should we write:
> * - strbuf filename holds the filename of the lockfile
> --
> (and at some places filename[0] is mentioned,
> should that be filename.buf[0] ?)
I think it is OK to speak of a strbuf as "holding" a filename (what else
would that construct mean?
But you are correct that the comments shouldn't speak of filename[0]
anymore. I will fix it.
> =
> int commit_lock_file(struct lock_file *lk)
> {
> static struct strbuf result_file = STRBUF_INIT;
> int err;
>
> if (lk->fd >= 0 && close_lock_file(lk))
> return -1;
> ##What happens if close() fails and close_lock_file() returns != 0?
> ##Is the lk now in a good shaped state?
> I think the file which had been open by lk->fd is in an unkown state,
> and should not be used for anything.
> When I remember it right, an error on close() can mean "the file could not
> be written and closed as expected, it can be truncated or corrupted.
> This can happen on a network file system like NFS, or probably even other FS.
> For me the failing of close() means I smell a need for a rollback.
Yes, this is a good catch. I think a rollback should definitely be done
in this case. I will fix it.
In fact, I'm wondering whether it would be appropriate for
close_lock_file() itself to do a rollback if close() fails. I guess I
will first have to audit callers to make sure that they don't try to use
lock_file::filename after a failed close_lock_file() (e.g., for
generating an error message).
> Please treat my comments more than questions rather than answers,
> thanks for an interesting reading
Thanks for your helpful comments!
Michael
--
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 00/25] Lockfile correctness and refactoring
refs.c:
int close_ref(struct ref_lock *lock)
{
if (close_lock_file(lock->lk))
return -1;
lock->lock_fd = -1;
return 0;
}
When the close() fails, fd is still >= 0, even if the file is closed.
Could it be written like this ?
int close_ref(struct ref_lock *lock)
{
return close_lock_file(lock->lk);
}
=
lockfile.c, line 49
* - filename holds the filename of the lockfile
I think we have a strbuf here? (which is a good thing),
should we write:
* - strbuf filename holds the filename of the lockfile
--
(and at some places filename[0] is mentioned,
should that be filename.buf[0] ?)
=
int commit_lock_file(struct lock_file *lk)
{
static struct strbuf result_file = STRBUF_INIT;
int err;
if (lk->fd >= 0 && close_lock_file(lk))
return -1;
##What happens if close() fails and close_lock_file() returns != 0?
##Is the lk now in a good shaped state?
I think the file which had been open by lk->fd is in an unkown state,
and should not be used for anything.
When I remember it right, an error on close() can mean "the file could not
be written and closed as expected, it can be truncated or corrupted.
This can happen on a network file system like NFS, or probably even other FS.
For me the failing of close() means I smell a need for a rollback.
if (!lk->active)
die("BUG: attempt to commit unlocked object");
/* remove ".lock": */
strbuf_add(&result_file, lk->filename.buf,
lk->filename.len - LOCK_SUFFIX_LEN);
err = rename(lk->filename.buf, result_file.buf);
strbuf_reset(&result_file);
if (err)
return -1;
lk->active = 0;
strbuf_reset(&lk->filename);
return 0;
}
Please treat my comments more than questions rather than answers,
thanks for an interesting reading
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 00/25] Lockfile correctness and refactoring
Round v3. Thanks to Johannes Sixt and Peff for feedback on v2. This version addresses all issues raised for v1 [1] and v2 [2]. Changes since v2: * Instead of keeping track of whether a lock_file object is active via a new bit in a flags bitmask, store it in a separate volatile sig_atomic_t struct member. This makes it a little less undefined when this field is accessed by the signal handler. * Add some other "volatile" qualifiers to values used by the signal handler. * Define constants LOCK_SUFFIX and LOCK_SUFFIX_LEN in cache.h and use them both inside and outside lockfile.c. (In v2, only LOCK_SUFFIX_LEN was defined and it was only used within lockfile.c, because the other potential users in refs.c were rewritten anyway. But that rewriting is no longer included in the patch series, so it makes sense to define these constants as part of the public lockfile API.) * Swap order of first two patches because the documentation lists unable_to_lock_die() under its new name. * die() (instead of NOP) if commit_lock_file() is called for an unlocked lock_file object. * Rebase to current master (there were no conflicts). [1] http://thread.gmane.org/gmane.comp.version-control.git/245609 [2] http://thread.gmane.org/gmane.comp.version-control.git/245801 Michael Haggerty (25): unable_to_lock_die(): rename function from unable_to_lock_index_die() api-lockfile: expand the documentation rollback_lock_file(): do not clear filename redundantly rollback_lock_file(): set fd to -1 lockfile: unlock file if lockfile permissions cannot be adjusted hold_lock_file_for_append(): release lock on errors lock_file(): always add lock_file object to lock_file_list lockfile.c: document the various states of lock_file objects cache.h: define constants LOCK_SUFFIX and LOCK_SUFFIX_LEN delete_ref_loose(): don't muck around in the lock_file's filename prepare_index(): declare return value to be (const char *) write_packed_entry_fn(): convert cb_data into a (const int *) lock_file(): exit early if lockfile cannot be opened remove_lock_file(): call rollback_lock_file() commit_lock_file(): inline temporary variable commit_lock_file(): die() if called for unlocked lockfile object lockfile: avoid transitory invalid states struct lock_file: declare some fields volatile try_merge_strategy(): remove redundant lock_file allocation try_merge_strategy(): use a statically-allocated lock_file object commit_lock_file(): use a strbuf to manage temporary space Change lock_file::filename into a strbuf resolve_symlink(): use a strbuf for internal scratch space resolve_symlink(): take a strbuf parameter trim_last_path_elm(): replace last_path_elm() Documentation/technical/api-lockfile.txt | 40 - builtin/commit.c | 16 +- builtin/merge.c | 15 +- builtin/reflog.c | 2 +- builtin/update-index.c | 2 +- cache.h | 15 +- config.c | 6 +- lockfile.c | 275 ++- refs.c | 29 ++-- shallow.c| 6 +- 10 files changed, 247 insertions(+), 159 deletions(-) -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
