Re: [PATCH v3 00/25] Lockfile correctness and refactoring

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

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
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/