[glib/1302-file-set-contents-fsync] gfileutils: make g_file_set_contents() always fsync()



commit fc631fbeac07842fface39b2000d25506c60e87b
Author: Will Thompson <wjt endlessm com>
Date:   Wed Nov 22 11:43:55 2017 +0000

    gfileutils: make g_file_set_contents() always fsync()
    
    Previously, this function only called fsync() if @filename exists and is
    non-empty. This behaviour was introduced when the function was first
    written (6cff88ba18b3bc0d118308f109840cb163dcea03) and shortly
    afterwards (d20a188b1250ab3cf211d684429127d99378e886) respectively, with
    the latter justified as a performance optimisation.
    
    This meant that g_file_set_contents() does not provide the guarantee
    that developers assume it has, namely that after a call and a crash,
    @filename will either contain its previous contents or its new
    @contents. In practice, when it was previously non-existent or empty on
    a bog-standard ext4 filesystem, it would often contain NUL bytes
    matching the @length of @contents, requiring application developers to
    explicitly handle this third case.
    
    Given the documentation includes the word "atomic", we make this
    function provide the guarantee that was previously implied but untrue,
    and document it. If applications require higher performance at the cost
    of correctness, they can open-code the old behaviour, or we can add a
    new function to glib providing weaker guarantees.
    
    https://gitlab.gnome.org/GNOME/glib/issues/1302

 glib/gfileutils.c | 21 +++++++--------------
 1 file changed, 7 insertions(+), 14 deletions(-)
---
diff --git a/glib/gfileutils.c b/glib/gfileutils.c
index 1e7a771a9..cf0a8bc3f 100644
--- a/glib/gfileutils.c
+++ b/glib/gfileutils.c
@@ -1111,16 +1111,14 @@ write_to_temp_file (const gchar  *contents,
 
 #ifdef HAVE_FSYNC
   {
-    struct stat statbuf;
-
     errno = 0;
-    /* If the final destination exists and is > 0 bytes, we want to sync the
+    /* We want to sync the
      * newly written file to ensure the data is on disk when we rename over
      * the destination. Otherwise if we get a system crash we can lose both
      * the new and the old file on some filesystems. (I.E. those that don't
      * guarantee the data is written to the disk before the metadata.)
      */
-    if (g_lstat (dest_file, &statbuf) == 0 && statbuf.st_size > 0 && fsync (fd) != 0)
+    if (fsync (fd) != 0)
       {
         int saved_errno = errno;
         set_file_error (err,
@@ -1173,16 +1171,11 @@ write_to_temp_file (const gchar  *contents,
  *   lists, metadata etc. may be lost. If @filename is a symbolic link,
  *   the link itself will be replaced, not the linked file.
  *
- * - On UNIX, if @filename already exists and is non-empty, and if the system
- *   supports it (via a journalling filesystem or equivalent), the fsync()
- *   call (or equivalent) will be used to ensure atomic replacement: @filename
- *   will contain either its old contents or @contents, even in the face of
- *   system power loss, the disk being unsafely removed, etc.
- *
- * - On UNIX, if @filename does not already exist or is empty, there is a
- *   possibility that system power loss etc. after calling this function will
- *   leave @filename empty or full of NUL bytes, depending on the underlying
- *   filesystem.
+ * - On UNIX, if the filesystem is uncleanly unmounted after a successful call
+ *   to this function, it is guaranteed that @filename will contain either its
+ *   old contents, or @contents. In particular, if @filename did not previously
+ *   exist, following a crash it will either not exist or contain its new
+ *   @contents.
  *
  * - On Windows renaming a file will not remove an existing file with the
  *   new name, so on Windows there is a race condition between the existing


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]