I looked at this patch, I'm not happy with it, but my issues are so
specific (eg: <this> variable's initialization has <this> problem) that
I feel I'd be *dictating* a rewrite of the patch if I reviewed it.
And that is bad form, so I'm not going to do that...
Sorry,
Daniel
Ramkumar Ramachandra wrote on Sun, Jan 09, 2011 at 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. */
