Kevin Pilch-Bisson <kevin@pilch-bisson.net> writes:
> No problem. I did see the stuff there, and I thought it might be possible to
> put it there while waiting for it to go into APR. I'll re-work the patch to
> include a patch to APR as well.
Great, thanks.
> > - I don't understand why we suddenly need to pay attention to umask
> > for this change. The idea is to make the .svn area read-only,
> > except for the tmp area, which doesn't need umask afaik. So if we
> > weren't using umasks before now, why does this change require
> > paying attention to them?
> >
> > This isn't actually a nit, it's a real question -- probably
> > there's a good reason for the umask stuff, and I'm just not
> > thinking of it...
>
> Consider the way an update works if there are no local mods. It copies the
> text-base file to the tmp area, applies the txdelta, and then moves it back to
> text-base and copies it out to the working copy. Note that this means it
> starts with the supposedly read-only text-base file. If we don't change the
> permissions, then we run into problems moving stuff around. Also even if we
> didn't we wind up with a read-only file in the working copy. However, if we
> just set the permissions, without worrying about the umask, then what do we
> set them to? We could set them -rw-------, but then people have to re-set
> the permissions to allow group reading every time the do an update. We can't
> set them -rw-r--r--, because they might contain information that a user doesn't
> want to be shared. So overall our best strategy is to move or copy the file,
> then set the permissions to look like we just created the file, which means
> obeying the umask.
>
> There are however, two things I don't like about this patch.
>
> 1) It resets a file to non-executable after an update, even if the user had
> made it executable (e.g. autogen.sh). Not a big deal in the long run, since
> sometime (hopefully soon), we will know that a file should have the execute
> bit set, and set it.
Bingo.
What we should do, in the case of copies over existing working files,
is mimic the permissions of the file being replaced. (Unless the
update also sets the execute bit via properties or something, but
that's another story and isn't a concern yet.)
When creating a new file, then I guess best bet is to pay attention to
umask and whatever the directory inheritance conventions are (I can't
remember them off the top of my head, but I think there are some...?)
> 2) There is a race condition in svn_io_set_permissions for multithreaded
> apps, with regard to the umask. Just wondering if anyone had any ideas of
> how to fix that for apr. I was thinking of making:
>
> apr_status_t apr_umask_get(apr_fileperms_t *mask);
>
> and
>
> apr_status_t apr_umask_set(apr_fileperms_t *mask);
>
> Then it would be possible to perform a umask(mask=umask(0777)) in
> apr_initialize (before any threads are created) and store the umask somewhere.
> Then apr_umask_get would just return the value, and apr_umask_set could set
> both apr's copy and call umask to set the new value.
>
> Sound okay???
Hmm, don't quite understand the bit about the race condition and the
proposed solution, may need more explanation... What exactly is the
problem? I thought umask was something APR will be getting from the
user's environment?
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Oct 21 14:36:50 2006