subversion-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From i...@apache.org
Subject svn commit: r1701298 - in /subversion/trunk/subversion: include/private/svn_io_private.h libsvn_subr/io.c libsvn_subr/stream.c
Date Fri, 04 Sep 2015 17:24:35 GMT
Author: ivan
Date: Fri Sep  4 17:24:34 2015
New Revision: 1701298

URL: http://svn.apache.org/r1701298
Log:
Fix spurious 'Access Denied' errors during checkout or commit on Windows
reported on TortoiseSVN users mailing list [1].

The issue can be reproduced locally even though known problems reports are
with working copies stored on network share. The reproduction script is:
1. Checkout working copy with a file 'foo'
2. Run 'for /L %I in (1,1,50000000) do type foo' command in background to
   emulate indexers/antivirus scanners
3. Modify file 'foo' in repository via another working copy.
4. Update the original working copy with 'type foo' running

The real fix is to add retry loop for SetFileInformationByHandle() call. All
other changes are refactoring to move existing platform specific code from
libsvn_subr/stream.c to libsvn_subr/io.c to use WIN32_RETRY_LOOP macro. We
already have retry loops for almost all IO operations on Windows.

[1] http://tortoisesvn.tigris.org/ds/viewMessage.do?dsForumId=4061&dsMessageId=3134203

* subversion/include/private/svn_io_private.h
  (svn_io__win_delete_file_on_close, svn_io__win_rename_open_file): Declare
   new library private functions.

* subversion/libsvn_subr/io.c
  (FILE_RENAME_INFO, FILE_DISPOSITION_INFO, FileRenameInfo,
   FileDispositionInfo, SetFileInformationByHandle_t,
   set_file_information_by_handle_proc): Move it here from
   libsvn_subr/stream.c.

  (win_init_dynamic_imports): Obtain pointer to SetFileInformationByHandle().

  (win32_set_file_information_by_handle): New helper.

  (svn_io__win_delete_file_on_close): New. Implementation extracted from
   svn_stream__install_delete().

  (svn_io__win_rename_open_file): New. Implementation extracted from
   svn_stream__install_stream(). Retry operation on Access Denied errors.
   
* subversion/libsvn_subr/stream.c
  (FILE_RENAME_INFO, FILE_DISPOSITION_INFO, SetFileInformationByHandle_t):
   Move to libsvn_subr/io.c.

  (SetFileInformationByHandle, SetFileInformationByHandle_a,
   find_SetFileInformationByHandle): Remove.

  (svn_stream__install_stream): Use svn_io__win_rename_open_file().
  
  (svn_stream__install_delete): Use svn_io__win_delete_file_on_close().

Modified:
    subversion/trunk/subversion/include/private/svn_io_private.h
    subversion/trunk/subversion/libsvn_subr/io.c
    subversion/trunk/subversion/libsvn_subr/stream.c

Modified: subversion/trunk/subversion/include/private/svn_io_private.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/private/svn_io_private.h?rev=1701298&r1=1701297&r2=1701298&view=diff
==============================================================================
--- subversion/trunk/subversion/include/private/svn_io_private.h (original)
+++ subversion/trunk/subversion/include/private/svn_io_private.h Fri Sep  4 17:24:34 2015
@@ -171,6 +171,26 @@ svn_error_t*
 svn_io__utf8_to_unicode_longpath(const WCHAR **result,
                                  const char *source,
                                  apr_pool_t *result_pool);
+
+/* This Windows-specific function marks the file to be deleted on close using
+   an existing file handle. It can be used to avoid having to reopen the file
+   as part of the delete handling. Return SVN_ERR_UNSUPPORTED_FEATURE if
+   delete on close operation is not supported by OS. */
+svn_error_t *
+svn_io__win_delete_file_on_close(apr_file_t *file,
+                                 const char *path,
+                                 apr_pool_t *pool);
+
+/* This Windows-specific function renames the file using an existing file
+   handle. It can be used to avoid having to reopen the file as part of the
+   rename operation. Return SVN_ERR_UNSUPPORTED_FEATURE if renaming open
+   file is not supported by OS.*/
+svn_error_t *
+svn_io__win_rename_open_file(apr_file_t *file,
+                             const char *from_path,
+                             const char *to_path,
+                             apr_pool_t *pool);
+
 #endif /* WIN32 */
 
 #ifdef __cplusplus

Modified: subversion/trunk/subversion/libsvn_subr/io.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/io.c?rev=1701298&r1=1701297&r2=1701298&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_subr/io.c (original)
+++ subversion/trunk/subversion/libsvn_subr/io.c Fri Sep  4 17:24:34 2015
@@ -142,6 +142,23 @@
 #endif
 
 #ifdef WIN32
+
+#if _WIN32_WINNT < 0x600 /* Does the SDK assume Windows Vista+? */
+typedef struct _FILE_RENAME_INFO {
+  BOOL   ReplaceIfExists;
+  HANDLE RootDirectory;
+  DWORD  FileNameLength;
+  WCHAR  FileName[1];
+} FILE_RENAME_INFO, *PFILE_RENAME_INFO;
+
+typedef struct _FILE_DISPOSITION_INFO {
+  BOOL DeleteFile;
+} FILE_DISPOSITION_INFO, *PFILE_DISPOSITION_INFO;
+
+#define FileRenameInfo 3
+#define FileDispositionInfo 4
+#endif /* WIN32 < Vista */
+
 /* One-time initialization of the late bound Windows API functions. */
 static volatile svn_atomic_t win_dynamic_imports_state = 0;
 
@@ -152,7 +169,13 @@ typedef DWORD (WINAPI *GETFINALPATHNAMEB
                DWORD cchFilePath,
                DWORD dwFlags);
 
+typedef BOOL (WINAPI *SetFileInformationByHandle_t)(HANDLE hFile,
+                                                    int FileInformationClass,
+                                                    LPVOID lpFileInformation,
+                                                    DWORD dwBufferSize);
+
 static GETFINALPATHNAMEBYHANDLE get_final_path_name_by_handle_proc = NULL;
+static SetFileInformationByHandle_t set_file_information_by_handle_proc = NULL;
 
 /* Forward declaration. */
 static svn_error_t * io_win_read_link(svn_string_t **dest,
@@ -1867,6 +1890,9 @@ static svn_error_t *win_init_dynamic_imp
     {
       get_final_path_name_by_handle_proc = (GETFINALPATHNAMEBYHANDLE)
         GetProcAddress(kernel32, "GetFinalPathNameByHandleW");
+
+      set_file_information_by_handle_proc = (SetFileInformationByHandle_t)
+        GetProcAddress(kernel32, "SetFileInformationByHandle");
     }
 
   return SVN_NO_ERROR;
@@ -1941,6 +1967,120 @@ static svn_error_t * io_win_read_link(sv
       }
 }
 
+/* Wrapper around Windows API function SetFileInformationByHandle() that
+ * returns APR status instead of boolean flag. */
+static apr_status_t
+win32_set_file_information_by_handle(HANDLE hFile,
+                                     int FileInformationClass,
+                                     LPVOID lpFileInformation,
+                                     DWORD dwBufferSize)
+{
+  svn_error_clear(svn_atomic__init_once(&win_dynamic_imports_state,
+                                        win_init_dynamic_imports,
+                                        NULL, NULL));
+
+  if (!set_file_information_by_handle_proc)
+    {
+      return SVN_ERR_UNSUPPORTED_FEATURE;
+    }
+
+  if (!set_file_information_by_handle_proc(hFile, FileInformationClass,
+                                           lpFileInformation,
+                                           dwBufferSize))
+    {
+      return apr_get_os_error();
+    }
+
+  return APR_SUCCESS;
+}
+
+svn_error_t *
+svn_io__win_delete_file_on_close(apr_file_t *file,
+                                 const char *path,
+                                 apr_pool_t *pool)
+{
+  FILE_DISPOSITION_INFO disposition_info;
+  HANDLE hFile;
+  apr_status_t status;
+
+  apr_os_file_get(&hFile, file);
+
+  disposition_info.DeleteFile = TRUE;
+
+  status = win32_set_file_information_by_handle(hFile, FileDispositionInfo,
+                                                &disposition_info,
+                                                sizeof(disposition_info));
+
+  if (status)
+    {
+      return svn_error_wrap_apr(status, _("Can't remove file '%s'"),
+                                svn_dirent_local_style(path, pool));
+    }
+
+  return SVN_NO_ERROR;
+}
+
+svn_error_t *
+svn_io__win_rename_open_file(apr_file_t *file,
+                             const char *from_path,
+                             const char *to_path,
+                             apr_pool_t *pool)
+{
+  WCHAR *w_final_abspath;
+  size_t path_len;
+  size_t rename_size;
+  FILE_RENAME_INFO *rename_info;
+  HANDLE hFile;
+  apr_status_t status;
+
+  apr_os_file_get(&hFile, file);
+
+  SVN_ERR(svn_io__utf8_to_unicode_longpath(
+            &w_final_abspath, svn_dirent_local_style(to_path,pool),
+            pool));
+
+  path_len = wcslen(w_final_abspath);
+  rename_size = sizeof(*rename_info) + sizeof(WCHAR) * path_len;
+
+  /* The rename info struct doesn't need hacks for long paths,
+     so no ugly escaping calls here */
+  rename_info = apr_pcalloc(pool, rename_size);
+  rename_info->ReplaceIfExists = TRUE;
+  rename_info->FileNameLength = path_len;
+  memcpy(rename_info->FileName, w_final_abspath, path_len * sizeof(WCHAR));
+
+  status = win32_set_file_information_by_handle(hFile, FileRenameInfo,
+                                                rename_info,
+                                                rename_size);
+
+  if (APR_STATUS_IS_EACCES(status) || APR_STATUS_IS_EEXIST(status))
+    {
+      /* Set the destination file writable because Windows will not allow
+         us to rename when final_abspath is read-only. */
+      SVN_ERR(svn_io_set_file_read_write(to_path, TRUE, pool));
+
+      status = win32_set_file_information_by_handle(hFile,
+                                                    FileRenameInfo,
+                                                    rename_info,
+                                                    rename_size);
+   }
+
+  WIN32_RETRY_LOOP(status,
+                   win32_set_file_information_by_handle(hFile,
+                                                        FileRenameInfo,
+                                                        rename_info,
+                                                        rename_size));
+
+  if (status)
+    {
+      return svn_error_wrap_apr(status, _("Can't move '%s' to '%s'"),
+                                svn_dirent_local_style(from_path, pool),
+                                svn_dirent_local_style(to_path, pool));
+    }
+
+  return SVN_NO_ERROR;
+}
+
 #endif /* WIN32 */
 
 svn_error_t *

Modified: subversion/trunk/subversion/libsvn_subr/stream.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/stream.c?rev=1701298&r1=1701297&r2=1701298&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_subr/stream.c (original)
+++ subversion/trunk/subversion/libsvn_subr/stream.c Fri Sep  4 17:24:34 2015
@@ -2113,48 +2113,6 @@ struct install_baton_t
 
 #ifdef WIN32
 
-#if _WIN32_WINNT < 0x600 /* Does the SDK assume Windows Vista+? */
-typedef struct _FILE_RENAME_INFO {
-  BOOL   ReplaceIfExists;
-  HANDLE RootDirectory;
-  DWORD  FileNameLength;
-  WCHAR  FileName[1];
-} FILE_RENAME_INFO, *PFILE_RENAME_INFO;
-
-typedef struct _FILE_DISPOSITION_INFO {
-  BOOL DeleteFile;
-} FILE_DISPOSITION_INFO, *PFILE_DISPOSITION_INFO;
-
-#define FileRenameInfo 3
-#define FileDispositionInfo 4
-
-typedef BOOL (WINAPI *SetFileInformationByHandle_t)(HANDLE hFile,
-                                                    int FileInformationClass,
-                                                    LPVOID lpFileInformation,
-                                                    DWORD dwBufferSize);
-
-static volatile SetFileInformationByHandle_t SetFileInformationByHandle_p = 0;
-#define SetFileInformationByHandle (*SetFileInformationByHandle_p)
-
-static volatile svn_atomic_t SetFileInformationByHandle_a = 0;
-
-
-static svn_error_t *
-find_SetFileInformationByHandle(void *baton, apr_pool_t *scratch_pool)
-{
-  HMODULE kernel32 = GetModuleHandle("Kernel32.dll");
-
-  if (kernel32)
-    {
-      SetFileInformationByHandle_p =
-                    (SetFileInformationByHandle_t)
-                      GetProcAddress(kernel32, "SetFileInformationByHandle");
-    }
-
-  return SVN_NO_ERROR;
-}
-#endif /* WIN32 < Vista */
-
 /* Create and open a tempfile in DIRECTORY. Return its handle and path */
 static svn_error_t *
 create_tempfile(HANDLE *hFile,
@@ -2312,98 +2270,40 @@ svn_stream__install_stream(svn_stream_t
 
   SVN_ERR_ASSERT(svn_dirent_is_absolute(final_abspath));
 #ifdef WIN32
-
-#if _WIN32_WINNT < 0x600
-  SVN_ERR(svn_atomic__init_once(&SetFileInformationByHandle_a,
-                                find_SetFileInformationByHandle,
-                                NULL, scratch_pool));
-
-  if (!SetFileInformationByHandle_p)
-    SVN_ERR(svn_io_file_close(ib->baton_apr.file, scratch_pool));
-  else
-#endif /* WIN32 < Windows Vista */
+  err = svn_io__win_rename_open_file(ib->baton_apr.file,  ib->tmp_path,
+                                     final_abspath, scratch_pool);
+  if (make_parents && err && APR_STATUS_IS_ENOENT(err->apr_err))
     {
-      WCHAR *w_final_abspath;
-      size_t path_len;
-      size_t rename_size;
-      FILE_RENAME_INFO *rename_info;
-      HANDLE hFile;
-
-      apr_os_file_get(&hFile, ib->baton_apr.file);
-
-      SVN_ERR(svn_io__utf8_to_unicode_longpath(&w_final_abspath,
-                                               svn_dirent_local_style(
-                                                          final_abspath,
-                                                          scratch_pool),
-                                               scratch_pool));
-      path_len = wcslen(w_final_abspath);
-      rename_size = sizeof(*rename_info) + sizeof(WCHAR) * path_len;
-
-      /* The rename info struct doesn't need hacks for long paths,
-         so no ugly escaping calls here */
-      rename_info = apr_pcalloc(scratch_pool, rename_size);
-      rename_info->ReplaceIfExists = TRUE;
-      rename_info->FileNameLength = path_len;
-      memcpy(rename_info->FileName, w_final_abspath, path_len * sizeof(WCHAR));
+      svn_error_t *err2;
 
-      if (!SetFileInformationByHandle(hFile, FileRenameInfo, rename_info,
-                                      rename_size))
-        {
-          svn_boolean_t retry = FALSE;
-          err = svn_error_wrap_apr(apr_get_os_error(), NULL);
+      err2 = svn_io_make_dir_recursively(svn_dirent_dirname(final_abspath,
+                                                    scratch_pool),
+                                         scratch_pool);
 
-          /* ### rhuijben: I wouldn't be surprised if we later find out that we
-                           have to fall back to close+rename on some specific
-                           error values here, to support some non standard NAS
-                           and filesystem scenarios. */
-
-          if (make_parents && err && APR_STATUS_IS_ENOENT(err->apr_err))
-            {
-              svn_error_t *err2;
+      if (err2)
+        return svn_error_trace(svn_error_compose_create(err, err2));
+      else
+        svn_error_clear(err);
 
-              err2 = svn_io_make_dir_recursively(svn_dirent_dirname(final_abspath,
-                                                            scratch_pool),
-                                                 scratch_pool);
+      err = svn_io__win_rename_open_file(ib->baton_apr.file, ib->tmp_path,
+                                         final_abspath, scratch_pool);
+    }
 
-              if (err2)
-                return svn_error_trace(svn_error_compose_create(err, err2));
-              else
-                svn_error_clear(err);
-
-              retry = TRUE;
-              err = NULL;
-            }
-          else if (err && (APR_STATUS_IS_EACCES(err->apr_err)
-                           || APR_STATUS_IS_EEXIST(err->apr_err)))
-            {
-              svn_error_clear(err);
-              retry = TRUE;
-              err = NULL;
-
-              /* Set the destination file writable because Windows will not allow
-                 us to rename when final_abspath is read-only. */
-              SVN_ERR(svn_io_set_file_read_write(final_abspath, TRUE,
-                                                 scratch_pool));
-            }
-
-          if (retry)
-            {
-              if (!SetFileInformationByHandle(hFile, FileRenameInfo,
-                                              rename_info, rename_size))
-                {
-                  err = svn_error_wrap_apr(
-                                apr_get_os_error(),
-                                _("Can't move '%s' to '%s'"),
-                                svn_dirent_local_style(ib->tmp_path,
-                                                       scratch_pool),
-                                svn_dirent_local_style(final_abspath,
-                                                       scratch_pool));
-                }
-            }
-        }
-      else
-        err = NULL;
+  /* ### rhuijben: I wouldn't be surprised if we later find out that we
+                   have to fall back to close+rename on some specific
+                   error values here, to support some non standard NAS
+                   and filesystem scenarios. */
+  if (err && err->apr_err == SVN_ERR_UNSUPPORTED_FEATURE)
+    {
+      /* Rename open files is not supported on this platform: fallback to
+         svn_io_file_rename2(). */
+      svn_error_clear(err);
+      err = SVN_NO_ERROR;
 
+      SVN_ERR(svn_io_file_close(ib->baton_apr.file, scratch_pool));
+    }
+  else
+    {
       return svn_error_compose_create(err,
                                       svn_io_file_close(ib->baton_apr.file,
                                                         scratch_pool));
@@ -2467,37 +2367,22 @@ svn_stream__install_delete(svn_stream_t
   struct install_baton_t *ib = install_stream->baton;
 
 #ifdef WIN32
-  BOOL done;
-
-#if _WIN32_WINNT < 0x600
-
-  SVN_ERR(svn_atomic__init_once(&SetFileInformationByHandle_a,
-                                find_SetFileInformationByHandle,
-                                NULL, scratch_pool));
+  svn_error_t *err;
 
-  if (!SetFileInformationByHandle_p)
-    done = FALSE;
-  else
-#endif /* WIN32 < Windows Vista */
+  /* Mark the file as delete on close to avoid having to reopen
+     the file as part of the delete handling. */
+  err = svn_io__win_delete_file_on_close(ib->baton_apr.file,  ib->tmp_path,
+                                         scratch_pool);
+  if (err == SVN_NO_ERROR)
     {
-      FILE_DISPOSITION_INFO disposition_info;
-      HANDLE hFile;
-
-      apr_os_file_get(&hFile, ib->baton_apr.file);
-
-      disposition_info.DeleteFile = TRUE;
-
-      /* Mark the file as delete on close to avoid having to reopen
-         the file as part of the delete handling. */
-      done = SetFileInformationByHandle(hFile, FileDispositionInfo,
-                                        &disposition_info,
-                                        sizeof(disposition_info));
+      SVN_ERR(svn_io_file_close(ib->baton_apr.file, scratch_pool));
+      return SVN_NO_ERROR; /* File is already gone */
     }
 
+  /* Deleting file on close may be unsupported, so ignore errors and
+     fallback to svn_io_remove_file2(). */
+  svn_error_clear(err);
   SVN_ERR(svn_io_file_close(ib->baton_apr.file, scratch_pool));
-
-  if (done)
-    return SVN_NO_ERROR; /* File is already gone */
 #endif
 
   return svn_error_trace(svn_io_remove_file2(ib->tmp_path, FALSE,



Mime
View raw message