Bazaar

Code review comment for lp:~parthm/bzr/no-chown-if-bzrlog-exists

no-chown-if-bzrlog-exists
Merge into bzr.dev

Revision history for this message

John A Meinel (jameinel) wrote on 2010-03-27:

-----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://enigmail.mozdev.org/

iEYEARECAAYFAkuuCMkACgkQJdeBCYSNAAM5dACgvJpiReYnhZzkJQP9xBLJOBE5
SLUAoL+Ew9G0uk4N88VQSdALDxIb1ySV
=zhfY
-----END PGP SIGNATURE-----

-----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 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://enigmail.mozdev.org/

iEYEARECAAYFAkuuCMkACgkQJdeBCYSNAAM5dACgvJpiReYnhZzkJQP9xBLJOBE5
SLUAoL+Ew9G0uk4N88VQSdALDxIb1ySV
=zhfY
-----END PGP SIGNATURE-----

« Back to merge proposal