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

[Lex Trotman]

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!!!

In both cases there is no way of preventing the temp file being
renamed over the original.  See where there is lots of ?? below.

> g_file_replace() just /starts/ a replace operation [1]. It returns a
> stream to which you probably want to write data, but it doesn't do it.
> Getting this stream is safe, and writing to it is also safe.
>
> g_file_replace_contents(), however, also writes to the stream, then
> close it.
>
> The problem is that if writing to the stream failed, there is no
> (non-hackish) way to abort the final step of the replacement -- moving
> the temp file to the original one -- which is done on stream closing.
>
> I know this is a bit confusing at first glance... let's do a small picture:

I understand it I've looked at the source too

>
> 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.

So we can't use g_file_replace, have to do it all ourselves :-(

Second the comment above about glib developers :-)

>  } else {
>    /* the final part of the replacement (3) will happen here, when we
>     * close the stream.
>     * note that the g_object_unref() call above would do this
>     * automatically and we do it manually only to get possible
>     * errors. */
>    success = g_output_stream_close (ostream, &err);
>  }
>  g_object_unref (ostream);
> }
>
>>> g_file_replace() use a probably correct backup strategy, and returns a
>>> stream to write to in a safe way. This stream will actually point to a
>>> temporary file (or likely, seems there are 2 different strategies), and
>>> this file will be moved automatically to the final place when the stream
>>> get closed/destructed.
>>
>> And then we get the same permissions problem that glibs
>> g_file_set_contents has which was where we came in....
> Theoretically, yes. But GIO seems to keep attributes quite decently (at
> least the permissions are copied, I haven't checked more).

In fact if if can't set the permissions it switches to your suggestion
of copy the file to the backup and then truncate the original.

Of course that has the associated performance issues but is more
correct if the "fast" method didn't work.  But note that the backup is
now made when the file is opened, not when it is closed.

>
>>> The problem is that there is no API yet to abort the replacement, and so
>>> prevent the final move to be done.
>>>
>>> Actually, this is safe if e.g. the caller crashes during write, or
>>> anything that prevents the stream to be closed, and so the file to be
>>> moved...
>>>
>>>>> Perhaps you might like to file a bug against GIO. Perhaps first use gdb
>>>>> to break on that function just to be absolutely sure.
>>>>
>>>> Perhaps. But bugs filed to gnome and kde may take years to resolve...
>>> The bug is already reported, see
>>> https://bugzilla.gnome.org/show_bug.cgi?id=602412
>>> Sadly, this bug was reported on 2009-11 and last activity was 2010-04...
>>>
>>
>> Par for the course, if no one is worried enough to fix it then why
>> should the Gnome guys, its open source after all ....
> I'd think that maintainers should care a bit more than others, but
> basically you're right.

Sure but no one has asked for ages until you did,

They have limited time and use the amount of noise and status of the
reporter to decide priorities.  If the thread is very quiet then no
one wants it badly.

>
>>> 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.

In fact all in all, as far as I can see GIO isn't much help for this problem :-)

>
> 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

Cheers
Lex

>
>
> Regards,
> Colomban
> _______________________________________________
> Geany-devel mailing list
> Geany-devel at uvena.de
> http://lists.uvena.de/cgi-bin/mailman/listinfo/geany-devel
>