subversion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Stefan Fuhrmann <eq...@web.de>
Subject Re: 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/ subve
Date Sun, 22 Apr 2012 19:15:32 GMT
Bert Huijben wrote:
>> -----Original Message-----
>> From:stefan2@apache.org  [mailto:stefan2@apache.org]
>> Sent: zondag 15 april 2012 13:21
>> To:commits@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.

Mime
View raw message