Just a few comments on the implementation from me...
Daniel Shahaf wrote:
> 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;
I understand that 'perms_init_state' needs to be marked 'volatile'
because that's a requirement of the 'init-once' function. But why does
'default_perms' need to be volatile now that it is guaranteed to be
initialized exactly once? (I don't think it does need to be.)
If it *is* going to be marked 'volatile' then please don't cast away
that qualifier when you access it. (See below.)
But I don't think it wants to be global anyway. (See below.)
> > +
> > +/* 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));
Please don't cast away the 'volatile' qualifier here. Even if it's
guaranteed to be correct (which I'm not sure about) it's confusing.
Instead, put '&default_perms' into a struct and pass the struct as the
baton.
> > + *perms = default_perms;
Within this function, the 'default_perms' variable could quite happily
be a local (static) variable. By making it a global variable, you're
asking this function to return the information in two ways at once (the
global and the output parameter), which is bad. Not strictly incorrect,
just bad style. Please choose one or the other. From what I can see,
keeping it local and returning through the output parameter would be
best. Then I think all the other things I mentioned here would be
solved too.
> > 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));
Please don't cast away the 'volatile' qualifier here. Avoid using type
casts as far as possible. I think this code was good as it was.
> > 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.
You can write this as:
(default_perms, perms_init_state): New variables.
and their purpose should be described in their doc-strings if not 100%
obvious.
> >
> > (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.
> > ]]]
- Julian
Received on 2011-01-10 13:15:57 CET