[Geany-devel] Changed file saving implementation for systems with GIO

[Colomban Wendling]

Le 06/11/2010 06:28, Lex Trotman a écrit :
> On 6 November 2010 13:30, Colomban Wendling <lists.ban at herbesfolles.org> wrote:
>> Le 06/11/2010 01:58, Lex Trotman a écrit :
>>> On 6 November 2010 10:43, Colomban Wendling <lists.ban at herbesfolles.org> wrote:
>>>> Le 04/11/2010 21:42, Dimitar Zhekov a écrit :
>>>>> On Thu, 4 Nov 2010 17:53:47 +0000
>>>>> Nick Treleaven <nick.treleaven at btinternet.com> wrote:
>>>>>
>>>>>> So it seems that function doesn't handle disk exhaustion safely. (But
>>>>>> this is no worse than before for Geany).
>>>>>
>>>>> Before we had safe or unsafe file saving. Let's keep that choice, shall
>>>>> we? I can write the patch, it's only a few lines.
>>>>>
>>>>> For more than 20 years now, the only safe save is to write the data
>>>>> into a temporary file in the same directory, and then rename it over
>>>>> the target file. Even if the rename fails, you still have the temporary
>>>>> file. That changes the onwership and permissions, which is exactly the
>>>>> behaviour of the safe g_file_set_contents().
>>>>>
>>>>> The documentation of g_file_replace() may state "will try to replace
>>>>> the file in the safest way possible", but I don't believe in miracles,
>>>>> much less in ones from the glib developers.
>>>> While searching to report the bug (see below), I found that this isn't
>>>> that simple. Actually, g_file_replace() itself seems quite safe, but
>>>> g_file_replace_contents() isn't.
>>>
>>> Well, it doesn't say much in the doc, but why would it not use
>>> g_file_replace and so be just as safe??
> 
> Ok, I wasn't clear, rather than saying replace_contents is just as
> safe lets say just as unsafe!!!
Yes, finally it's unsafe (even though it tries to be safe).

Ironically, notice that the docs of g_file_replace_contents() doesn't
speak of any safety.

>> 1) create a stream that points to a temp file
>> 2) write data to this stream
>> 3) move the temp file over the original one
>>
>> 1 is done by g_file_replace();
>> 2 is done with something like g_output_stream_write();
>> 3 is "scheduled" by g_file_replace() and is done upon stream closing
>> (g_output_stream_close() or g_object_unref() that calls the former).
>>
>> ...and if 1 succeeded, we can't prevent 3 to happen, even if 2 failed.
>>
>>
>> A small example is better that thousand words:
>>
>> ostream = g_file_replace(file, ..., &err);
>> if (ostream) {
>>  if (! g_output_stream_write_all (ostream, data, length, ..., &err)) {
>>    /* here's the problem, we don't have
>>     * g_output_stream_abort_replace() */
> 
> Lets say you are doing this yourself, not in g_file_replace_contents.
> 
> ???????? Now what are you going to do, the rename will still happen
> when the stream is closed or unrefed.
That's the problem, yep. Same for g_file_replace_contents() BTW, which
is basically a wrapper that does the above by itself.
So yes, can't use it for safe saving until it get fixed.

BTW there are possible hackish workarounds (GEdit has one, I implemented
a test case with another) to simulate an abortion, but it uses some
internal implementation details to do so, nothing good. Moreover, it
leaves the temporary file on disk.

> So we can't use g_file_replace, have to do it all ourselves :-(
> 
> Second the comment above about glib developers :-)
...I also feel really strange seeing that there was an effort for
g_file_replace() to provide correct safety, but none to be able to use
this safety. Moreover when g_file_replace_contents() exists.

>>>> Le 04/11/2010 18:53, Nick Treleaven a écrit :
>>>>> Also, if the argument for backups is set perhaps disk exhaustion is
>>>>> handled:
>>>>> http://library.gnome.org/devel/gio/2.25/GFile.html#g-file-replace-contents
>>>>
>>>> More or less. The backup file will be kept, but the "original" might
>>>> still be lost. The data isn't completely lost so, but the user might not
>>>> notice the backup is here (and it will not be restored).
>>>>
>>>
>>> I thought the sequence with backup would be:
>>>
>>> 1. write new contents to temp file, uses space
>>> 2. if successful rename old file to backup, uses no space
>>> 3. rename temp file to filename, uses no space
>>>
>>> so if the write fails for exhaustion the old file is still there but
>>> no backup, what we want.
>>>
>>> If one of the renames fails the state could be confusing, with a temp
>>> file and no old file, or a backup and no new file but that needs a
>>> filesystem crash to lose metadata right in the rename, *very* rare
>>> especially with journaled filesystems.
>> That's (probably, not really checked) right, but that's not really the
>> concern since the one that move/rename files doesn't know of any write
>> error (see above).
> 
> Yes we have to do this ourselves so we don't do the rename when the
> write fails, which includes having to do all the annoying permissions
> fixes, checks for non-regular files etc.
> 
> According to the comment in GIO source this stuff is copied from
> gedit.  IIRC at the beginning of the loooong series of several threads
> gedit was noted as working better.  Maybe just copy it, we are all GPL
> here.
I've not looked at all the GEdit sources, but at least a part uses GIO's
g_file_replace() and a hack to prevent renaming to happen on write error.

But yes, if we can re-use the code, it's just less effort and
everybody's happy :)

> In fact all in all, as far as I can see GIO isn't much help for this problem :-)
...until it gets fixed, yes.

>
>>
>> Hope it's a little bit more clear -- though I'm not sure I'm really
>> clear at 3 and half AM :D
> 
> Only 1430 for me.
> 
> Lack of sleep is bad for you so I have delayed this reply for some
> time so hopefully you have gone to bed and I can't be blamed for
> keeping you up :-D
Thank you very much, Lex! :D

Regards,
Colomban