After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 562976 - %gconf.xml atomic update is broken
%gconf.xml atomic update is broken
Status: RESOLVED FIXED
Product: GConf
Classification: Deprecated
Component: gconf
2.16.x
Other All
: Normal minor
: ---
Assigned To: GConf Maintainers
GConf Maintainers
Depends on:
Blocks:
 
 
Reported: 2008-12-02 14:37 UTC by Artem Bityutskiy
Modified: 2008-12-08 15:42 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Sync when saving data as well (830 bytes, patch)
2008-12-03 09:55 UTC, Richard Hult
committed Details | Review

Description Artem Bityutskiy 2008-12-02 14:37:46 UTC
Please describe the problem:
If I do stuff like 'gconftool-2 --set --type=bool /apps/foo true', gconftool-2 is trying to update the '%gconf.xml' file atomically. Namely, it creates a temporary '%gconf.xml.new' file, puts the new contents there, then moves '%gconf.xml.new' to '%gconf.xml'. The idea is right, however the implementation is buggy, because '%gconf.xml.new' is not synchronized before the rename. This means that if power cut happens after re-name, we end up with corrupted '%gconf.xml' anyway.

We actually observe this at our ARM-based device. I think you should just add a 'sync(fd)' call before closing 'tmpfile' in the 'logfile_save()' function (I found it in gconf/gconfd.c).

Steps to reproduce:


Actual results:


Expected results:


Does this happen every time?


Other information:
Comment 1 Artem Bityutskiy 2008-12-02 14:40:23 UTC
Note, we use UBIFS and here is some documentation about how to use synchronization properly, which applies to any Linux FS, not just UBIFS:

http://www.linux-mtd.infradead.org/doc/ubifs.html#L_writeback
Comment 2 Ray Strode [halfline] 2008-12-02 15:11:52 UTC
Do you have a tested patch for this issue?

I assume it's something along the lines of:

       goto out;
     }
 
+  if (fdatasync (fd) < 0)
+    {
+      gconf_log (GCL_WARNING,
+                 _("Could not flush saved state file '%s' to disk: %s"),
+                 tmpfile, g_strerror (errno));
+    }
+
   if (close (fd) < 0)
     {
Comment 3 Artem Bityutskiy 2008-12-02 15:19:27 UTC
Looks right to me.
Comment 4 Artem Bityutskiy 2008-12-02 15:22:33 UTC
Err, no, I do not have a patch. I just noticed the bug and hoped you guys would fix it.
Comment 5 Ray Strode [halfline] 2008-12-02 16:03:59 UTC
I don't have an environment set up with ubifs to test this.  I was hoping you already had a patch in your internal builds that has already been vetted.  If not, we can commit the above.
Comment 6 Ray Strode [halfline] 2008-12-02 16:15:23 UTC
commited
Comment 7 Artem Bityutskiy 2008-12-03 09:06:13 UTC
Well, other guys are doing gconf fixes, and we could wait and I could take the patch when they do it and test it. But their patch is the same anyway. Thank you for the fix.
Comment 8 Richard Hult 2008-12-03 09:55:15 UTC
Created attachment 123861 [details] [review]
Sync when saving data as well

The real cause of issues is when saving the data, not the log file. Attached equally untested patch that syncs the data in the markup backend.
Comment 9 Artem Bityutskiy 2008-12-03 09:58:15 UTC
Well, I guess the log file fix does not hurt anyway.
Comment 10 Ray Strode [halfline] 2008-12-03 14:20:27 UTC
ah, that makes a lot more sense to me.  Can one of you guys test that patch and say if it actually fixes the issue?  
Comment 11 Artem Bityutskiy 2008-12-04 10:53:43 UTC
Richard Hult has reported in our internal bugzilla that his patch fixes the problem with corrupted gconf files for our platform.
Comment 12 Ray Strode [halfline] 2008-12-04 15:02:51 UTC
Sounds good.
Comment 13 Ray Strode [halfline] 2008-12-08 15:42:35 UTC
Commited this with a two small changes

1) fflush() the FILE stream before syncing
2) Use fsync instead of fdatasync (see bug 563401)