[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r1004286 - in /subversion/trunk: ./ subversion/libsvn_subr/io.c

From: Ramkumar Ramachandra <artagnon_at_gmail.com>
Date: Sun, 9 Jan 2011 15:29:10 +0530

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. */
Received on 2011-01-09 10:59:12 CET

This is an archived mail posted to the Subversion Dev mailing list.