Hi,
I would like to get r985477 merged into trunk. I've applied and used
it successfully and checked that all tests pass.
Warning: I have no background knowledge. I'm just reviewing the patch
as-it-is, because Stefan asked me to.
> [[[
> r985477 | stefan2 | 2010-08-14 18:02:04 +0530 (Sat, 14 Aug 2010) | 9 lines
>
> Eliminate all file system access in get_default_file_perms() for all but the first execution.
>
> The only downside is that we don't detect FS permission changes made while the
> (client) process runs. Because such changes would cause race conditions and file I/O
> errors anyway, we are not make things worse by omitting those tests.
>
> * subversion/libsvn_subr/io.c
> (get_default_file_perms): store result in a singleton in the first run and bypass
> the FS access in all later runs
I looked at this after reading the patch, and I must admit that I
didn't get the idea from the log message: how about using "static
variable" instead of "singleton".
> ]]]
>
> Index: subversion/libsvn_subr/io.c
> ===================================================================
> --- subversion/libsvn_subr/io.c (revision 985476)
> +++ subversion/libsvn_subr/io.c (revision 985477)
> @@ -1297,30 +1297,47 @@ reown_file(const char *path,
> static svn_error_t *
> get_default_file_perms(apr_fileperms_t *perms, apr_pool_t *scratch_pool)
> {
> - apr_finfo_t finfo;
> - apr_file_t *fd;
> + /* the default permissions as read from the temp folder */
> + static apr_fileperms_t default_perms = 0;
From the APR documentation, this is just an int32. Hm, you've got rid
of the file description and finfo -- let's see how this works out.
> - /* Get the perms for a newly created file to find out what bits
> - should be set.
> + /* 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;
Ah, so you didn't remove them.
> - NOTE: 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.
> + /* Get the perms for a newly created file to find out what bits
> + should be set.
>
> - NOTE: not so fast, shorty. if some other thread forks off a child,
> - then the APR cleanups run, and the file will disappear. sigh.
> + NOTE: 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.
Bogus diff due to re-indent. The only real addition is "Get the perms
for a newly created file to find out what bits should be set".
> - 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));
> + NOTE: not so fast, shorty. if some other thread forks off a child,
> + then the APR cleanups run, and the file will disappear. sigh.
Ok. You've moved this comment down to say why the del_on_close idea in
the previous comment is a bad idea.
> + *perms = finfo.protection;
> + default_perms = finfo.protection;
Very simple patch. Basically, if the permissions were found in the
last function call, simply return the found value, using a static
variable for remembering it. Otherwise, re-calculate permissions.
-- Ram
Received on 2010-10-01 16:09:47 CEST