I believe this is semantically correct.
Ramkumar, Julian: is the struct still needed? IIRC the reasons for
originally introducing (namely the 'volatile' qualifier) it have since
disappeared.
Daniel
Ramkumar Ramachandra wrote on Tue, Jan 11, 2011 at 21:33:33 +0530:
> Hi Julian and Daniel,
>
> Here's another revision after your suggestions on IRC. Thanks.
>
> [[[
> Determine default perms in an elegant thread-safe way, not racily.
>
> * subversion/libsvn_subr/io.c
>
> (default_perms_baton, perms_init_state): New struct, variable.
>
> (get_default_file_perms): Remove all functional code into
> init_default_file_perms, and use apr_atomic__init_once so that code
> is only executed once.
>
> (init_default_file_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.
> ]]]
>
> Index: subversion/libsvn_subr/io.c
> ===================================================================
> --- subversion/libsvn_subr/io.c (revision 1057166)
> +++ subversion/libsvn_subr/io.c (working copy)
> @@ -1300,6 +1300,39 @@ reown_file(const char *path,
> return svn_error_return(svn_io_remove_file2(unique_name, FALSE, pool));
> }
>
> +static volatile svn_atomic_t perms_init_state = 0;
> +struct default_perms_baton
> +{
> + apr_fileperms_t *default_perms;
> +};
> +
> +
> +/* 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 *
> +init_default_file_perms(void *baton, apr_pool_t *scratch_pool)
> +{
> + apr_fileperms_t *default_perms =
> + ((struct default_perms_baton *) baton)->default_perms;
> + apr_finfo_t finfo;
> + apr_file_t *fd;
> +
> + if (*default_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));
> + *default_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
> @@ -1310,46 +1343,13 @@ 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;
> + struct default_perms_baton baton;
>
> - /* 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;
> -
> + baton.default_perms = &default_perms;
> + SVN_ERR(svn_atomic__init_once(&perms_init_state, init_default_file_perms,
> + &baton, scratch_pool));
> + *perms = default_perms;
> return SVN_NO_ERROR;
> }
>
