Re: svn commit: r1004286 - in /subversion/trunk: ./ subversion/libsvn_subr/io.c
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: [PATCH] allow svnsync to translate non-UTF-8 log messages to UTF-8 (v. 3)
Perhaps, though, I should use svn_subst_translate_string2() with the encoding set to UTF-8. It would greatly simplify normalize_string(), +1. Just call it with encoding=NULL; using the same API in both branches makes life easier for the next person to revv that API. One thing that worries me about using NULL for ENCODING is that svn_subst_translate_string2() calls svn_utf_cstring_to_utf8() on the string in that case, implying some sort of re-encoding from the native narrow string encoding to UTF-8. Couldn't this be problematic?
Re: [PATCH] allow svnsync to translate non-UTF-8 log messages to UTF-8 (v. 3)
Danny Trebbien wrote on Mon, Jan 10, 2011 at 08:55:35 -0800: Perhaps, though, I should use svn_subst_translate_string2() with the encoding set to UTF-8. It would greatly simplify normalize_string(), +1. Just call it with encoding=NULL; using the same API in both branches makes life easier for the next person to revv that API. One thing that worries me about using NULL for ENCODING is that svn_subst_translate_string2() calls svn_utf_cstring_to_utf8() on the string in that case, implying some sort of re-encoding from the native narrow string encoding to UTF-8. Couldn't this be problematic? svn_subst_translate_string2()'s doc string explicitly blesses passing encoding=NULL, so it's not of our concern in svnsync. I agree, however, that it might be a bug in svn_subst_translate_string2().) Feel free to check how svn_subst_translate_string() handled encoding=NULL and, if needed, submit a patch to svn_subst_translate_string2().
Re: svn commit: r1054701 - in /subversion/trunk/subversion/bindings/javahl/native: CopySources.cpp CreateJ.cpp EnumMapper.cpp ListCallback.cpp Revision.cpp RevisionRange.cpp StatusCallback.cpp org_apa
On Mon, Jan 3, 2011 at 7:34 PM, hwri...@apache.org wrote: Author: hwright Date: Mon Jan 3 18:34:35 2011 New Revision: 1054701 URL: http://svn.apache.org/viewvc?rev=1054701view=rev Log: Fix JavaHL build and test failures introduced in r1054680. * subversion/bindings/javahl/native/CreateJ.cpp, subversion/bindings/javahl/native/StatusCallback.cpp, subversion/bindings/javahl/native/CopySources.cpp, subversion/bindings/javahl/native/Revision.cpp, subversion/bindings/javahl/native/RevisionRange.cpp, subversion/bindings/javahl/native/EnumMapper.cpp, subversion/bindings/javahl/native/org_apache_subversion_javahl_SVNClient.cpp, subversion/bindings/javahl/native/ListCallback.cpp: Update references to moved classes. Modified: subversion/trunk/subversion/bindings/javahl/native/CopySources.cpp subversion/trunk/subversion/bindings/javahl/native/CreateJ.cpp subversion/trunk/subversion/bindings/javahl/native/EnumMapper.cpp subversion/trunk/subversion/bindings/javahl/native/ListCallback.cpp subversion/trunk/subversion/bindings/javahl/native/Revision.cpp subversion/trunk/subversion/bindings/javahl/native/RevisionRange.cpp subversion/trunk/subversion/bindings/javahl/native/StatusCallback.cpp subversion/trunk/subversion/bindings/javahl/native/org_apache_subversion_javahl_SVNClient.cpp snip ... Modified: subversion/trunk/subversion/bindings/javahl/native/CreateJ.cpp URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/bindings/javahl/native/CreateJ.cpp?rev=1054701r1=1054700r2=1054701view=diff == --- subversion/trunk/subversion/bindings/javahl/native/CreateJ.cpp (original) +++ subversion/trunk/subversion/bindings/javahl/native/CreateJ.cpp Mon Jan 3 18:34:35 2011 @@ -31,7 +31,7 @@ #include EnumMapper.h #include RevisionRange.h #include CreateJ.h -#include ../include/org_apache_subversion_javahl_Revision.h +#include ../include/org_apache_subversion_javahl_types_Revision.h Typo/Replace-o? This broke something: WARNING: ..\include\org_apache_subversion_javahl_types_Revision.h header not found, file subversion\bindings\javahl\native\CreateJ.cpp The .h file is still named ../include/org_apache_subversion_javahl_Revision.h Cheers, -- Johan
Re: svn commit: r1054701 - in /subversion/trunk/subversion/bindings/javahl/native: CopySources.cpp CreateJ.cpp EnumMapper.cpp ListCallback.cpp Revision.cpp RevisionRange.cpp StatusCallback.cpp org_apa
On Mon, Jan 10, 2011 at 4:07 PM, Johan Corveleyn jcor...@gmail.com wrote: On Mon, Jan 3, 2011 at 7:34 PM, hwri...@apache.org wrote: Author: hwright Date: Mon Jan 3 18:34:35 2011 New Revision: 1054701 URL: http://svn.apache.org/viewvc?rev=1054701view=rev Log: Fix JavaHL build and test failures introduced in r1054680. * subversion/bindings/javahl/native/CreateJ.cpp, subversion/bindings/javahl/native/StatusCallback.cpp, subversion/bindings/javahl/native/CopySources.cpp, subversion/bindings/javahl/native/Revision.cpp, subversion/bindings/javahl/native/RevisionRange.cpp, subversion/bindings/javahl/native/EnumMapper.cpp, subversion/bindings/javahl/native/org_apache_subversion_javahl_SVNClient.cpp, subversion/bindings/javahl/native/ListCallback.cpp: Update references to moved classes. Modified: subversion/trunk/subversion/bindings/javahl/native/CopySources.cpp subversion/trunk/subversion/bindings/javahl/native/CreateJ.cpp subversion/trunk/subversion/bindings/javahl/native/EnumMapper.cpp subversion/trunk/subversion/bindings/javahl/native/ListCallback.cpp subversion/trunk/subversion/bindings/javahl/native/Revision.cpp subversion/trunk/subversion/bindings/javahl/native/RevisionRange.cpp subversion/trunk/subversion/bindings/javahl/native/StatusCallback.cpp subversion/trunk/subversion/bindings/javahl/native/org_apache_subversion_javahl_SVNClient.cpp snip ... Modified: subversion/trunk/subversion/bindings/javahl/native/CreateJ.cpp URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/bindings/javahl/native/CreateJ.cpp?rev=1054701r1=1054700r2=1054701view=diff == --- subversion/trunk/subversion/bindings/javahl/native/CreateJ.cpp (original) +++ subversion/trunk/subversion/bindings/javahl/native/CreateJ.cpp Mon Jan 3 18:34:35 2011 @@ -31,7 +31,7 @@ #include EnumMapper.h #include RevisionRange.h #include CreateJ.h -#include ../include/org_apache_subversion_javahl_Revision.h +#include ../include/org_apache_subversion_javahl_types_Revision.h Typo/Replace-o? This broke something: No, but there was an omission. I updated build.conf in r1057409 to fix the problem. WARNING: ..\include\org_apache_subversion_javahl_types_Revision.h header not found, file subversion\bindings\javahl\native\CreateJ.cpp The .h file is still named ../include/org_apache_subversion_javahl_Revision.h This is probably left over from the old package. Running 'make clean-javahl' should get rid of it. -Hyrum
