RE: svn commit: r1004286 - in /subversion/trunk: ./ subversion/libsvn_subr/io.c

On Mon, 2010-10-11 at 11:11 +0200, Bert Huijben wrote:
>
> > -----Original Message-----
> > From: Greg Stein [mailto:gstein_at_gmail.com]
> > Sent: maandag 11 oktober 2010 11:03
> > To: Bert Huijben
> > Cc: dev_at_subversion.apache.org; artagnon_at_apache.org
> > Subject: Re: svn commit: r1004286 - in /subversion/trunk: ./
> > subversion/libsvn_subr/io.c
> >
> > On Mon, Oct 11, 2010 at 04:58, Bert Huijben <bert_at_qqmail.nl> wrote:
> > >> -----Original Message-----
> > >> From: artagnon_at_apache.org [mailto:artagnon_at_apache.org]
> > >> Sent: maandag 4 oktober 2010 17:27
> > >> To: commits_at_subversion.apache.org
> > >> Subject: svn commit: r1004286 - in /subversion/trunk: ./
> > >> subversion/libsvn_subr/io.c
> > >>
> > >> Author: artagnon
> > >> Date: Mon Oct 4 15:26:44 2010
> > >> New Revision: 1004286
> > >>
> > >> URL: http://svn.apache.org/viewvc?rev=1004286&view=rev
> > >> Log:
> > >> Merge r985477 from subversion/branches/performance
> > >>
> > >> * subversion/libsvn_subr/io.c
> > >> (get_default_file_perms): Store the permissions of the created
> > >> temporary file in a static variable and re-use it in subsequent
> > >> calls instead of checking persmissions everytime. This has
> > >> performance benefits.
> > >>
> > >> Review by: artagnon
> > >> Approved by: julianfoad
> > >
> > > Delayed review:
> > >
> > > Shouldn't this function use some 'atomic initialization' handling?
> > >
> > > Currently it uses a static apr_fileperms_t (integer?) which can be
> > manipulated by multiple threads at the same time.
> > >
> > > This part of subversion is a library and inside tools like Subclipse,
> > TortoiseSVN, AnkhSVN and others it is used multithreaded.
> >
> > So what? Aren't all of those threads going to write the exact same
> > value into the variable?
> >
> > And if they *don't*, then I believe we have larger problems.
>
> No, they are taking the value that is being stored there at the same time
> (read: value is undefined) and use that as the umask.
>
> The pattern
> static long x;

> if (x == 0)
> {
> <calculating>
> x = <value>
> }
> <use x>
>
> is not thread safe.

<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]."

- Julian
Received on 2010-10-11 11:34:31 CEST