Merge lp:~parthm/bzr/no-chown-if-bzrlog-exists into lp:bzr
- no-chown-if-bzrlog-exists
- Merge into bzr.dev
Status: | Merged |
---|---|
Approved by: | Vincent Ladeuil |
Approved revision: | 5126 |
Merge reported by: | Vincent Ladeuil |
Merged at revision: | not available |
Proposed branch: | lp:~parthm/bzr/no-chown-if-bzrlog-exists |
Merge into: | lp:bzr |
Diff against target: |
206 lines (+62/-62) 5 files modified
NEWS (+5/-5) bzrlib/config.py (+4/-2) bzrlib/osutils.py (+1/-25) bzrlib/tests/test_osutils.py (+23/-26) bzrlib/trace.py (+29/-4) |
To merge this branch: | bzr merge lp:~parthm/bzr/no-chown-if-bzrlog-exists |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Robert Collins (community) | Approve | ||
Martin Packman (community) | Approve | ||
Martin Pool | Needs Fixing | ||
John A Meinel | Approve | ||
Review via email: mp+22197@code.launchpad.net |
Commit message
Only chown() the .bzr.log when creating it.
Description of the change
This mp fixes the issue discussed in this mail:
http://
Basically, .bzr.log permissions were updated even when the file already existed rather than doing it just during file creation.
Robert Collins (lifeless) wrote : | # |
Parth Malwankar (parthm) wrote : | # |
> This doesn't actually solve the issue, as there is a race condition :).
You are right of course. Thanks for pointing that out.
After experimenting for some time I can't think of a good solution for the race condition. One option might be to just open() the file with 'at' later chown only if the ownership is different from from parent dir. The disadvantage is that we will do two stats per invocation. 1 for the parent dir and 1 for the file itself. Also, this will lead to bzr _always_ maintaining .bzr.log ownership the same as that of the containing directory. The could be considered a bug or a feature :-)
I will explore some more and see if I can come up with something.
Suggestions welcome.
John A Meinel (jameinel) wrote : | # |
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Parth Malwankar wrote:
>> This doesn't actually solve the issue, as there is a race condition :).
>
> You are right of course. Thanks for pointing that out.
>
> After experimenting for some time I can't think of a good solution for the race condition. One option might be to just open() the file with 'at' later chown only if the ownership is different from from parent dir. The disadvantage is that we will do two stats per invocation. 1 for the parent dir and 1 for the file itself. Also, this will lead to bzr _always_ maintaining .bzr.log ownership the same as that of the containing directory. The could be considered a bug or a feature :-)
> I will explore some more and see if I can come up with something.
>
> Suggestions welcome.
>
There *is* a race condition, but the window is small, and it is better
to be racy and get it mostly right than to fail in obvious ways.
If someone has set perms on .bzr.log explicitly, I'd rather we didn't
keep changing it on them. Given that .bzr.log is in ~, it is *possible*
you'd have them set differently. (I don't want people to see what files
are in ~, but I want to share ~/.bzr.log, for example. Set the dir to
rwx--x--- and the file to rw-r-----)
If the file doesn't exist, we should try to be nice about what perms the
file gets.
The only way I see to avoid a race is to do something like
os.open(O_EXCL) and if that succeeds, then we chmod/chown. If it fails,
then we just fall back to opening regularly.
Also note that it is only 'racy' if they:
1) For some reason create .bzr.log themselves
2) Start 2 bzr processes simultaneously that are *different* versions.
Otherwise, the 2 versions of bzr would use the same logic. And while
it make chown twice, they would do the same thing.
John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://
iEYEARECAAYFAku
SLUAoL+
=zhfY
-----END PGP SIGNATURE-----
John A Meinel (jameinel) wrote : | # |
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Parth Malwankar wrote:
>> This doesn't actually solve the issue, as there is a race condition :).
>
> You are right of course. Thanks for pointing that out.
>
> After experimenting for some time I can't think of a good solution for the race condition. One option might be to just open() the file with 'at' later chown only if the ownership is different from from parent dir. The disadvantage is that we will do two stats per invocation. 1 for the parent dir and 1 for the file itself. Also, this will lead to bzr _always_ maintaining .bzr.log ownership the same as that of the containing directory. The could be considered a bug or a feature :-)
> I will explore some more and see if I can come up with something.
>
> Suggestions welcome.
>
I should mention, *I* prefer having this patch to not having it. So
review: approve
But it seems like Robert is either hesitant or objecting, though he
didn't actually vote.
John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://
iEYEARECAAYFAku
pOsAmwdDCRuFZMM
=UQ/q
-----END PGP SIGNATURE-----
Martin Packman (gz) wrote : | # |
This can be done completely non-racily with os.open - a really picky version could be a function something like:
lomode = os.O_WRONLY | os.O_APPEND | osutils.O_TEXT
while True:
try:
return os.open(filename, lomode)
except OSError, e:
if e.errno != errno.ENOENT:
try:
fd = os.open(filename, lomode | os.O_CREAT | os.O_EXCL)
except OSError, e:
if e.errno != errno.EEXIST:
else:
return fd
And os.fdopen to make a regular file object.
Raises the question though, is it ever going to be right for a osutils.
Parth Malwankar (parthm) wrote : | # |
> This can be done completely non-racily with os.open - a really picky version
> could be a function something like:
>
> lomode = os.O_WRONLY | os.O_APPEND | osutils.O_TEXT
> while True:
> try:
> return os.open(filename, lomode)
> except OSError, e:
> if e.errno != errno.ENOENT:
> raise
> try:
> fd = os.open(filename, lomode | os.O_CREAT | os.O_EXCL)
> except OSError, e:
> if e.errno != errno.EEXIST:
> raise
> else:
> copy_ownership(
> return fd
>
> And os.fdopen to make a regular file object.
>
Thanks for this.
This certainly looks good to me. Would this approach work on Mac? In the python docs[1] the availability of os.open is shown as Unix and Windows. I don't know much about Mac so I ask.
> Raises the question though, is it ever going to be right for a
> osutils.
> created it? Makes me wonder again whether bundling the chown into the open
> function makes any sense.
I agree, I don't see too much use of open_with_ownership in a non-create case.
I can try to create and test a patch based on the above approach which also deals with open_with_
Martin Packman (gz) wrote : | # |
> This certainly looks good to me. Would this approach work on Mac? In the
> python docs[1] the availability of os.open is shown as Unix and Windows. I
> don't know much about Mac so I ask.
The Python docs are a little confusing on this, but by their definition there is no longer such a thing as a mac, anything running OS X just another unix. Upstream Python has pretty much dropped support for everything but posix and windows systems.
> > Raises the question though, is it ever going to be right for a
> > osutils.
> > created it? Makes me wonder again whether bundling the chown into the open
> > function makes any sense.
Got my thinking a little backwards here, if chown should only be on create, building that logic into open_with_ownership probably makes sense.
> I agree, I don't see too much use of open_with_ownership in a non-create case.
> I can try to create and test a patch based on the above approach which also
> deals with open_with_
Worth pondering how you think the interfaces should work, there are currently three new public functions in osutils, and only as many callsites.
Parth Malwankar (parthm) wrote : | # |
Updated based on file creation code by Martin [gz]. Thanks.
Removed open_with_ownership and copy_with_
Parth Malwankar (parthm) wrote : | # |
One minor annoyance with that is that .bzr.log is created with the executable bit set due to O_CREAT. As per the open(2) man page:
The effective permissions are modified by the process's umask in the usual way: The permissions of the created file are (mode & ~umask).
On my system the mask is 022. I suppose we can do a chown specifically to unset executable bit. Thoughts?
Note: This isn't tested on Windows yet as I don't have a windows system available at the moment.
John A Meinel (jameinel) wrote : | # |
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Parth Malwankar wrote:
> One minor annoyance with that is that .bzr.log is created with the executable bit set due to O_CREAT. As per the open(2) man page:
>
> The effective permissions are modified by the process's umask in the usual way: The permissions of the created file are (mode & ~umask).
>
> On my system the mask is 022. I suppose we can do a chown specifically to unset executable bit. Thoughts?
>
> Note: This isn't tested on Windows yet as I don't have a windows system available at the moment.
>
Why not just use "mode = source_mode & ~0111", which should always strip
the executable bit off. I don't see any need have the file executable.
John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://
iEYEARECAAYFAku
pZEAoLDZtbpEfnW
=nMDG
-----END PGP SIGNATURE-----
Martin Pool (mbp) wrote : | # |
+ """Create a log file in while avoiding race.
some kind of typo here.
I would make the docstring be:
Create the .bzr.log file.
It inherits the ownership and permissions (masked by umask) from the containing
directory to cope better with being run under sudo with $HOME still set to the
user's homedir.
Also the fact that you have a comment next to every copy_ownership() invocation suggests it should be renamed to copy_ownership_
To set the permissions you need to pass a third parameter to os.open(), which here should be 0666. This is separate from the O_APPEND etc mode.
Thanks!
Parth Malwankar (parthm) wrote : | # |
> + """Create a log file in while avoiding race.
>
> some kind of typo here.
>
> I would make the docstring be:
>
> Create the .bzr.log file.
>
> It inherits the ownership and permissions (masked by umask) from the
> containing
> directory to cope better with being run under sudo with $HOME still set to
> the
> user's homedir.
>
Done.
> Also the fact that you have a comment next to every copy_ownership()
> invocation suggests it should be renamed to copy_ownership_
>
I have renamed it to copy_ownership_
directory are accepted, though we use only parent directory in
the existing cases.
> To set the permissions you need to pass a third parameter to os.open(), which
> here should be 0666. This is separate from the O_APPEND etc mode.
>
I have added permission of 0644 and tested it out.
Thanks.
Martin Pool (mbp) wrote : | # |
On 30 March 2010 16:37, Parth Malwankar <email address hidden> wrote:
> I have added permission of 0644 and tested it out.
No, it should actually be 0666. If the user sets their umask to allow
group-writable files we should respect that unless there is a good
reason.
--
Martin <http://
Parth Malwankar (parthm) wrote : | # |
> No, it should actually be 0666. If the user sets their umask to allow
> group-writable files we should respect that unless there is a good
> reason.
>
Makes sense. Changed permissions to 0666.
Martin Packman (gz) wrote : | # |
Couple of style points, one of which causes a bug.
Generally you want the body of try/except loops to be as minimal, hence my use of else in the example before. I have mixed feelings about nested functions that aren't actually being used as closures. Factoring common logic branches into one route is generally a good thing, even if it means using break-as-goto.
Rather than things along the lines of:
permissions = 0666
fd = os.open(filename, flags, permissions)
it's better to use:
fd = os.open(filename, flags, mode=0666)
which is still self-documenting, and avoids the variable.
As you do that with flags too, you're defeating the race protection of the loop by making all attempts after the first exclusive, potentially causing an infinite loop instead:
flags = os.O_WRONLY | os.O_APPEND | osutils.O_TEXT
while True:
...
fd = os.open(filename, flags)
...
flags = flags | os.O_CREAT | os.O_EXCL
fd = os.open(filename, flags, permissions)
Also bt.test_
Pull <lp:~gz/bzr/no-chown-if-bzrlog-exists> for my fixes for these. I've poked a couple of other minor bits while I was there, but didn't do anything major. (I'd really like windows to avoid this codepath entirely, but this doesn't need to be done right now.)
Parth Malwankar (parthm) wrote : | # |
> Pull <lp:~gz/bzr/no-chown-if-bzrlog-exists> for my fixes for these. I've poked
> a couple of other minor bits while I was there, but didn't do anything major.
> (I'd really like windows to avoid this codepath entirely, but this doesn't
> need to be done right now.)
Thanks for the updates Martin [gz]. I have merged in the changes.
Parth Malwankar (parthm) wrote : | # |
merged in changes from trunk and moved NEWS entry to 2.2b2
Robert Collins (lifeless) wrote : | # |
I think this is an improvement. However my experience with race conditions is that we always, eventually, run into them.
So I'd like a new bug filed saying that we have this race and should fix it. That can be a wishlist bug, but we know the defect exists so lets capture the thinking about it now to save a future user having to figure it out.
Parth Malwankar (parthm) wrote : | # |
> I think this is an improvement. However my experience with race conditions is
> that we always, eventually, run into them.
>
> So I'd like a new bug filed saying that we have this race and should fix it.
> That can be a wishlist bug, but we know the defect exists so lets capture the
> thinking about it now to save a future user having to figure it out.
Filed Bug #567036 for this.
Robert Collins (lifeless) wrote : | # |
So, this is failing on PQM. I don't have a detailed diagnostic for you yet, but there are 14 test failures.
Robert Collins (lifeless) wrote : | # |
Some failures:
=======
FAIL: bzrlib.
-------
_StringException: Text attachment: log
------------
------------
Text attachment: traceback
------------
Traceback (most recent call last):
File "/usr/lib/
return fn(*args)
File "/usr/lib/
testMethod()
File "/home/
self.
AssertionError: 'Frob the sample object' is not None.
------------
FAIL: bzrlib.
-------
_StringException: Text attachment: log
------------
------------
Text attachment: traceback
------------
Traceback (most recent call last):
File "/usr/lib/
return fn(*args)
File "/usr/lib/
testMethod()
File "/home/
sam.bank)
AssertionError: 'Bank the sample, but using bar and biz.' is not None.
------------
FAIL: bzrlib.
-------
_StringException: Text attachment: log
------------
------------
Text attachment: traceback
------------
Traceback (most recent call last):
File "/usr/lib/
return fn(*args)
File "/usr/lib/
testMethod()
File "/home/
self.
AssertionError: 'Just a function that supplies several arguments.' is not None.
------------
FAIL: bzrlib.
-------
_StringException: Text attachment: log
------------
------------
Text attachment: traceback
------------
Traceback (most recent call last):
File "/usr/lib/
return fn(*args)
File "/usr/lib/
testMethod()
File "/home/
self.
AssertionError: 'Just a function that supplies several arguments.' is not None.
------------
FAIL: bzrlib.
------------...
Parth Malwankar (parthm) wrote : | # |
On Wed, Apr 21, 2010 at 1:11 PM, Robert Collins
<email address hidden> wrote:
> Some failures:
>
> =======
> FAIL: bzrlib.
> -------
...
> Which is pretty strange. Its possible that I've got the wrong mp here, but tomorrow we should have the reporting code all fixed up in pqm anyhow. Have you tried a full test run?
> Its very odd to see these failures on this proposal, as I'm sure you can guess ;).
>
These results do look surprising. I am able to run all the failing
tests without any problems. I ran bt.test_decorators on WinXP
+ Linux and others on linux.
Any ideas?
-------
Ran 3 tests in 0.142s
OK
[no-chown-
bt.test_decorators
bzr selftest: /storage/
/storage/
bzr-2.2.0dev1 python-2.6.4
Linux-2.
-------
Ran 34 tests in 0.121s
OK
[no-chown-
bzr selftest: /storage/
/storage/
bzr-2.2.0dev1 python-2.6.4
Linux-2.
-------
Ran 1214 tests in 326.842s
OK (known_failures=2)
Missing feature 'case-insensitive case-preserving filesystem' skipped 18 tests.
Missing feature 'case-insensitive filesystem' skipped 1 tests.
bzrlib.
is leaking threads among 31 leaking tests.
17 non-main threads were left active in the end.
[no-chown-
Unable to load plugin 'hg'. It requested API version (2, 1, 0) of
module <module 'bzrlib' from
'/storage/
but the minimum exported version is (2, 2, 0), and the maximum is (2,
2, 0)
Unable to load plugin 'svn'. It requested API version (2, 1, 0) of
module <module 'bzrlib' from
'/storage/
but the minimum exported version is (2, 2, 0), and the maximum is (2,
2, 0)
Unable to load plugin 'git'. It requested API version (2, 1, 0) of
module <module 'bzrlib' from
'/storage/
but the minimum exported version is (2, 2, 0), and the maximum is (2,
2, 0)
bzr selftest: /storage/
/storage/
bzr-2.2.0dev1 python-2.6.4
Linux-2.
-------
Ran 72 tests in 1.313s
OK
[no-chown-
bzr sel...
Parth Malwankar (parthm) wrote : | # |
Its been about an hour since I emailed on the test but for some reason that hasn't shown up so I just paste it here for now. Sorry if it shows up twice.
On Wed, Apr 21, 2010 at 1:11 PM, Robert Collins
<email address hidden> wrote:
> Some failures:
>
> =======
> FAIL: bzrlib.
> -------
...
> Which is pretty strange. Its possible that I've got the wrong mp here, but tomorrow we should have the reporting code all fixed up in pqm anyhow. Have you tried a full test run?
> Its very odd to see these failures on this proposal, as I'm sure you can guess ;).
>
These results do look surprising. I am able to run all the failing
tests without any problems. I ran bt.test_decorators on WinXP
+ Linux and others on linux.
-------
Ran 3 tests in 0.142s
OK
[no-chown-
bt.test_decorators
bzr selftest: /storage/
/storage/
bzr-2.2.0dev1 python-2.6.4
Linux-2.
-------
Ran 34 tests in 0.121s
OK
[no-chown-
bzr selftest: /storage/
/storage/
bzr-2.2.0dev1 python-2.6.4
Linux-2.
-------
Ran 1214 tests in 326.842s
OK (known_failures=2)
Missing feature 'case-insensitive case-preserving filesystem' skipped 18 tests.
Missing feature 'case-insensitive filesystem' skipped 1 tests.
bzrlib.
is leaking threads among 31 leaking tests.
17 non-main threads were left active in the end.
[no-chown-
Unable to load plugin 'hg'. It requested API version (2, 1, 0) of
module <module 'bzrlib' from
'/storage/
but the minimum exported version is (2, 2, 0), and the maximum is (2,
2, 0)
Unable to load plugin 'svn'. It requested API version (2, 1, 0) of
module <module 'bzrlib' from
'/storage/
but the minimum exported version is (2, 2, 0), and the maximum is (2,
2, 0)
Unable to load plugin 'git'. It requested API version (2, 1, 0) of
module <module 'bzrlib' from
'/storage/
but the minimum exported version is (2, 2, 0), and the maximum is (2,
2, 0)
bzr selftest: /storage/
/storage/
bzr-2.2.0dev1 python-2.6.4
Linux-2.
-------
Martin Packman (gz) wrote : | # |
Those all look like they're from my <lp:~gz/bzr/support_OO_flag> branch, and seem to just be a dumb error on my part, I'll handle over there.
Vincent Ladeuil (vila) wrote : | # |
It looks like we had one more failure without feedback on pqm here... no mail, no comment in the mp... I'll look into it first thing monday morning unless lifeless (nudge nudge, what ? still not unpacked ? :-P) beats me to it
Vincent Ladeuil (vila) wrote : | # |
Haa, submitting by mail revealed a conflict in NEWS, new submission sent.
Vincent Ladeuil (vila) wrote : | # |
..which also failed due to test failing (some renaming was still needed for copy copy_ownership_
- 5127. By Parth Malwankar
-
fixed test failure in bt.test_osutils
Parth Malwankar (parthm) wrote : | # |
> ..which also failed due to test failing (some renaming was still needed for
> copy copy_ownership_
Ouch. This is embarrassing. I don't know how I missed something this obvious. Sorry about that.
I have fixed it and pushed the changes.
- 5128. By Parth Malwankar
-
merged in changes from trunk
- 5129. By Parth Malwankar
-
moved NEWS entry to 2.2b3
Vincent Ladeuil (vila) wrote : | # |
>>>>> Parth Malwankar <email address hidden> writes:
>> ..which also failed due to test failing (some renaming was still needed for
>> copy copy_ownership_
> Ouch. This is embarrassing.
Nah, don't worry about it. The fact that *pqm* didn't tell me that is
more embarassing...
> I don't know how I missed something this obvious. Sorry about
> that. I have fixed it and pushed the changes.
I had already sent the mp to pqm, but thanks all the same ;)
Preview Diff
1 | === modified file 'NEWS' |
2 | --- NEWS 2010-04-23 14:15:23 +0000 |
3 | +++ NEWS 2010-04-24 15:10:01 +0000 |
4 | @@ -29,6 +29,11 @@ |
5 | Bug Fixes |
6 | ********* |
7 | |
8 | +* ``.bazaar``, ``.bazaar/bazaar.conf`` and ``.bzr.log`` inherit user and |
9 | + group ownership from the containing directory. This allow bzr to work |
10 | + better with sudo. |
11 | + (Martin <gzlist@googlemail.com>, Parth Malwankar, #376388) |
12 | + |
13 | * ``bzr selftest --parallel=fork`` wait for its children avoiding zombies. |
14 | (Vincent Ladeuil, #566670) |
15 | |
16 | @@ -369,11 +374,6 @@ |
17 | mainline (i.e. it supports dotted revisions). |
18 | (Parth Malwankar, #517800) |
19 | |
20 | -* ``.bazaar``, ``.bazaar/bazaar.conf`` and ``.bzr.log`` inherit user and |
21 | - group ownership from the containing directory. This allow bzr to work |
22 | - better with sudo. |
23 | - (Parth Malwankar, #376388) |
24 | - |
25 | * Use first apparent author not committer in GNU Changelog format. |
26 | (Martin von Gagern, #513322) |
27 | |
28 | |
29 | === modified file 'bzrlib/config.py' |
30 | --- bzrlib/config.py 2010-04-23 08:51:52 +0000 |
31 | +++ bzrlib/config.py 2010-04-24 15:10:01 +0000 |
32 | @@ -519,7 +519,8 @@ |
33 | |
34 | def _write_config_file(self): |
35 | path = self._get_filename() |
36 | - f = osutils.open_with_ownership(path, 'wb') |
37 | + f = open(path, 'wb') |
38 | + osutils.copy_ownership_from_path(path) |
39 | self._get_parser().write(f) |
40 | f.close() |
41 | |
42 | @@ -818,7 +819,8 @@ |
43 | trace.mutter('creating config parent directory: %r', parent_dir) |
44 | os.mkdir(parent_dir) |
45 | trace.mutter('creating config directory: %r', path) |
46 | - osutils.mkdir_with_ownership(path) |
47 | + os.mkdir(path) |
48 | + osutils.copy_ownership_from_path(path) |
49 | |
50 | |
51 | def config_dir(): |
52 | |
53 | === modified file 'bzrlib/osutils.py' |
54 | --- bzrlib/osutils.py 2010-04-23 09:28:29 +0000 |
55 | +++ bzrlib/osutils.py 2010-04-24 15:10:01 +0000 |
56 | @@ -1827,7 +1827,7 @@ |
57 | real_handlers[kind](abspath, relpath) |
58 | |
59 | |
60 | -def copy_ownership(dst, src=None): |
61 | +def copy_ownership_from_path(dst, src=None): |
62 | """Copy usr/grp ownership from src file/dir to dst file/dir. |
63 | |
64 | If src is None, the containing directory is used as source. If chown |
65 | @@ -1849,30 +1849,6 @@ |
66 | trace.warning("Unable to copy ownership from '%s' to '%s': IOError: %s." % (src, dst, e)) |
67 | |
68 | |
69 | -def mkdir_with_ownership(path, ownership_src=None): |
70 | - """Create the directory 'path' with specified ownership. |
71 | - |
72 | - If ownership_src is given, copies (chown) usr/grp ownership |
73 | - from 'ownership_src' to 'path'. If ownership_src is None, use the |
74 | - containing dir ownership. |
75 | - """ |
76 | - os.mkdir(path) |
77 | - copy_ownership(path, ownership_src) |
78 | - |
79 | - |
80 | -def open_with_ownership(filename, mode='r', bufsize=-1, ownership_src=None): |
81 | - """Open the file 'filename' with the specified ownership. |
82 | - |
83 | - If ownership_src is specified, copy usr/grp ownership from ownership_src |
84 | - to filename. If ownership_src is None, copy ownership from containing |
85 | - directory. |
86 | - Returns the opened file object. |
87 | - """ |
88 | - f = open(filename, mode, bufsize) |
89 | - copy_ownership(filename, ownership_src) |
90 | - return f |
91 | - |
92 | - |
93 | def path_prefix_key(path): |
94 | """Generate a prefix-order path key for path. |
95 | |
96 | |
97 | === modified file 'bzrlib/tests/test_osutils.py' |
98 | --- bzrlib/tests/test_osutils.py 2010-03-11 06:24:00 +0000 |
99 | +++ bzrlib/tests/test_osutils.py 2010-04-24 15:10:01 +0000 |
100 | @@ -1990,29 +1990,26 @@ |
101 | def _dummy_chown(self, path, uid, gid): |
102 | self.path, self.uid, self.gid = path, uid, gid |
103 | |
104 | - def test_mkdir_with_ownership_chown(self): |
105 | - """Ensure that osutils.mkdir_with_ownership chowns correctly with ownership_src. |
106 | - """ |
107 | - ownsrc = '/' |
108 | - osutils.mkdir_with_ownership('foo', ownsrc) |
109 | - |
110 | - s = os.stat(ownsrc) |
111 | - self.assertEquals(self.path, 'foo') |
112 | - self.assertEquals(self.uid, s.st_uid) |
113 | - self.assertEquals(self.gid, s.st_gid) |
114 | - |
115 | - def test_open_with_ownership_chown(self): |
116 | - """Ensure that osutils.open_with_ownership chowns correctly with ownership_src. |
117 | - """ |
118 | - ownsrc = '/' |
119 | - f = osutils.open_with_ownership('foo', 'w', ownership_src=ownsrc) |
120 | - |
121 | - # do a test write and close |
122 | - f.write('hello') |
123 | - f.close() |
124 | - |
125 | - s = os.stat(ownsrc) |
126 | - self.assertEquals(self.path, 'foo') |
127 | - self.assertEquals(self.uid, s.st_uid) |
128 | - self.assertEquals(self.gid, s.st_gid) |
129 | - |
130 | + def test_copy_ownership_from_path(self): |
131 | + """copy_ownership_from_path test with specified src. |
132 | + """ |
133 | + ownsrc = '/' |
134 | + f = open('test_file', 'wt') |
135 | + osutils.copy_ownership_from_path('test_file', ownsrc) |
136 | + |
137 | + s = os.stat(ownsrc) |
138 | + self.assertEquals(self.path, 'test_file') |
139 | + self.assertEquals(self.uid, s.st_uid) |
140 | + self.assertEquals(self.gid, s.st_gid) |
141 | + |
142 | + def test_copy_ownership_nonesrc(self): |
143 | + """copy_ownership_from_path test with src=None. |
144 | + """ |
145 | + f = open('test_file', 'wt') |
146 | + # should use parent dir for permissions |
147 | + osutils.copy_ownership_from_path('test_file') |
148 | + |
149 | + s = os.stat('..') |
150 | + self.assertEquals(self.path, 'test_file') |
151 | + self.assertEquals(self.uid, s.st_uid) |
152 | + self.assertEquals(self.gid, s.st_gid) |
153 | |
154 | === modified file 'bzrlib/trace.py' |
155 | --- bzrlib/trace.py 2010-03-25 11:08:55 +0000 |
156 | +++ bzrlib/trace.py 2010-04-24 15:10:01 +0000 |
157 | @@ -238,12 +238,37 @@ |
158 | This sets the global _bzr_log_filename. |
159 | """ |
160 | global _bzr_log_filename |
161 | + |
162 | + def _open_or_create_log_file(filename): |
163 | + """Open existing log file, or create with ownership and permissions |
164 | + |
165 | + It inherits the ownership and permissions (masked by umask) from |
166 | + the containing directory to cope better with being run under sudo |
167 | + with $HOME still set to the user's homedir. |
168 | + """ |
169 | + flags = os.O_WRONLY | os.O_APPEND | osutils.O_TEXT |
170 | + while True: |
171 | + try: |
172 | + fd = os.open(filename, flags) |
173 | + break |
174 | + except OSError, e: |
175 | + if e.errno != errno.ENOENT: |
176 | + raise |
177 | + try: |
178 | + fd = os.open(filename, flags | os.O_CREAT | os.O_EXCL, 0666) |
179 | + except OSError, e: |
180 | + if e.errno != errno.EEXIST: |
181 | + raise |
182 | + else: |
183 | + osutils.copy_ownership_from_path(filename) |
184 | + break |
185 | + return os.fdopen(fd, 'at', 0) # unbuffered |
186 | + |
187 | + |
188 | _bzr_log_filename = _get_bzr_log_filename() |
189 | _rollover_trace_maybe(_bzr_log_filename) |
190 | try: |
191 | - buffering = 0 # unbuffered |
192 | - bzr_log_file = osutils.open_with_ownership(_bzr_log_filename, 'at', buffering) |
193 | - # bzr_log_file.tell() on windows always return 0 until some writing done |
194 | + bzr_log_file = _open_or_create_log_file(_bzr_log_filename) |
195 | bzr_log_file.write('\n') |
196 | if bzr_log_file.tell() <= 2: |
197 | bzr_log_file.write("this is a debug log for diagnosing/reporting problems in bzr\n") |
198 | @@ -252,7 +277,7 @@ |
199 | |
200 | return bzr_log_file |
201 | |
202 | - except IOError, e: |
203 | + except EnvironmentError, e: |
204 | # If we are failing to open the log, then most likely logging has not |
205 | # been set up yet. So we just write to stderr rather than using |
206 | # 'warning()'. If we using warning(), users get the unhelpful 'no |
This doesn't actually solve the issue, as there is a race condition :).