You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by ar...@apache.org on 2010/10/04 17:26:45 UTC
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
Modified:
subversion/trunk/ (props changed)
subversion/trunk/subversion/libsvn_subr/io.c
Propchange: subversion/trunk/
------------------------------------------------------------------------------
--- svn:mergeinfo (original)
+++ svn:mergeinfo Mon Oct 4 15:26:44 2010
@@ -23,7 +23,7 @@
/subversion/branches/log-g-performance:870941-871032
/subversion/branches/merge-skips-obstructions:874525-874615
/subversion/branches/nfc-nfd-aware-client:870276,870376
-/subversion/branches/performance:983764,983766,984927,985014,985037,985046,985669,987893,995507,995603,1001413
+/subversion/branches/performance:983764,983766,984927,985014,985037,985046,985477,985669,987893,995507,995603,1001413
/subversion/branches/ra_serf-digest-authn:875693-876404
/subversion/branches/reintegrate-improvements:873853-874164
/subversion/branches/subtree-mergeinfo:876734-878766
Modified: subversion/trunk/subversion/libsvn_subr/io.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/io.c?rev=1004286&r1=1004285&r2=1004286&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_subr/io.c (original)
+++ subversion/trunk/subversion/libsvn_subr/io.c Mon Oct 4 15:26:44 2010
@@ -1268,30 +1268,46 @@ 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;
+
+ /* 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.
+ /* Get the perms for a newly created file to find out what bits
+ should be set.
- 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.
-
- NOTE: not so fast, shorty. if some other thread forks off a child,
- then the APR cleanups run, and the file will disappear. sigh.
-
- 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));
+ 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;
- *perms = finfo.protection;
return SVN_NO_ERROR;
}
Re: svn commit: r1004286 - in /subversion/trunk: ./ subversion/libsvn_subr/io.c
Posted by Philip Martin <ph...@wandisco.com>.
Ramkumar Ramachandra <ar...@gmail.com> writes:
> Hi Philip,
>
> Philip Martin writes:
>> Most of the functions don't share any state between threads, so they
>> have no atomic issues. svn_io_temp_dir is one of the few that does,
>> so it uses svn_atomic__init_once. I think the file perms stuff should
>> also use it.
>
> I see. Could you point me to some functions that share some state
> between threads or some sort of manual? I want to see how to do it
> right- I'm not satisfied with my current understanding.
Generally it would be functions that access a static variable, or that
make calls to library initialisation functions.
--
Philip
Re: svn commit: r1004286 - in /subversion/trunk: ./ subversion/libsvn_subr/io.c
Posted by Philip Martin <ph...@wandisco.com>.
Ramkumar Ramachandra <ar...@gmail.com> writes:
> Hi Philip,
>
> Philip Martin writes:
>> Most of the functions don't share any state between threads, so they
>> have no atomic issues. svn_io_temp_dir is one of the few that does,
>> so it uses svn_atomic__init_once. I think the file perms stuff should
>> also use it.
>
> I see. Could you point me to some functions that share some state
> between threads or some sort of manual? I want to see how to do it
> right- I'm not satisfied with my current understanding.
Generally it would be functions that access a static variable, or that
make calls to library initialisation functions.
--
Philip
Re: svn commit: r1004286 - in /subversion/trunk: ./
subversion/libsvn_subr/io.c
Posted by Ramkumar Ramachandra <ar...@gmail.com>.
Hi Philip,
Philip Martin writes:
> Most of the functions don't share any state between threads, so they
> have no atomic issues. svn_io_temp_dir is one of the few that does,
> so it uses svn_atomic__init_once. I think the file perms stuff should
> also use it.
I see. Could you point me to some functions that share some state
between threads or some sort of manual? I want to see how to do it
right- I'm not satisfied with my current understanding.
-- Ram
Re: svn commit: r1004286 - in /subversion/trunk: ./
subversion/libsvn_subr/io.c
Posted by Ramkumar Ramachandra <ar...@gmail.com>.
Hi Philip,
Philip Martin writes:
> Most of the functions don't share any state between threads, so they
> have no atomic issues. svn_io_temp_dir is one of the few that does,
> so it uses svn_atomic__init_once. I think the file perms stuff should
> also use it.
I see. Could you point me to some functions that share some state
between threads or some sort of manual? I want to see how to do it
right- I'm not satisfied with my current understanding.
-- Ram
Re: svn commit: r1004286 - in /subversion/trunk: ./ subversion/libsvn_subr/io.c
Posted by Philip Martin <ph...@wandisco.com>.
Ramkumar Ramachandra <ar...@gmail.com> writes:
> Hi Philip,
>
> [sorry about the delayed reply; was ill]
>
> Philip Martin writes:
>> Ramkumar Ramachandra <ar...@gmail.com> writes:
>> > Hm. I read up a little more about this, but what confuses me is-
>> > shouldn't the rest of the code already be needing this?
>>
>> I don't understand your questions. To what does "rest of the code"
>> refer?
>
> What I meant is that the rest of the functions in io.c should also
> have to handle atomicity. I see svn_io_temp_dir using
> svn_atomic__init_once for example.
Most of the functions don't share any state between threads, so they
have no atomic issues. svn_io_temp_dir is one of the few that does,
so it uses svn_atomic__init_once. I think the file perms stuff should
also use it.
--
Philip
Re: svn commit: r1004286 - in /subversion/trunk: ./ subversion/libsvn_subr/io.c
Posted by Philip Martin <ph...@wandisco.com>.
Ramkumar Ramachandra <ar...@gmail.com> writes:
> Hi Philip,
>
> [sorry about the delayed reply; was ill]
>
> Philip Martin writes:
>> Ramkumar Ramachandra <ar...@gmail.com> writes:
>> > Hm. I read up a little more about this, but what confuses me is-
>> > shouldn't the rest of the code already be needing this?
>>
>> I don't understand your questions. To what does "rest of the code"
>> refer?
>
> What I meant is that the rest of the functions in io.c should also
> have to handle atomicity. I see svn_io_temp_dir using
> svn_atomic__init_once for example.
Most of the functions don't share any state between threads, so they
have no atomic issues. svn_io_temp_dir is one of the few that does,
so it uses svn_atomic__init_once. I think the file perms stuff should
also use it.
--
Philip
Re: svn commit: r1004286 - in /subversion/trunk: ./
subversion/libsvn_subr/io.c
Posted by Ramkumar Ramachandra <ar...@gmail.com>.
Hi Philip,
[sorry about the delayed reply; was ill]
Philip Martin writes:
> Ramkumar Ramachandra <ar...@gmail.com> writes:
> > Hm. I read up a little more about this, but what confuses me is-
> > shouldn't the rest of the code already be needing this?
>
> I don't understand your questions. To what does "rest of the code"
> refer?
What I meant is that the rest of the functions in io.c should also
have to handle atomicity. I see svn_io_temp_dir using
svn_atomic__init_once for example.
> > Why are we re-thinking everything from scratch?
>
> To what does "everything" refer?
Frankly, I don't understand this too well. Could the author of io.c
(or other similar files) please correct this?
-- Ram
Re: svn commit: r1004286 - in /subversion/trunk: ./
subversion/libsvn_subr/io.c
Posted by Ramkumar Ramachandra <ar...@gmail.com>.
Hi Philip,
[sorry about the delayed reply; was ill]
Philip Martin writes:
> Ramkumar Ramachandra <ar...@gmail.com> writes:
> > Hm. I read up a little more about this, but what confuses me is-
> > shouldn't the rest of the code already be needing this?
>
> I don't understand your questions. To what does "rest of the code"
> refer?
What I meant is that the rest of the functions in io.c should also
have to handle atomicity. I see svn_io_temp_dir using
svn_atomic__init_once for example.
> > Why are we re-thinking everything from scratch?
>
> To what does "everything" refer?
Frankly, I don't understand this too well. Could the author of io.c
(or other similar files) please correct this?
-- Ram
Re: svn commit: r1004286 - in /subversion/trunk: ./ subversion/libsvn_subr/io.c
Posted by Philip Martin <ph...@wandisco.com>.
Ramkumar Ramachandra <ar...@gmail.com> writes:
> Hi Philip,
>
> [sorry about the delayed reply: I had a bad internet connection]
>
> Philip Martin writes:
>> If we are going to use the APR atomic interface then the two reads
>> should use apr_atomic_read32.
>>
>> It would be better to use svn_atomic__init_once. It's a clear
>> indication that we are doing once only initialisation, so we don't
>> need all the comments, and it avoids any problems related to the size
>> of apr_fileperms_t. Also if enhancements are required (more memory
>> barriers say) then svn_atomic__init_once is the place to do it.
>
> Hm. I read up a little more about this, but what confuses me is-
> shouldn't the rest of the code already be needing this?
I don't understand your questions. To what does "rest of the code"
refer?
> Why are we re-thinking everything from scratch?
To what does "everything" refer?
--
Philip
Re: svn commit: r1004286 - in /subversion/trunk: ./ subversion/libsvn_subr/io.c
Posted by Philip Martin <ph...@wandisco.com>.
Ramkumar Ramachandra <ar...@gmail.com> writes:
> Hi Philip,
>
> [sorry about the delayed reply: I had a bad internet connection]
>
> Philip Martin writes:
>> If we are going to use the APR atomic interface then the two reads
>> should use apr_atomic_read32.
>>
>> It would be better to use svn_atomic__init_once. It's a clear
>> indication that we are doing once only initialisation, so we don't
>> need all the comments, and it avoids any problems related to the size
>> of apr_fileperms_t. Also if enhancements are required (more memory
>> barriers say) then svn_atomic__init_once is the place to do it.
>
> Hm. I read up a little more about this, but what confuses me is-
> shouldn't the rest of the code already be needing this?
I don't understand your questions. To what does "rest of the code"
refer?
> Why are we re-thinking everything from scratch?
To what does "everything" refer?
--
Philip
Re: svn commit: r1004286 - in /subversion/trunk: ./
subversion/libsvn_subr/io.c
Posted by Ramkumar Ramachandra <ar...@gmail.com>.
Hi Philip,
[sorry about the delayed reply: I had a bad internet connection]
Philip Martin writes:
> If we are going to use the APR atomic interface then the two reads
> should use apr_atomic_read32.
>
> It would be better to use svn_atomic__init_once. It's a clear
> indication that we are doing once only initialisation, so we don't
> need all the comments, and it avoids any problems related to the size
> of apr_fileperms_t. Also if enhancements are required (more memory
> barriers say) then svn_atomic__init_once is the place to do it.
Hm. I read up a little more about this, but what confuses me is-
shouldn't the rest of the code already be needing this? Why are we
re-thinking everything from scratch?
-- Ram
Re: svn commit: r1004286 - in /subversion/trunk: ./
subversion/libsvn_subr/io.c
Posted by Ramkumar Ramachandra <ar...@gmail.com>.
Hi Philip,
[sorry about the delayed reply: I had a bad internet connection]
Philip Martin writes:
> If we are going to use the APR atomic interface then the two reads
> should use apr_atomic_read32.
>
> It would be better to use svn_atomic__init_once. It's a clear
> indication that we are doing once only initialisation, so we don't
> need all the comments, and it avoids any problems related to the size
> of apr_fileperms_t. Also if enhancements are required (more memory
> barriers say) then svn_atomic__init_once is the place to do it.
Hm. I read up a little more about this, but what confuses me is-
shouldn't the rest of the code already be needing this? Why are we
re-thinking everything from scratch?
-- Ram
Re: svn commit: r1004286 - in /subversion/trunk: ./ subversion/libsvn_subr/io.c
Posted by Philip Martin <ph...@wandisco.com>.
Ramkumar Ramachandra <ar...@gmail.com> writes:
> Right, got it. Can I get a +1 for the following patch?
If we are going to use the APR atomic interface then the two reads
should use apr_atomic_read32.
It would be better to use svn_atomic__init_once. It's a clear
indication that we are doing once only initialisation, so we don't
need all the comments, and it avoids any problems related to the size
of apr_fileperms_t. Also if enhancements are required (more memory
barriers say) then svn_atomic__init_once is the place to do it.
>
> [[[
> * subversion/libsvn_subr/io.c
>
> (get_default_file_perms): The 32-bit integer `default_perms` is only
> really atomic (and hence thread-safe) on x86 and x64 where compilers
> handle the proper alignment for static variables. Handle it in the
> more general case using `apr_atomic_set32`.
>
> Suggested by: Bert
> ]]]
>
> Index: subversion/libsvn_subr/io.c
> ===================================================================
> --- subversion/libsvn_subr/io.c (revision 1005706)
> +++ subversion/libsvn_subr/io.c (working copy)
> @@ -1303,7 +1303,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);
> }
> else
> *perms = default_perms;
>
--
Philip
Re: svn commit: r1004286 - in /subversion/trunk: ./ subversion/libsvn_subr/io.c
Posted by Philip Martin <ph...@wandisco.com>.
Ramkumar Ramachandra <ar...@gmail.com> writes:
> Right, got it. Can I get a +1 for the following patch?
If we are going to use the APR atomic interface then the two reads
should use apr_atomic_read32.
It would be better to use svn_atomic__init_once. It's a clear
indication that we are doing once only initialisation, so we don't
need all the comments, and it avoids any problems related to the size
of apr_fileperms_t. Also if enhancements are required (more memory
barriers say) then svn_atomic__init_once is the place to do it.
>
> [[[
> * subversion/libsvn_subr/io.c
>
> (get_default_file_perms): The 32-bit integer `default_perms` is only
> really atomic (and hence thread-safe) on x86 and x64 where compilers
> handle the proper alignment for static variables. Handle it in the
> more general case using `apr_atomic_set32`.
>
> Suggested by: Bert
> ]]]
>
> Index: subversion/libsvn_subr/io.c
> ===================================================================
> --- subversion/libsvn_subr/io.c (revision 1005706)
> +++ subversion/libsvn_subr/io.c (working copy)
> @@ -1303,7 +1303,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);
> }
> else
> *perms = default_perms;
>
--
Philip
Re: svn commit: r1004286 - in /subversion/trunk: ./
subversion/libsvn_subr/io.c
Posted by Ramkumar Ramachandra <ar...@gmail.com>.
Hi Bert,
Bert Huijben writes:
> > -----Original Message-----
> > From: Ramkumar Ramachandra [mailto:artagnon@gmail.com]
> >
> > 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.
Ok.
> >
> > /* 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)
Right, got it. Can I get a +1 for the following patch?
[[[
* subversion/libsvn_subr/io.c
(get_default_file_perms): The 32-bit integer `default_perms` is only
really atomic (and hence thread-safe) on x86 and x64 where compilers
handle the proper alignment for static variables. Handle it in the
more general case using `apr_atomic_set32`.
Suggested by: Bert
]]]
Index: subversion/libsvn_subr/io.c
===================================================================
--- subversion/libsvn_subr/io.c (revision 1005706)
+++ subversion/libsvn_subr/io.c (working copy)
@@ -1303,7 +1303,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);
}
else
*perms = default_perms;
Re: svn commit: r1004286 - in /subversion/trunk: ./
subversion/libsvn_subr/io.c
Posted by Ramkumar Ramachandra <ar...@gmail.com>.
Hi Bert,
Bert Huijben writes:
> > -----Original Message-----
> > From: Ramkumar Ramachandra [mailto:artagnon@gmail.com]
> >
> > 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.
Ok.
> >
> > /* 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)
Right, got it. Can I get a +1 for the following patch?
[[[
* subversion/libsvn_subr/io.c
(get_default_file_perms): The 32-bit integer `default_perms` is only
really atomic (and hence thread-safe) on x86 and x64 where compilers
handle the proper alignment for static variables. Handle it in the
more general case using `apr_atomic_set32`.
Suggested by: Bert
]]]
Index: subversion/libsvn_subr/io.c
===================================================================
--- subversion/libsvn_subr/io.c (revision 1005706)
+++ subversion/libsvn_subr/io.c (working copy)
@@ -1303,7 +1303,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);
}
else
*perms = default_perms;
RE: svn commit: r1004286 - in /subversion/trunk: ./ subversion/libsvn_subr/io.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Ramkumar Ramachandra [mailto:artagnon@gmail.com]
> Sent: maandag 11 oktober 2010 14:54
> To: Bert Huijben
> Cc: dev@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@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
Re: svn commit: r1004286 - in /subversion/trunk: ./
subversion/libsvn_subr/io.c
Posted by Ramkumar Ramachandra <ar...@gmail.com>.
Hi Bert and Julian,
Bert Huijben writes:
> > -----Original Message-----
> > From: Julian Foad [mailto:julian.foad@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);
/* 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);
}
else
*perms = default_perms;
Re: svn commit: r1004286 - in /subversion/trunk: ./
subversion/libsvn_subr/io.c
Posted by Ramkumar Ramachandra <ar...@gmail.com>.
Hi Bert and Julian,
Bert Huijben writes:
> > -----Original Message-----
> > From: Julian Foad [mailto:julian.foad@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);
/* 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);
}
else
*perms = default_perms;
RE: svn commit: r1004286 - in /subversion/trunk: ./ subversion/libsvn_subr/io.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Julian Foad [mailto:julian.foad@wandisco.com]
> Sent: maandag 11 oktober 2010 11:46
> To: Bert Huijben
> Cc: 'Greg Stein'; dev@subversion.apache.org; artagnon@apache.org
> Subject: RE: svn commit: r1004286 - in /subversion/trunk: ./
> subversion/libsvn_subr/io.c
>
> On Mon, 2010-10-11, Julian Foad wrote:
> > On Mon, 2010-10-11, Bert Huijben wrote:
> > > > -----Original Message-----
> > > > From: Greg Stein [mailto:gstein@gmail.com]
> > > > On Mon, Oct 11, 2010 at 04:58, Bert Huijben <be...@qqmail.nl> wrote:
> > > > >> -----Original Message-----
> > > > >> From: artagnon@apache.org [mailto:artagnon@apache.org]
> > > > >> * 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.
> > > > >
> > > > > 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]."
>
> 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.
Bert
RE: svn commit: r1004286 - in /subversion/trunk: ./
subversion/libsvn_subr/io.c
Posted by Julian Foad <ju...@wandisco.com>.
On Mon, 2010-10-11, Julian Foad wrote:
> On Mon, 2010-10-11, Bert Huijben wrote:
> > > -----Original Message-----
> > > From: Greg Stein [mailto:gstein@gmail.com]
> > > On Mon, Oct 11, 2010 at 04:58, Bert Huijben <be...@qqmail.nl> wrote:
> > > >> -----Original Message-----
> > > >> From: artagnon@apache.org [mailto:artagnon@apache.org]
> > > >> * 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.
> > > >
> > > > 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]."
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.
- Julian
RE: svn commit: r1004286 - in /subversion/trunk: ./
subversion/libsvn_subr/io.c
Posted by Julian Foad <ju...@wandisco.com>.
On Mon, 2010-10-11, Julian Foad wrote:
> On Mon, 2010-10-11, Bert Huijben wrote:
> > > -----Original Message-----
> > > From: Greg Stein [mailto:gstein@gmail.com]
> > > On Mon, Oct 11, 2010 at 04:58, Bert Huijben <be...@qqmail.nl> wrote:
> > > >> -----Original Message-----
> > > >> From: artagnon@apache.org [mailto:artagnon@apache.org]
> > > >> * 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.
> > > >
> > > > 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]."
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.
- Julian
RE: svn commit: r1004286 - in /subversion/trunk: ./
subversion/libsvn_subr/io.c
Posted by Julian Foad <ju...@btopenworld.com>.
On Mon, 2010-10-11 at 11:11 +0200, Bert Huijben wrote:
>
> > -----Original Message-----
> > From: Greg Stein [mailto:gstein@gmail.com]
> > Sent: maandag 11 oktober 2010 11:03
> > To: Bert Huijben
> > Cc: dev@subversion.apache.org; artagnon@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 <be...@qqmail.nl> wrote:
> > >> -----Original Message-----
> > >> From: artagnon@apache.org [mailto:artagnon@apache.org]
> > >> Sent: maandag 4 oktober 2010 17:27
> > >> To: commits@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
RE: svn commit: r1004286 - in /subversion/trunk: ./
subversion/libsvn_subr/io.c
Posted by Julian Foad <ju...@btopenworld.com>.
On Mon, 2010-10-11 at 11:11 +0200, Bert Huijben wrote:
>
> > -----Original Message-----
> > From: Greg Stein [mailto:gstein@gmail.com]
> > Sent: maandag 11 oktober 2010 11:03
> > To: Bert Huijben
> > Cc: dev@subversion.apache.org; artagnon@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 <be...@qqmail.nl> wrote:
> > >> -----Original Message-----
> > >> From: artagnon@apache.org [mailto:artagnon@apache.org]
> > >> Sent: maandag 4 oktober 2010 17:27
> > >> To: commits@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
RE: svn commit: r1004286 - in /subversion/trunk: ./ subversion/libsvn_subr/io.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Greg Stein [mailto:gstein@gmail.com]
> Sent: maandag 11 oktober 2010 11:03
> To: Bert Huijben
> Cc: dev@subversion.apache.org; artagnon@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 <be...@qqmail.nl> wrote:
> >> -----Original Message-----
> >> From: artagnon@apache.org [mailto:artagnon@apache.org]
> >> Sent: maandag 4 oktober 2010 17:27
> >> To: commits@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.
Bert
Re: svn commit: r1004286 - in /subversion/trunk: ./ subversion/libsvn_subr/io.c
Posted by Greg Stein <gs...@gmail.com>.
On Mon, Oct 11, 2010 at 04:58, Bert Huijben <be...@qqmail.nl> wrote:
>> -----Original Message-----
>> From: artagnon@apache.org [mailto:artagnon@apache.org]
>> Sent: maandag 4 oktober 2010 17:27
>> To: commits@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.
Cheers,
-g
Re: svn commit: r1004286 - in /subversion/trunk: ./
subversion/libsvn_subr/io.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Another bug in the baton usage:
Daniel Shahaf wrote on Sat, Jan 08, 2011 at 14:00:04 +0200:
> Ramkumar Ramachandra wrote on Sat, Jan 08, 2011 at 16:06:01 +0530:
> > +/* the default permissions as read from the temp folder */
> > +static volatile apr_fileperms_t default_perms = 0;
> > +static volatile svn_atomic_t perms_init_state;
> > +
> > +/* Helper function to set default permissions. Passed to svn_atomic__init_once */
> > static svn_error_t *
> > +set_default_perms(void *baton, apr_pool_t *scratch_pool)
> > {
> > + apr_fileperms_t *default_perms = (apr_fileperms_t *) baton;
> > ...
> > +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, set_default_perms,
> > + default_perms, scratch_pool));
Re: svn commit: r1004286 - in /subversion/trunk: ./
subversion/libsvn_subr/io.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Daniel Shahaf wrote on Tue, Jan 11, 2011 at 18:34:57 +0200:
> Ramkumar Ramachandra wrote on Tue, Jan 11, 2011 at 21:33:33 +0530:
> > - /* 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));
Why did you remove the comment? (I think it should be retained.)
Re: svn commit: r1004286 - in /subversion/trunk: ./
subversion/libsvn_subr/io.c
Posted by Julian Foad <ju...@wandisco.com>.
On Tue, 2011-01-11, Daniel Shahaf wrote:
> I believe this is semantically correct.
>
> Ramkumar, Julian: is the struct still needed? IIRC the reasons for
> originally introducing (namely the 'volatile' qualifier) it have since
> disappeared.
It's not strictly needed but it's a good way to structure the code
anyway.
Another question: what is this bit of code for?
> + if (*default_perms != 0)
> + /* Nothing to do */
> + return SVN_NO_ERROR;
I thought the function was guaranteed to be called only once, in which
case this would be redundant.
(I stared at this because it makes an assumption that 0 is not a valid
or at least not a likely permissions result. It would still work
correctly if the result was 0, as this is just a speed optimization.)
In fact, this initialization is redundant too, isn't it ...
> 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;
... here?
- Julian
Re: svn commit: r1004286 - in /subversion/trunk: ./
subversion/libsvn_subr/io.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
I believe this is semantically correct.
Ramkumar, Julian: is the struct still needed? IIRC the reasons for
originally introducing (namely the 'volatile' qualifier) it have since
disappeared.
Daniel
Ramkumar Ramachandra wrote on Tue, Jan 11, 2011 at 21:33:33 +0530:
> Hi Julian and Daniel,
>
> Here's another revision after your suggestions on IRC. Thanks.
>
> [[[
> Determine default perms in an elegant thread-safe way, not racily.
>
> * subversion/libsvn_subr/io.c
>
> (default_perms_baton, perms_init_state): New struct, variable.
>
> (get_default_file_perms): Remove all functional code into
> init_default_file_perms, and use apr_atomic__init_once so that code
> is only executed once.
>
> (init_default_file_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.
> ]]]
>
> Index: subversion/libsvn_subr/io.c
> ===================================================================
> --- subversion/libsvn_subr/io.c (revision 1057166)
> +++ subversion/libsvn_subr/io.c (working copy)
> @@ -1300,6 +1300,39 @@ reown_file(const char *path,
> return svn_error_return(svn_io_remove_file2(unique_name, FALSE, pool));
> }
>
> +static volatile svn_atomic_t perms_init_state = 0;
> +struct default_perms_baton
> +{
> + apr_fileperms_t *default_perms;
> +};
> +
> +
> +/* 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 *
> +init_default_file_perms(void *baton, apr_pool_t *scratch_pool)
> +{
> + apr_fileperms_t *default_perms =
> + ((struct default_perms_baton *) baton)->default_perms;
> + apr_finfo_t finfo;
> + apr_file_t *fd;
> +
> + if (*default_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));
> + *default_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
> @@ -1310,46 +1343,13 @@ 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;
> + struct default_perms_baton baton;
>
> - /* 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;
> -
> + baton.default_perms = &default_perms;
> + SVN_ERR(svn_atomic__init_once(&perms_init_state, init_default_file_perms,
> + &baton, scratch_pool));
> + *perms = default_perms;
> return SVN_NO_ERROR;
> }
>
Re: svn commit: r1004286 - in /subversion/trunk: ./
subversion/libsvn_subr/io.c
Posted by Ramkumar Ramachandra <ar...@gmail.com>.
Hi Julian and Daniel,
Here's another revision after your suggestions on IRC. Thanks.
[[[
Determine default perms in an elegant thread-safe way, not racily.
* subversion/libsvn_subr/io.c
(default_perms_baton, perms_init_state): New struct, variable.
(get_default_file_perms): Remove all functional code into
init_default_file_perms, and use apr_atomic__init_once so that code
is only executed once.
(init_default_file_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.
]]]
Index: subversion/libsvn_subr/io.c
===================================================================
--- subversion/libsvn_subr/io.c (revision 1057166)
+++ subversion/libsvn_subr/io.c (working copy)
@@ -1300,6 +1300,39 @@ reown_file(const char *path,
return svn_error_return(svn_io_remove_file2(unique_name, FALSE, pool));
}
+static volatile svn_atomic_t perms_init_state = 0;
+struct default_perms_baton
+{
+ apr_fileperms_t *default_perms;
+};
+
+
+/* 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 *
+init_default_file_perms(void *baton, apr_pool_t *scratch_pool)
+{
+ apr_fileperms_t *default_perms =
+ ((struct default_perms_baton *) baton)->default_perms;
+ apr_finfo_t finfo;
+ apr_file_t *fd;
+
+ if (*default_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));
+ *default_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
@@ -1310,46 +1343,13 @@ 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;
+ struct default_perms_baton baton;
- /* 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;
-
+ baton.default_perms = &default_perms;
+ SVN_ERR(svn_atomic__init_once(&perms_init_state, init_default_file_perms,
+ &baton, scratch_pool));
+ *perms = default_perms;
return SVN_NO_ERROR;
}
Re: svn commit: r1004286 - in /subversion/trunk: ./
subversion/libsvn_subr/io.c
Posted by Ramkumar Ramachandra <ar...@gmail.com>.
Hi Julian and Daniel,
Julian Foad writes:
> Just a few comments on the implementation from me...
Thanks for the wonderful review! I've also included a few of Daniel's
suggestions (on IRC)- I hope I've got it right this time. I'm not sure
the volatile qualifier is necessary either- I'm just using it for
extra safety, making sure that no compiler optimizations mangle it.
[[[
Determine default perms in an elegant thread-safe way, not racily.
* subversion/libsvn_subr/io.c
(default_perms_baton, perms_init_state): New struct, variable.
(get_default_file_perms): Remove all functional code into
init_default_file_perms, and use apr_atomic__init_once so that code
is only executed once.
(init_default_file_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.
]]]
Index: subversion/libsvn_subr/io.c
===================================================================
--- subversion/libsvn_subr/io.c (revision 1057166)
+++ subversion/libsvn_subr/io.c (working copy)
@@ -1300,6 +1300,39 @@ reown_file(const char *path,
return svn_error_return(svn_io_remove_file2(unique_name, FALSE, pool));
}
+static volatile svn_atomic_t perms_init_state = 0;
+struct default_perms_baton
+{
+ volatile apr_fileperms_t *default_perms;
+};
+
+
+/* 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 *
+init_default_file_perms(void *baton, apr_pool_t *scratch_pool)
+{
+ volatile apr_fileperms_t *default_perms =
+ ((struct default_perms_baton *) baton)->default_perms;
+ apr_finfo_t finfo;
+ apr_file_t *fd;
+
+ if (*default_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));
+ *default_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
@@ -1310,46 +1343,13 @@ 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;
+ static volatile apr_fileperms_t default_perms = 0;
+ struct default_perms_baton baton;
- /* 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;
-
+ baton.default_perms = &default_perms;
+ SVN_ERR(svn_atomic__init_once(&perms_init_state, init_default_file_perms,
+ (void *) &baton, scratch_pool));
+ *perms = default_perms;
return SVN_NO_ERROR;
}
Re: svn commit: r1004286 - in /subversion/trunk: ./
subversion/libsvn_subr/io.c
Posted by Julian Foad <ju...@wandisco.com>.
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
Re: svn commit: r1004286 - in /subversion/trunk: ./
subversion/libsvn_subr/io.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
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. */
Re: svn commit: r1004286 - in /subversion/trunk: ./
subversion/libsvn_subr/io.c
Posted by Ramkumar Ramachandra <ar...@gmail.com>.
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. */
Re: svn commit: r1004286 - in /subversion/trunk: ./
subversion/libsvn_subr/io.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Ramkumar Ramachandra wrote on Sat, Jan 08, 2011 at 16:06:01 +0530:
> Hi Daniel,
>
> Daniel Shahaf writes:
> > Ping, this doesn't seem to have been fixed yet?
>
> Thanks for the ping, and sorry about the delay. I think this should
> work -- I'll write a commit message if you think this is okay.
>
> Index: subversion/libsvn_subr/io.c
> ===================================================================
> --- subversion/libsvn_subr/io.c (revision 1056662)
> +++ subversion/libsvn_subr/io.c (working copy)
> @@ -1299,59 +1299,64 @@ reown_file(const char *path,
> 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. */
> +/* the default permissions as read from the temp folder */
> +static volatile apr_fileperms_t default_perms = 0;
> +static volatile svn_atomic_t perms_init_state;
> +
> +/* Helper function to set default permissions. Passed to svn_atomic__init_once */
> static svn_error_t *
> -get_default_file_perms(apr_fileperms_t *perms, apr_pool_t *scratch_pool)
> +set_default_perms(void *baton, apr_pool_t *scratch_pool)
> {
> - /* the default permissions as read from the temp folder */
> - static apr_fileperms_t default_perms = 0;
> + apr_fileperms_t *default_perms = (apr_fileperms_t *) baton;
>
> - /* 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;
> + if (default_perms != 0)
This should dereference default_perms.
> + /* Nothing to do */
> + return SVN_NO_ERROR;
>
> - /* Get the perms for a newly created file to find out what bits
> - should be set.
> + apr_finfo_t finfo;
> + apr_file_t *fd;
>
Declarations after statement. C89 only allows declarations at the top
of a block, please fix.
> +/* 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, set_default_perms,
> + default_perms, scratch_pool));
> + *perms = default_perms;
> + return SVN_NO_ERROR;
+1, this should fix the "DEFAULT_PERMS were being determined racily"
problem.
> +}
> +
> /* OR together permission bits of the file FD and the default permissions
> of a file as determined by get_default_file_perms(). Do temporary
> allocations in SCRATCH_POOL. */
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.
Thanks,
Daniel
Re: svn commit: r1004286 - in /subversion/trunk: ./
subversion/libsvn_subr/io.c
Posted by Ramkumar Ramachandra <ar...@gmail.com>.
Hi Daniel,
Daniel Shahaf writes:
> Ping, this doesn't seem to have been fixed yet?
Thanks for the ping, and sorry about the delay. I think this should
work -- I'll write a commit message if you think this is okay.
Index: subversion/libsvn_subr/io.c
===================================================================
--- subversion/libsvn_subr/io.c (revision 1056662)
+++ subversion/libsvn_subr/io.c (working copy)
@@ -1299,59 +1299,64 @@ reown_file(const char *path,
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. */
+/* the default permissions as read from the temp folder */
+static volatile apr_fileperms_t default_perms = 0;
+static volatile svn_atomic_t perms_init_state;
+
+/* Helper function to set default permissions. Passed to svn_atomic__init_once */
static svn_error_t *
-get_default_file_perms(apr_fileperms_t *perms, apr_pool_t *scratch_pool)
+set_default_perms(void *baton, apr_pool_t *scratch_pool)
{
- /* the default permissions as read from the temp folder */
- static apr_fileperms_t default_perms = 0;
+ apr_fileperms_t *default_perms = (apr_fileperms_t *) baton;
- /* 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;
+ if (default_perms != 0)
+ /* Nothing to do */
+ return SVN_NO_ERROR;
- /* Get the perms for a newly created file to find out what bits
- should be set.
+ apr_finfo_t finfo;
+ apr_file_t *fd;
- 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.
- 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.
+ 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.
- 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));
+ 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.
- *perms = finfo.protection;
- default_perms = finfo.protection;
- }
- else
- *perms = default_perms;
+ 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));
+ default_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
+ 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, set_default_perms,
+ default_perms, scratch_pool));
+ *perms = default_perms;
+ return SVN_NO_ERROR;
+}
+
/* OR together permission bits of the file FD and the default permissions
of a file as determined by get_default_file_perms(). Do temporary
allocations in SCRATCH_POOL. */
Re: svn commit: r1004286 - in /subversion/trunk: ./
subversion/libsvn_subr/io.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Ping, this doesn't seem to have been fixed yet?
Bert Huijben wrote on Mon, Oct 11, 2010 at 10:58:38 +0200:
> > -----Original Message-----
> > From: artagnon@apache.org [mailto:artagnon@apache.org]
> > Sent: maandag 4 oktober 2010 17:27
> > To: commits@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.
>
> Bert
>
>
RE: svn commit: r1004286 - in /subversion/trunk: ./ subversion/libsvn_subr/io.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: artagnon@apache.org [mailto:artagnon@apache.org]
> Sent: maandag 4 oktober 2010 17:27
> To: commits@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.
Bert