Opened 11 years ago

Closed 11 years ago

#3975 closed Enhancement (fixed)

tr_bencToFile() contains unnecessary calls to stat() and unlink()

Reported by: jordan Owned by: jordan
Priority: Normal Milestone: 2.20
Component: libtransmission Version: 2.13
Severity: Minor Keywords:
Cc:

Description

When saving a tr_benc object to disk at $path, we first save it to a temp file in the same directory as $path. We check to see if $path exists, and if it does, we unlink it before calling rename().

however, rename() is guaranteed to atomically remove the old file and replace it with the new one. Our test-and-unlink steps are unnecessary.

Change History (8)

comment:1 Changed 11 years ago by jordan

  • Status changed from new to assigned

comment:2 Changed 11 years ago by jordan

  • Resolution set to fixed
  • Status changed from assigned to closed

r11819 libtransmission/bencode.c:

#3975: "tr_bencToFile() contains unnecessary calls to stat() and unlink()" -- fixed.

When saving a tr_benc object to disk at $dst, bencode.c saves it to a tmp file in the same directory as $dst, unlinks $dst if it exists, and then renames $tmp as $dst. This commit removes the middle step, which is unnecessary because rename() has guarantees about atomically overwriting $dst.

comment:3 Changed 11 years ago by rb07

  • Resolution fixed deleted
  • Status changed from closed to reopened

This change to rename()'s use doesn't work on MinGW, the call returns "File exists".

Is this too ugly?

Index: libtransmission/bencode.c
===================================================================
--- libtransmission/bencode.c   (revision 11833)
+++ libtransmission/bencode.c   (working copy)
@@ -1707,7 +1707,7 @@
             tr_fsync( fd );
             tr_close_file( fd );
 
-            if( !rename( tmp, filename ) )
+            if( !unlink(filename) && !rename( tmp, filename ) )
             {
                 tr_inf( _( "Saved \"%s\"" ), filename );
             }

comment:4 Changed 11 years ago by jordan

Hm, it would be nice to add an explanation of why the unlink() is there. This patch takes more space than yours does, but through the magic of inlining it shouldn't have any extra expense... what do you think of this:

Index: libtransmission/bencode.c
===================================================================
--- libtransmission/bencode.c	(revision 11833)
+++ libtransmission/bencode.c	(working copy)
@@ -1657,6 +1657,16 @@
 #endif
 }
 
+static inline int
+tr_rename( const char * oldpath, const char * newpath )
+{
+#ifdef WIN32
+    /* win32's rename() doesn't automatically overwrite the other file... */
+    unlink( newpath );
+#endif
+    return rename( oldpath, newpath );
+}
+
 int
 tr_bencToFile( const tr_benc * top, tr_fmt_mode mode, const char * filename )
 {
@@ -1707,7 +1717,7 @@
             tr_fsync( fd );
             tr_close_file( fd );
 
-            if( !rename( tmp, filename ) )
+            if( !tr_rename( tmp, filename ) )
             {
                 tr_inf( _( "Saved \"%s\"" ), filename );
             }

comment:5 Changed 11 years ago by Elbandi

comment:6 follow-up: Changed 11 years ago by jordan

revision based on Elbandi's feedback. rb07, does this work on Windows?

Index: libtransmission/bencode.c
===================================================================
--- libtransmission/bencode.c	(revision 11833)
+++ libtransmission/bencode.c	(working copy)
@@ -18,6 +18,8 @@
 #include <string.h>
 
 #ifdef WIN32 /* tr_mkstemp() */
+ #define WIN32_LEAN_AND_MEAN
+ #include <windows.h> /* MoveFileEx() */
  #include <fcntl.h>
  #define _S_IREAD 256
  #define _S_IWRITE 128
@@ -1707,7 +1709,11 @@
             tr_fsync( fd );
             tr_close_file( fd );
 
+#ifdef WIN32
+            if( MoveFileEx( tmp, filename, MOVEFILE_REPLACE_EXISTING ) )
+#else
             if( !rename( tmp, filename ) )
+#endif
             {
                 tr_inf( _( "Saved \"%s\"" ), filename );
             }

comment:7 in reply to: ↑ 6 Changed 11 years ago by rb07

Replying to jordan:

revision based on Elbandi's feedback. rb07, does this work on Windows?

You don't need the first 2 changes (#define, #include), the rest works fine.

comment:8 Changed 11 years ago by jordan

  • Resolution set to fixed
  • Status changed from reopened to closed

Thanks rb07!

jordan * r11838 libtransmission/bencode.c:

(trunk libT) #3975 "tr_bencToFile() contains unnecessary calls to stat() and unlink()" -- fixed for win32.

remove() doesn't have the same behavior on Windows. On that platform, we should use

MoveFileEx( oldpath, newpath, MOVEFILE_REPLACE_EXISTING )

Thanks to rb07 for testing & confirming the fix.

Note: See TracTickets for help on using tickets.