Bert Huijben wrote:
>> -----Original Message-----
>> From:stefan2_at_apache.org [mailto:stefan2_at_apache.org]
>> Sent: zondag 15 april 2012 13:21
>> To:commits_at_subversion.apache.org
>> Subject: svn commit: r1326307 - in /subversion/trunk: ./ build/ac-macros/
>> subversion/include/ subversion/include/private/ subversion/libsvn_fs_fs/
>> subversion/libsvn_subr/ subversion/mod_dav_svn/ subversion/svnadmin/
>> subversion/svnserve/ subversion/tests/ subver...
> Partial review for some of the code in svn_io.h
>
>> Author: stefan2
>> Date: Sun Apr 15 11:20:58 2012
>> New Revision: 1326307
>>
>> URL:http://svn.apache.org/viewvc?rev=1326307&view=rev
>> Log:
>> Merge all changes (-r1298521-1326293) from branches/revprop-cache to
>> trunk
>> and resolve minor conflicts.
>>
>> Added:
>> subversion/trunk/subversion/include/private/svn_named_atomic.h
>> - copied unchanged from r1326293, subversion/branches/revprop-
>> cache/subversion/include/private/svn_named_atomic.h
>> subversion/trunk/subversion/libsvn_subr/svn_named_atomic.c
>> - copied unchanged from r1326293, subversion/branches/revprop-
>> cache/subversion/libsvn_subr/svn_named_atomic.c
>> subversion/trunk/subversion/tests/libsvn_subr/named_atomic-test-
>> common.h
>> - copied unchanged from r1326293, subversion/branches/revprop-
>> cache/subversion/tests/libsvn_subr/named_atomic-test-common.h
>> subversion/trunk/subversion/tests/libsvn_subr/named_atomic-test-
>> proc.c
>> - copied unchanged from r1326293, subversion/branches/revprop-
>> cache/subversion/tests/libsvn_subr/named_atomic-test-proc.c
>> subversion/trunk/subversion/tests/libsvn_subr/named_atomic-test.c
>> - copied, changed from r1326293, subversion/branches/revprop-
>> cache/subversion/tests/libsvn_subr/named_atomic-test.c
>> Modified:
>> subversion/trunk/ (props changed)
>> subversion/trunk/build.conf
>> subversion/trunk/build/ac-macros/svn-macros.m4
>> subversion/trunk/configure.ac
>> subversion/trunk/subversion/include/svn_error_codes.h
>> subversion/trunk/subversion/include/svn_fs.h
>> subversion/trunk/subversion/include/svn_io.h
>> subversion/trunk/subversion/libsvn_fs_fs/caching.c
>> subversion/trunk/subversion/libsvn_fs_fs/fs.h
>> subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
>> subversion/trunk/subversion/libsvn_subr/io.c
>> subversion/trunk/subversion/mod_dav_svn/dav_svn.h
>> subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c
>> subversion/trunk/subversion/mod_dav_svn/repos.c
>> subversion/trunk/subversion/svnadmin/main.c
>> subversion/trunk/subversion/svnserve/main.c
>> subversion/trunk/subversion/svnserve/serve.c
>> subversion/trunk/subversion/svnserve/server.h
>> subversion/trunk/subversion/tests/libsvn_repos/repos-test.c
>> subversion/trunk/subversion/tests/libsvn_subr/ (props changed)
>> subversion/trunk/subversion/tests/svn_test.h
>>
>> Propchange: subversion/trunk/
>> ------------------------------------------------------------------------------
>> Merged /subversion/branches/revprop-cache:r1298521-1326293
>>
>> Modified: subversion/trunk/subversion/include/svn_io.h
>> URL:
>> http://svn.apache.org/viewvc/subversion/trunk/subversion/include/svn_io.
>> h?rev=1326307&r1=1326306&r2=1326307&view=diff
>> ==========================================================
>> ====================
>> --- subversion/trunk/subversion/include/svn_io.h (original)
>> +++ subversion/trunk/subversion/include/svn_io.h Sun Apr 15 11:20:58 2012
>> @@ -682,6 +682,39 @@ svn_io_file_lock2(const char *lock_file,
>> svn_boolean_t exclusive,
>> svn_boolean_t nonblocking,
>> apr_pool_t *pool);
>> +
>> +/**
>> + * Lock the file @a lockfile_handle. If @a exclusive is TRUE,
>> + * obtain exclusive lock, otherwise obtain shared lock.
>> + *
>> + * If @a nonblocking is TRUE, do not wait for the lock if it
>> + * is not available: throw an error instead.
>> + *
>> + * Lock will be automatically released when @a pool is cleared or destroyed.
>> + * You may also explicitly call @ref svn_io_unlock_open_file.
>> + * Use @a pool for memory allocations. @a pool must be the pool that
>> + * @a lockfile_handle has been created in or one of its sub-pools.
>> + *
>> + * @since New in 1.8.
>> + */
>> +svn_error_t *
>> +svn_io_lock_open_file(apr_file_t *lockfile_handle,
>> + svn_boolean_t exclusive,
>> + svn_boolean_t nonblocking,
>> + apr_pool_t *pool);
> What does a nonexclusive shared lock 'lock'?
>
> What does it block? What does it allow?
>
> Does it just block exclusive locks? Or does it block writers?
The same thing as in svn_io_file_lock2() ;)
I haven't looked at the implementation but
my guess is that it will prevent exclusive locks.
>> +
>> +/**
>> + * Unlock the file @a lockfile_handle.
>> + *
>> + * Use @a pool for memory allocations. @a pool must be the pool that
>> + * @a lockfile_handle has been created in or one of its sub-pools.
>> + *
>> + * @since New in 1.8.
>> + */
>> +svn_error_t *
>> +svn_io_unlock_open_file(apr_file_t *lockfile_handle,
>> + apr_pool_t *pool);
>> +
>> /**
>> * Flush any unwritten data from @a file to disk. Use @a pool for
>> * memory allocations.
> <snip>
>
>> Modified: subversion/trunk/subversion/libsvn_subr/io.c
>> URL:
>> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/io.
>> c?rev=1326307&r1=1326306&r2=1326307&view=diff
>> ==========================================================
>> ====================
>> --- subversion/trunk/subversion/libsvn_subr/io.c (original)
>> +++ subversion/trunk/subversion/libsvn_subr/io.c Sun Apr 15 11:20:58 2012
>> @@ -1866,29 +1866,23 @@ file_clear_locks(void *arg)
>> #endif
>>
>> svn_error_t *
>> -svn_io_file_lock2(const char *lock_file,
>> - svn_boolean_t exclusive,
>> - svn_boolean_t nonblocking,
>> - apr_pool_t *pool)
>> +svn_io_lock_open_file(apr_file_t *lockfile_handle,
>> + svn_boolean_t exclusive,
>> + svn_boolean_t nonblocking,
>> + apr_pool_t *pool)
>> {
>> int locktype = APR_FLOCK_SHARED;
>> - apr_file_t *lockfile_handle;
>> - apr_int32_t flags;
>> apr_status_t apr_err;
>> + const char *fname;
>>
>> if (exclusive)
>> locktype = APR_FLOCK_EXCLUSIVE;
>> -
>> - flags = APR_READ;
>> - if (locktype == APR_FLOCK_EXCLUSIVE)
>> - flags |= APR_WRITE;
>> -
>> if (nonblocking)
>> locktype |= APR_FLOCK_NONBLOCK;
>>
>> - SVN_ERR(svn_io_file_open(&lockfile_handle, lock_file, flags,
>> - APR_OS_DEFAULT,
>> - pool));
>> + /* We need this only in case of an error but this is cheap to get -
>> + * so we do it here for clarity. */
>> + apr_err = apr_file_name_get(&fname, lockfile_handle);
> fname here is path encoded, not necessary utf-8 encoded.
>> /* Get lock on the filehandle. */
>> apr_err = apr_file_lock(lockfile_handle, locktype);
>> @@ -1915,11 +1909,11 @@ svn_io_file_lock2(const char *lock_file,
>> case APR_FLOCK_SHARED:
>> return svn_error_wrap_apr(apr_err,
>> _("Can't get shared lock on file '%s'"),
>> - svn_dirent_local_style(lock_file, pool));
>> + svn_dirent_local_style(fname, pool));
>> case APR_FLOCK_EXCLUSIVE:
>> return svn_error_wrap_apr(apr_err,
>> _("Can't get exclusive lock on file '%s'"),
>> - svn_dirent_local_style(lock_file, pool));
>> + svn_dirent_local_style(fname, pool));
> So it can't just be used in error messages like here.
>> default:
>> SVN_ERR_MALFUNCTION();
>> }
>> @@ -1936,6 +1930,58 @@ svn_io_file_lock2(const char *lock_file,
>> return SVN_NO_ERROR;
>> }
>>
>> +svn_error_t *
>> +svn_io_unlock_open_file(apr_file_t *lockfile_handle,
>> + apr_pool_t *pool)
>> +{
>> + const char *fname;
>> + apr_status_t apr_err = apr_file_unlock(lockfile_handle);
>> +
>> + /* We need this only in case of an error but this is cheap to get -
>> + * so we do it here for clarity. */
>> + apr_err = apr_file_name_get(&fname, lockfile_handle);
>> +
>> +/* On Windows and OS/2 file locks are automatically released when
>> + the file handle closes */
>> +#if !defined(WIN32)&& !defined(__OS2__)
>> + apr_pool_cleanup_kill(pool, lockfile_handle, file_clear_locks);
>> +#endif
>> +
>> + return apr_err
>> + ? svn_error_wrap_apr(apr_err, _("Can't unlock file '%s'"),
>> + svn_dirent_local_style(fname, pool))
> apr_err depends just on the filename getting.
>
> Is failing to obtain the name an unlock error?
Oops. I got the checking order wrong there.
> Is a succeeded obtaining of the name a success of the unlock?
>
> On Windows this function can just return SVN_NO_ERROR, as we don't unlock, so the unlock can't fail. The name obtaining is unrelated. And converting locale for the filename in the error (which can be expensive) should be added for !Windows.
> (There are file local optimized helpers for the path conversions)
>
>> + : SVN_NO_ERROR;
>> +}
>> +
>> +svn_error_t *
>> +svn_io_file_lock2(const char *lock_file,
>> + svn_boolean_t exclusive,
>> + svn_boolean_t nonblocking,
>> + apr_pool_t *pool)
>> +{
>> + int locktype = APR_FLOCK_SHARED;
>> + apr_file_t *lockfile_handle;
>> + apr_int32_t flags;
>> + apr_status_t apr_err;
>> +
>> + if (exclusive)
>> + locktype = APR_FLOCK_EXCLUSIVE;
>> +
>> + flags = APR_READ;
>> + if (locktype == APR_FLOCK_EXCLUSIVE)
>> + flags |= APR_WRITE;
>> +
>> + if (nonblocking)
>> + locktype |= APR_FLOCK_NONBLOCK;
>> +
>> + SVN_ERR(svn_io_file_open(&lockfile_handle, lock_file, flags,
>> + APR_OS_DEFAULT,
>> + pool));
>> +
>> + /* Get lock on the filehandle. */
>> + return svn_io_lock_open_file(lockfile_handle, exclusive, nonblocking,
>> pool);
>> +}
>> +
>>
>>
Thanks for the review! r1328939 should address these issues.
-- Stefan^2.
Received on 2012-04-22 21:16:09 CEST