Hi Daniel, Daniel Shahaf writes: > If you send another patch that indents/dedents a block, could you please > attach a 'svn diff -x-w' version of the patch too? It would make > reviewing easier.
Sure. Here's the (hopefully) final patch. Sorry about the slopiness earlier -- I was in a hurry to get the concept right. diff -x-w version (for review): Index: subversion/libsvn_subr/io.c =================================================================== --- subversion/libsvn_subr/io.c (revision 1056662) +++ subversion/libsvn_subr/io.c (working copy) @@ -1299,56 +1299,47 @@ return svn_error_return(svn_io_remove_file2(unique_name, FALSE, pool)); } -/* Determine what the PERMS for a new file should be by looking at the - permissions of a temporary file that we create. - Unfortunately, umask() as defined in POSIX provides no thread-safe way - to get at the current value of the umask, so what we're doing here is - the only way we have to determine which combination of write bits - (User/Group/World) should be set by default. - Make temporary allocations in SCRATCH_POOL. */ +static volatile apr_fileperms_t default_perms = 0; +static volatile svn_atomic_t perms_init_state; + +/* Helper function to discover default file permissions; it does this + by creating a temporary file and extracting the permissions from + it. Passed to svn_atomic__init_once. All temporary allocations are + done in SCRATCH_POOL. */ static svn_error_t * -get_default_file_perms(apr_fileperms_t *perms, apr_pool_t *scratch_pool) +get_tmpfile_perms(void *baton, apr_pool_t *scratch_pool) { - /* the default permissions as read from the temp folder */ - static apr_fileperms_t default_perms = 0; - - /* Technically, this "racy": Multiple threads may use enter here and - try to figure out the default permisission concurrently. That's fine - since they will end up with the same results. Even more technical, - apr_fileperms_t is an atomic type on 32+ bit machines. - */ - if (default_perms == 0) - { + apr_fileperms_t *perms = (apr_fileperms_t *) baton; apr_finfo_t finfo; apr_file_t *fd; - /* Get the perms for a newly created file to find out what bits - should be set. + if (*perms != 0) + /* Nothing to do */ + return SVN_NO_ERROR; - Normally del_on_close can be problematic because APR might - delete the file if we spawned any child processes. In this - case, the lifetime of this file handle is about 3 lines of - code, so we can safely use del_on_close here. - - Not so fast! If some other thread forks off a child, then the - APR cleanups run, and the file will disappear. So use - del_on_pool_cleanup instead. - - Using svn_io_open_uniquely_named() here because other tempfile - creation functions tweak the permission bits of files they create. - */ SVN_ERR(svn_io_open_uniquely_named(&fd, NULL, NULL, "svn-tempfile", ".tmp", svn_io_file_del_on_pool_cleanup, scratch_pool, scratch_pool)); SVN_ERR(svn_io_file_info_get(&finfo, APR_FINFO_PROT, fd, scratch_pool)); SVN_ERR(svn_io_file_close(fd, scratch_pool)); - *perms = finfo.protection; - default_perms = finfo.protection; + + return SVN_NO_ERROR; } - else - *perms = default_perms; +/* Determine what the PERMS for a new file should be by looking at the + permissions of a temporary file that we create. + Unfortunately, umask() as defined in POSIX provides no thread-safe way + to get at the current value of the umask, so what we're doing here is + the only way we have to determine which combination of write bits + (User/Group/World) should be set by default. + Make temporary allocations in SCRATCH_POOL. */ +static svn_error_t * +get_default_file_perms(apr_fileperms_t *perms, apr_pool_t *scratch_pool) +{ + SVN_ERR(svn_atomic__init_once(&perms_init_state, get_tmpfile_perms, + (void *) &default_perms, scratch_pool)); + *perms = default_perms; return SVN_NO_ERROR; } @@ -1360,9 +1351,9 @@ apr_pool_t *scratch_pool) { apr_finfo_t finfo; - apr_fileperms_t default_perms; - SVN_ERR(get_default_file_perms(&default_perms, scratch_pool)); + SVN_ERR(get_default_file_perms((apr_fileperms_t *) &default_perms, + scratch_pool)); SVN_ERR(svn_io_file_info_get(&finfo, APR_FINFO_PROT, fd, scratch_pool)); /* Glom the perms together. */ --8<-- [[[ Determine default perms in an elegant thread-safe way, not racily * subversion/libsvn_subr/io.c (): Add two static volatile global variables to be used for storing permissions: default_perms and perms_init_state. (get_default_file_perms): Remove all functional code into get_tmpfile_perms, and use apr_atomic__init_once so that code is only executed once. (get_tmpfile_perms): New function to determine default file permissions by creating a temporary file and extracting permissions from it. Default permissions are not determined racily anymore, since this function is an apr_atomic__init_once callback. (merge_default_file_perms): Remove unnecessary local default_perms variable; use the global one instead. ]]] Index: subversion/libsvn_subr/io.c =================================================================== --- subversion/libsvn_subr/io.c (revision 1056662) +++ subversion/libsvn_subr/io.c (working copy) @@ -1299,6 +1299,34 @@ reown_file(const char *path, return svn_error_return(svn_io_remove_file2(unique_name, FALSE, pool)); } +static volatile apr_fileperms_t default_perms = 0; +static volatile svn_atomic_t perms_init_state; + +/* Helper function to discover default file permissions; it does this + by creating a temporary file and extracting the permissions from + it. Passed to svn_atomic__init_once. All temporary allocations are + done in SCRATCH_POOL. */ +static svn_error_t * +get_tmpfile_perms(void *baton, apr_pool_t *scratch_pool) +{ + apr_fileperms_t *perms = (apr_fileperms_t *) baton; + apr_finfo_t finfo; + apr_file_t *fd; + + if (*perms != 0) + /* Nothing to do */ + return SVN_NO_ERROR; + + SVN_ERR(svn_io_open_uniquely_named(&fd, NULL, NULL, "svn-tempfile", ".tmp", + svn_io_file_del_on_pool_cleanup, + scratch_pool, scratch_pool)); + SVN_ERR(svn_io_file_info_get(&finfo, APR_FINFO_PROT, fd, scratch_pool)); + SVN_ERR(svn_io_file_close(fd, scratch_pool)); + *perms = finfo.protection; + + return SVN_NO_ERROR; +} + /* Determine what the PERMS for a new file should be by looking at the permissions of a temporary file that we create. Unfortunately, umask() as defined in POSIX provides no thread-safe way @@ -1309,46 +1337,9 @@ reown_file(const char *path, static svn_error_t * get_default_file_perms(apr_fileperms_t *perms, apr_pool_t *scratch_pool) { - /* the default permissions as read from the temp folder */ - static apr_fileperms_t default_perms = 0; - - /* Technically, this "racy": Multiple threads may use enter here and - try to figure out the default permisission concurrently. That's fine - since they will end up with the same results. Even more technical, - apr_fileperms_t is an atomic type on 32+ bit machines. - */ - if (default_perms == 0) - { - apr_finfo_t finfo; - apr_file_t *fd; - - /* Get the perms for a newly created file to find out what bits - should be set. - - Normally del_on_close can be problematic because APR might - delete the file if we spawned any child processes. In this - case, the lifetime of this file handle is about 3 lines of - code, so we can safely use del_on_close here. - - Not so fast! If some other thread forks off a child, then the - APR cleanups run, and the file will disappear. So use - del_on_pool_cleanup instead. - - Using svn_io_open_uniquely_named() here because other tempfile - creation functions tweak the permission bits of files they create. - */ - SVN_ERR(svn_io_open_uniquely_named(&fd, NULL, NULL, "svn-tempfile", ".tmp", - svn_io_file_del_on_pool_cleanup, - scratch_pool, scratch_pool)); - SVN_ERR(svn_io_file_info_get(&finfo, APR_FINFO_PROT, fd, scratch_pool)); - SVN_ERR(svn_io_file_close(fd, scratch_pool)); - - *perms = finfo.protection; - default_perms = finfo.protection; - } - else - *perms = default_perms; - + SVN_ERR(svn_atomic__init_once(&perms_init_state, get_tmpfile_perms, + (void *) &default_perms, scratch_pool)); + *perms = default_perms; return SVN_NO_ERROR; } @@ -1360,9 +1351,9 @@ merge_default_file_perms(apr_file_t *fd, apr_filep apr_pool_t *scratch_pool) { apr_finfo_t finfo; - apr_fileperms_t default_perms; - SVN_ERR(get_default_file_perms(&default_perms, scratch_pool)); + SVN_ERR(get_default_file_perms((apr_fileperms_t *) &default_perms, + scratch_pool)); SVN_ERR(svn_io_file_info_get(&finfo, APR_FINFO_PROT, fd, scratch_pool)); /* Glom the perms together. */