Re: [PATCH 05/22] lockfile: unlock file if lockfile permissions cannot be adjusted

2014-04-06 Thread Michael Haggerty 
On 04/02/2014 08:47 AM, Torsten Bögershausen wrote:
 []
 
 diff --git a/lockfile.c b/lockfile.c
 index c1af65b..1928e5e 100644
 --- a/lockfile.c
 +++ b/lockfile.c
 @@ -148,9 +148,11 @@ static int lock_file(struct lock_file *lk, const
 char *path, int flags)
  lock_file_list = lk;
  lk-on_list = 1;
  }
 -if (adjust_shared_perm(lk-filename))
 -return error(cannot fix permission bits on %s,
 - lk-filename);
 +if (adjust_shared_perm(lk-filename)) {
 +error(cannot fix permission bits on %s, lk-filename);
 +rollback_lock_file(lk);
 +return -1;
 
 Would it make sense to change the order of rollback() and error()?
 Make the rollback first (and as early as possible) and whine then?

Thanks for the feedback.  It is a nice idea.  But given that
rollback_lock_file() erases the filename field, making the change you
suggest would require us to make a copy before calling
rollback_lock_file().  The only advantage would be that we hold the lock
file for a moment less, right?  Since this code path is only reached
when the repository has screwy permissions anyway, the next caller will
probably fail too.  So the extra code complication doesn't seem worth it
to me.

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 05/22] lockfile: unlock file if lockfile permissions cannot be adjusted

2014-04-02 Thread Torsten Bögershausen 

[]

diff --git a/lockfile.c b/lockfile.c
index c1af65b..1928e5e 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -148,9 +148,11 @@ static int lock_file(struct lock_file *lk, const char 
*path, int flags)
lock_file_list = lk;
lk-on_list = 1;
}
-   if (adjust_shared_perm(lk-filename))
-   return error(cannot fix permission bits on %s,
-lk-filename);
+   if (adjust_shared_perm(lk-filename)) {
+   error(cannot fix permission bits on %s, lk-filename);
+   rollback_lock_file(lk);
+   return -1;

Would it make sense to change the order of rollback() and error()?
Make the rollback first (and as early as possible) and whine then?

--
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 05/22] lockfile: unlock file if lockfile permissions cannot be adjusted

2014-04-01 Thread Jeff King 
On Tue, Apr 01, 2014 at 05:58:13PM +0200, Michael Haggerty wrote:

 If the call to adjust_shared_perm() fails, lock_file returns -1, which
 to the caller looks like any other failure to lock the file.  So in
 this case, roll back the lockfile before returning so that the lock
 file is deleted immediately and the lockfile object is left in a
 predictable state that (namely, unlocked).  Previously, the lockfile
 was retained until process cleanup in this situation.

Another nice find. I wondered if we could test this, but I think it
would be hard to trigger. The obvious reason for adjust_shared_perm to
fail is that you do not have permissions on the file, but by definition
you just created it. So I doubt this ever comes up in practice short of
weird races (somebody dropping the x bit from an intermediate
directory between the open() and chmod() or something).

-Peff
--
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 05/22] lockfile: unlock file if lockfile permissions cannot be adjusted

2014-04-01 Thread Jeff King 
On Tue, Apr 01, 2014 at 04:02:42PM -0400, Jeff King wrote:

 On Tue, Apr 01, 2014 at 05:58:13PM +0200, Michael Haggerty wrote:
 
  If the call to adjust_shared_perm() fails, lock_file returns -1, which
  to the caller looks like any other failure to lock the file.  So in
  this case, roll back the lockfile before returning so that the lock
  file is deleted immediately and the lockfile object is left in a
  predictable state that (namely, unlocked).  Previously, the lockfile
  was retained until process cleanup in this situation.
 
 Another nice find. I wondered if we could test this, but I think it
 would be hard to trigger. The obvious reason for adjust_shared_perm to
 fail is that you do not have permissions on the file, but by definition
 you just created it. So I doubt this ever comes up in practice short of
 weird races (somebody dropping the x bit from an intermediate
 directory between the open() and chmod() or something).

...and I should have read the final sentence in your message more
carefully. Even if we did trigger it, the problem would only last until
the program exits anyway.

I agree that this is a nice cleanup, though; a caller that wants to
retry the lock before exiting would be much less surprised. And the same
logic applies to 06/22.

-Peff
--
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