Hi,
after some time spent debugging
https://github.com/geany/geany/issues/605
I think the file saving options are a bit confusing and misleading and should be simplified.
1. Both g_file_set_contents() and g_file_replace_contents() do absolutely the same thing (saving to temporary file, then renaming the file), the only difference is the first uses POSIX calls while the second GIO. The use_gio_unsafe_file_saving name is very confusing because it's not unsafe in any way.
2. It is now possible to set
use_gio_unsafe_file_saving=true use_atomic_file_saving=true
and to users it's unclear which of the options will be actually used (it's the use_atomic_file_saving case because it's checked first in the code).
3. The fallback "ordinary" file saving when
use_gio_unsafe_file_saving=false use_atomic_file_saving=false
doesn't bring any benefit compared to the two above - it's similar to use_atomic_file_saving=true in using POSIX calls, it's just a bit less safe because it writes directly to the file without the renaming. The GIO g_file_replace_contents() recreates the file's permissions/ownership correctly in
https://github.com/GNOME/glib/blob/master/gio/glocalfileoutputstream.c#L734
I would suggest removing the file saving option mentioned in (3) above and have just a single settings option like
use_gio_file_operations
Opinions?
Cheers,
Jiri
Hello,
I would suggest removing the file saving option mentioned in (3) above and have just a single settings option like
use_gio_file_operations
Opinions?
IIRC saving to temp file, then renaming isn't the default, because of concerns since both the rename and setting permissions can fail depending on the filesystem and permissions on the parent directly.
IIRC#2 it's called unsafe because the above issues are even more apparent on remote file systems.
Please correct me if I'm wrong.
Best regards
On Wed, Aug 19, 2015 at 11:52 AM, Thomas Martitz kugel@rockbox.org wrote:
Hello,
I would suggest removing the file saving option mentioned in (3) above and have just a single settings option like
use_gio_file_operations
Opinions?
IIRC saving to temp file, then renaming isn't the default, because of concerns since both the rename and setting permissions can fail depending on the filesystem and permissions on the parent directly.
The use_gio_unsafe_file_saving=true is the default (and it should be kept this way) and it does the renaming.
IIRC#2 it's called unsafe because the above issues are even more apparent on remote file systems.
In my opinion use_atomic_file_saving=true is much worse on remote file systems because it destroys file metadata.
Cheers,
Jiri
On 19 August 2015 at 19:48, Jiří Techet techet@gmail.com wrote:
Hi,
after some time spent debugging
https://github.com/geany/geany/issues/605
I think the file saving options are a bit confusing and misleading and should be simplified.
Agree the usability is poor with several options that interact with each other.
- Both g_file_set_contents() and g_file_replace_contents() do absolutely
the same thing (saving to temporary file, then renaming the file),
No they are not the same, set_contents does not work on file systems where rename is not available or is not allowed over existing files (windows), and it creates a new file with new file metadata (eg permissions), whereas replace_contents is much more complex than set, it tries to compensate for these issues but can do several copies of the data during the attempt, which is very costly on slow remote filesystems and has a bug (see below).
the only difference is the first uses POSIX calls while the second GIO. The use_gio_unsafe_file_saving name is very confusing because it's not unsafe in any way.
It is unsafe due to a long standing bug where a failure deletes the copy of the original data without copying it back to the original file, so you are left with a truncated original file and no backup of the original data. You still have the new data in the Geany buffer of course. But it regularly has caught people out if they closed the buffer after a failure assuming they had the original data safe.
- It is now possible to set
use_gio_unsafe_file_saving=true use_atomic_file_saving=true
and to users it's unclear which of the options will be actually used (it's the use_atomic_file_saving case because it's checked first in the code).
Yeah, an enum not a set of bools would be better, IIUC the current system grew all the options over time rather than being planned. For sure cleaning it up would be good.
- The fallback "ordinary" file saving when
use_gio_unsafe_file_saving=false use_atomic_file_saving=false
doesn't bring any benefit compared to the two above
It just overwrites the file, nothing else, one data transfer, fast, no rename, works on *all* filesystems, keeps the files metadata. Whilst its not in any way "safe" its got the best performance.
- it's similar to
use_atomic_file_saving=true in using POSIX calls, it's just a bit less safe because it writes directly to the file without the renaming. The GIO g_file_replace_contents() recreates the file's permissions/ownership correctly
Yes, but its part of the complexity I mentioned above, if it can't re-create the metadata on a new file or it can't do the rename then it copies the original file data, truncates the original file and writes to it directly, so it keeps the metadata. It is supposed to put the original data back on a failure to be as safe as the rename system, but it doesn't, which is where the "unsafe" comes in.
If this has been fixed in a version of GIO that matches the oldest Glib/GTK we use then the set_contents can be removed and replace_contents used in its place, but I am not aware of it having been done.
in
https://github.com/GNOME/glib/blob/master/gio/glocalfileoutputstream.c#L734
I would suggest removing the file saving option mentioned in (3) above and have just a single settings option like
use_gio_file_operations
Opinions?
Whilst the usability can definitely be improved, each of the three options has different characteristics and fits a different use-case. Since I only ever use reliable local native filesystems I would be happy with the simple overwrite method, but I acknowledge that others have differing needs.
Cheers,
Jiri
Devel mailing list Devel@lists.geany.org https://lists.geany.org/cgi-bin/mailman/listinfo/devel
On Wed, Aug 19, 2015 at 12:43 PM, Lex Trotman elextr@gmail.com wrote:
On 19 August 2015 at 19:48, Jiří Techet techet@gmail.com wrote:
Hi,
after some time spent debugging
https://github.com/geany/geany/issues/605
I think the file saving options are a bit confusing and misleading and should be simplified.
Agree the usability is poor with several options that interact with each other.
- Both g_file_set_contents() and g_file_replace_contents() do absolutely
the same thing (saving to temporary file, then renaming the file),
No they are not the same, set_contents does not work on file systems where rename is not available or is not allowed over existing files (windows), and it creates a new file with new file metadata (eg permissions), whereas replace_contents is much more complex than set, it tries to compensate for these issues but can do several copies of the data during the attempt, which is very costly on slow remote filesystems and has a bug (see below).
I meant "same" in the sense they both should be safe. Yes, you are right, g_file_replace_contents() tries to mitigate the problems of g_file_set_contents(). By the way g_file_set_contents() works on Windows, it just isn't atomic:
https://github.com/bratsche/glib/blob/master/glib/gfileutils.c#L1087
the only difference is the first uses POSIX calls while the second GIO. The use_gio_unsafe_file_saving name is very confusing because it's not
unsafe in
any way.
It is unsafe due to a long standing bug where a failure deletes the copy of the original data without copying it back to the original file, so you are left with a truncated original file and no backup of the original data. You still have the new data in the Geany buffer of course. But it regularly has caught people out if they closed the buffer after a failure assuming they had the original data safe.
Uh, just checked the code and it really seems to be the case. Might fix that if I have time.
- It is now possible to set
use_gio_unsafe_file_saving=true use_atomic_file_saving=true
and to users it's unclear which of the options will be actually used
(it's
the use_atomic_file_saving case because it's checked first in the code).
Yeah, an enum not a set of bools would be better, IIUC the current system grew all the options over time rather than being planned. For sure cleaning it up would be good.
- The fallback "ordinary" file saving when
use_gio_unsafe_file_saving=false use_atomic_file_saving=false
doesn't bring any benefit compared to the two above
It just overwrites the file, nothing else, one data transfer, fast, no rename, works on *all* filesystems, keeps the files metadata. Whilst its not in any way "safe" its got the best performance.
OK, might make sense to keep it then.
Jiri
On 19 August 2015 at 23:39, Jiří Techet techet@gmail.com wrote:
On Wed, Aug 19, 2015 at 12:43 PM, Lex Trotman elextr@gmail.com wrote:
On 19 August 2015 at 19:48, Jiří Techet techet@gmail.com wrote:
Hi,
after some time spent debugging
https://github.com/geany/geany/issues/605
I think the file saving options are a bit confusing and misleading and should be simplified.
Agree the usability is poor with several options that interact with each other.
- Both g_file_set_contents() and g_file_replace_contents() do
absolutely the same thing (saving to temporary file, then renaming the file),
No they are not the same, set_contents does not work on file systems where rename is not available or is not allowed over existing files (windows), and it creates a new file with new file metadata (eg permissions), whereas replace_contents is much more complex than set, it tries to compensate for these issues but can do several copies of the data during the attempt, which is very costly on slow remote filesystems and has a bug (see below).
I meant "same" in the sense they both should be safe. Yes, you are right, g_file_replace_contents() tries to mitigate the problems of g_file_set_contents(). By the way g_file_set_contents() works on Windows, it just isn't atomic:
https://github.com/bratsche/glib/blob/master/glib/gfileutils.c#L1087
And its wrong. The choice is made at compile time for the platform Glib is being compiled for, not at runtime based on the target filesystem. So it is only right for local native disks/partitions of the machine it runs on, and is wrong for example on cifs shares mounted on Linux.
the only difference is the first uses POSIX calls while the second GIO. The use_gio_unsafe_file_saving name is very confusing because it's not unsafe in any way.
It is unsafe due to a long standing bug where a failure deletes the copy of the original data without copying it back to the original file, so you are left with a truncated original file and no backup of the original data. You still have the new data in the Geany buffer of course. But it regularly has caught people out if they closed the buffer after a failure assuming they had the original data safe.
Uh, just checked the code and it really seems to be the case. Might fix that if I have time.
That would be great, but it would still be "some time" before the fix propagates to all machines using GIO so we could not rely on it until then.
- It is now possible to set
use_gio_unsafe_file_saving=true use_atomic_file_saving=true
and to users it's unclear which of the options will be actually used (it's the use_atomic_file_saving case because it's checked first in the code).
Yeah, an enum not a set of bools would be better, IIUC the current system grew all the options over time rather than being planned. For sure cleaning it up would be good.
- The fallback "ordinary" file saving when
use_gio_unsafe_file_saving=false use_atomic_file_saving=false
doesn't bring any benefit compared to the two above
It just overwrites the file, nothing else, one data transfer, fast, no rename, works on *all* filesystems, keeps the files metadata. Whilst its not in any way "safe" its got the best performance.
OK, might make sense to keep it then.
Jiri
Devel mailing list Devel@lists.geany.org https://lists.geany.org/cgi-bin/mailman/listinfo/devel
[...]
It is unsafe due to a long standing bug where a failure deletes the copy of the original data without copying it back to the original file, so you are left with a truncated original file and no backup of the original data. You still have the new data in the Geany buffer of course. But it regularly has caught people out if they closed the buffer after a failure assuming they had the original data safe.
Uh, just checked the code and it really seems to be the case. Might fix that if I have time.
That would be great, but it would still be "some time" before the fix propagates to all machines using GIO so we could not rely on it until then.
If the write of the new data to the file failed, I wonder if the copy of the old data back would work? But worst case it should at least leave the copy around.
- It is now possible to set
use_gio_unsafe_file_saving=true use_atomic_file_saving=true
and to users it's unclear which of the options will be actually used (it's the use_atomic_file_saving case because it's checked first in the code).
Yeah, an enum not a set of bools would be better, IIUC the current system grew all the options over time rather than being planned. For sure cleaning it up would be good.
- The fallback "ordinary" file saving when
use_gio_unsafe_file_saving=false use_atomic_file_saving=false
doesn't bring any benefit compared to the two above
It just overwrites the file, nothing else, one data transfer, fast, no rename, works on *all* filesystems, keeps the files metadata. Whilst its not in any way "safe" its got the best performance.
OK, might make sense to keep it then.
Jiri
Devel mailing list Devel@lists.geany.org https://lists.geany.org/cgi-bin/mailman/listinfo/devel