> -----Original Message-----
> From: Ramkumar Ramachandra [mailto:artagnon_at_gmail.com]
> Sent: maandag 11 oktober 2010 14:54
> To: Bert Huijben
> Cc: dev_at_subversion.apache.org; 'Greg Stein'; 'Julian Foad'
> Subject: Re: svn commit: r1004286 - in /subversion/trunk: ./
> subversion/libsvn_subr/io.c
>
> Hi Bert and Julian,
>
> Bert Huijben writes:
> > > -----Original Message-----
> > > From: Julian Foad [mailto:julian.foad_at_wandisco.com]
> > > On Mon, 2010-10-11, Julian Foad wrote:
> > > > <http://stackoverflow.com/questions/54188/are-c-reads-and-writes-
> of-
> > > an-int-atomic>.
> > > > "On an IA32 a correctly aligned address will be an atomic
> operation.
> > > > [... otherwise... can't assume it is]."
> > >
> > > Sorry, I pressed "Send" too early. That's not the most important
> bit of
> > > information. (That paragraph talks mostly about <= 32-bit CPUs,
> where
> > > of course there will be problems.) Bert explained to me on IRC
> that
> > > atomicity is not guaranteed even on >= 32 bit architectures, and
> the
> > > highest-ranked answer on that web page agrees. I'm no expert in
> this so
> > > I'll go away now.
> >
> > Let me add that calling apr_atomic_set32() instead of the 'x =
> <value>' part of the pattern will fix this issue in the way that was
> documented by the comment in the code: all threads do the same thing
> and one of the results is left in the static variable.
> >
> > Another option is to use an atomic initialization function to
> initialize the value just once.
>
> I see; I don't think I know enough to comment.
> Bert: So does this solve the issue or did I misunderstand something?
>
> Index: subversion/libsvn_subr/io.c
> ===================================================================
> --- subversion/libsvn_subr/io.c (revision 1005706)
> +++ subversion/libsvn_subr/io.c (working copy)
> @@ -1269,7 +1269,8 @@ 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;
> + static apr_fileperms_t default_perms;
> + apr_atomic_set32(&default_perms, 0);
This shouldn't change. This = 0 is applied at application/library load (just
once). After your change it happens at every invocation.
>
> /* Technically, this "racy": Multiple threads may use enter here and
> try to figure out the default permisission concurrently. That's
> fine
> @@ -1303,7 +1304,7 @@ get_default_file_perms(apr_fileperms_t *perms,
> apr
> SVN_ERR(svn_io_file_close(fd, scratch_pool));
>
> *perms = finfo.protection;
> - default_perms = finfo.protection;
> + apr_atomic_set32(&default_perms, finfo.protection);
Yes, assuming default_perms is a 32 bit integer (which I think it always is)
this makes it safe on all platforms. Without this it relies on cpu
architecture, alignment and possibly caching hints.
(As noted by others, without this patch it should be safe on x86 and x64
architectures (where compilers handle the proper alignment for the static
variable). But we can't be sure about the other architectures)
Bert
Received on 2010-10-11 15:40:27 CEST