On 2014-09-06 09.50, Michael Haggerty wrote:
> Sorry for the long delay since v3. This version mostly cleans up a
> couple more places where the lockfile object was left in an
> ill-defined state.
No problem with the delay.
The most important question is if we do the lk->active handling right.
Set it to false as seen as possible, and to true as late as possible,
then die() cleanly.
So the ->acive handling looks (more or less, please see below) and
deserves another critical review, may be.
Instead of commenting each patch, I collected a mixture of small questions
and possible suggestions into a diff file.
diff --git a/lockfile.c b/lockfile.c
index e54d260..7f27ea2 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -18,6 +18,10 @@
* Usually, if $FILENAME is a symlink, then it is resolved, and the
* file ultimately pointed to is the one that is locked and later
* replaced. However, if LOCK_NODEREF is used, then $FILENAME
+LOCK_NODEREF can be read either as
+LOCK_NODE_REF or LOCK_NO_DEREF
+should we change it ?
+
* itself is locked and later replaced, even if it is a symlink.
*
* 2. Write the new file contents to the lockfile.
@@ -46,9 +50,18 @@
* state:
* - the lockfile exists
* - active is set
- * - filename holds the filename of the lockfile
+ * - filename holds the filename of the lockfile in a strbuf
* - fd holds a file descriptor open for writing to the lockfile
* - owner holds the PID of the process that locked the file
+question: Why do we need the PID here ?
+Do we open a lock file and do a fork() ?
+And if yes, the child gets a new PID, what happens when the
+child gets a signal ?
+Who "owns" the lockfile, the parent, the child, both ?
+The child has access to all data, the fd is open and can be used,
+why do we not allow a rollback, when the child dies ?
+
+
*
* - Locked, lockfile closed (after close_lock_file()). Same as the
* previous state, except that the lockfile is closed and fd is -1.
@@ -57,7 +70,7 @@
* rollback_lock_file(), or a failed attempt to lock). In this
* state:
* - active is unset
- * - filename is the empty string (usually, though there are
+ * - filename is an empty string buffer (usually, though there are
* transitory states in which this condition doesn't hold)
* - fd is -1
* - the object is left registered in the lock_file_list, and
@@ -68,7 +81,7 @@
static struct lock_file *volatile lock_file_list;
-static void remove_lock_file(void)
+static void remove_lock_files(void)
{
pid_t me = getpid();
@@ -79,9 +92,9 @@ static void remove_lock_file(void)
}
}
-static void remove_lock_file_on_signal(int signo)
+static void remove_lock_files_on_signal(int signo)
{
- remove_lock_file();
+ remove_lock_files();
sigchain_pop(signo);
raise(signo);
}
@@ -93,7 +106,7 @@ static void remove_lock_file_on_signal(int signo)
* "/", if any). If path is empty or the root directory ("/"), set
* path to the empty string.
*/
-static void trim_last_path_elm(struct strbuf *path)
+static void trim_last_path_elem(struct strbuf *path)
{
int i = path->len;
@@ -143,7 +156,7 @@ static void resolve_symlink(struct strbuf *path)
* link is a relative path, so replace the
* last element of p with it.
*/
- trim_last_path_elm(path);
+ trim_last_path_elem(path);
strbuf_addbuf(path, &link);
}
@@ -153,13 +166,16 @@ static void resolve_symlink(struct strbuf *path)
/* Make sure errno contains a meaningful value on error */
static int lock_file(struct lock_file *lk, const char *path, int flags)
{
+ struct stat st;
+ int mode = 0666;
if (!lock_file_list) {
/* One-time initialization */
- sigchain_push_common(remove_lock_file_on_signal);
- atexit(remove_lock_file);
+ sigchain_push_common(remove_lock_files_on_signal);
+ atexit(remove_lock_files);
}
- assert(!lk->active);
+ if (lk->active)
+ die("lk->active %s", path);
if (!lk->on_list) {
/* Initialize *lk and add it to lock_file_list: */
@@ -167,16 +183,25 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
lk->active = 0;
lk->owner = 0;
lk->on_list = 1;
- strbuf_init(&lk->filename, 0);
+ strbuf_init(&lk->filename, strlen(path) + LOCK_SUFFIX_LEN);
lk->next = lock_file_list;
lock_file_list = lk;
}
+ strbuf_reset(&lk->filename); /* Better to be save */
strbuf_addstr(&lk->filename, path);
if (!(flags & LOCK_NODEREF))
resolve_symlink(&lk->filename);
strbuf_addstr(&lk->filename, LOCK_SUFFIX);
- lk->fd = open(lk->filename.buf, O_RDWR | O_CREAT | O_EXCL, 0666);
+ /*
+ * adjust_shared_perm() will widen permissions if needed,
+ * otherwise keep permissions restrictive
+ *
+ */
+ if (!stat(path, &st))
+ mode = st.st_mode & 07777;
+
+ lk->fd = open(lk->filename.buf, O_RDWR | O_CREAT | O_EXCL, mode);
if (lk->fd < 0) {
strbuf_reset(&lk->filename);
return -1;
@@ -268,7 +293,7 @@ int close_lock_file(struct lock_file *lk)
return close(fd);
}
-int reopen_lock_file(struct lock_file *lk)
+int reopen_lock_file_UNUSED_CAN_IT_BE_REMOVED(struct lock_file *lk)
{
if (0 <= lk->fd)
die(_("BUG: reopen a lockfile that is still open"));
@@ -283,7 +308,7 @@ int commit_lock_file_to(struct lock_file *lk, const char *path)
int save_errno;
if (!lk->active)
- die("BUG: attempt to commit unlocked object");
+ die("BUG: attempt to commit unlocked object %s", path);
if (lk->fd >= 0 && close_lock_file(lk))
goto rollback;
@@ -325,10 +350,12 @@ void rollback_lock_file(struct lock_file *lk)
{
if (!lk->active)
return;
+ lk->active = 0; /* We are going to de-activate,
+ so active is no longer valid already here ? */
if (lk->fd >= 0)
close_lock_file(lk);
unlink_or_warn(lk->filename.buf);
- lk->active = 0;
+ //lk->active = 0;
strbuf_reset(&lk->filename);
}
--
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