Bug 294059

Summary:	XML file corruption when disk is full
Product:	[Applications] kphotoalbum	Reporter:	Johannes Zarl-Zierl <johannes>
Component:	XML backend	Assignee:	KPhotoAlbum Bugs <kpabugs>
Status:	RESOLVED FIXED
Severity:	normal
Priority:	NOR
Version:	GIT master
Target Milestone:	---
Platform:	Compiled Sources
OS:	Linux
Latest Commit:		Version Fixed In:
Sentry Crash Report:
Attachments:	Save xml to temporary file first (prevent DB corruption). Don't fail if old xml file is removed during saving

Description Johannes Zarl-Zierl 2012-02-14 10:50:06 UTC

Version:           GIT master
OS:                Linux

When the disk on which the XML database resides is full, saving the database may fail. In this case, an error message is displayed, along with a warning that the xml file is corrupted (and that the user should save the file again).

From the mail-diskussion:

>>> A bit tired here..so I just ask about it. How is it handled if the
>>> write of index.xml will fail. Will the original one stay current or do
>>> we get a corrupted index.xml?
>>
>> Unfortunately we get a corrupted file. That's why I added a big fat warning to
>> the error message in this situation. It's certainly an improvement to the
>> previous situation, where KPA would silently fail and leave the file corrupted
>> (and because of the bugs in thumbnail-management it have an increased risk of
>> crashing in the same situation).
>>
>> Handling that situation cleanly would require writing to a temporary file,
>> copying the new file over the previous version and then deleting the temporary
>> file.
>
>Yep, but you should move the temporary file instead of copying to
>minimize the risk of error. Would you be willing to write this patch
>also?



Reproducible: Always

Steps to Reproduce:
Fill up the disk, add data to the database, save database.


Expected Results:  
After saving failed, DB-file should be the same as before saving.

Comment 1 Johannes Zarl-Zierl 2012-02-19 21:07:16 UTC

Created attachment 68936 [details]
Save xml to temporary file first (prevent DB corruption).

Comment 2 Miika Turkia 2012-02-20 05:39:14 UTC

Git commit fd1fe4b9989d911d773ef40e21937fc4a17e0cc2 by Miika Turkia, on behalf of Johannes Zarl.
Committed on 19/02/2012 at 22:03.
Pushed by mturkia into branch 'master'.

Save xml to temporary file first (prevent DB corruption).

M  +49   -18   XMLDB/FileWriter.cpp

http://commits.kde.org/kphotoalbum/fd1fe4b9989d911d773ef40e21937fc4a17e0cc2

Comment 3 Miika Turkia 2012-02-20 05:43:01 UTC

FYI, there is no need to go manually closing the bug when you add a
fixing patch there. It is actually better to close it only after the
patch has been applied to git. The BUG: <id> -keyword will
automatically close the bug when the patch is applied (if the
committer has proper rights in bugs.kde.org)

Anyway, thanks for the patch!
miika

On Sun, Feb 19, 2012 at 11:07 PM, Johannes Zarl <isilmendil@gmx.net> wrote:
> https://bugs.kde.org/show_bug.cgi?id=294059
>
>
> Johannes Zarl <isilmendil@gmx.net> changed:
>
>           What    |Removed                     |Added
> ----------------------------------------------------------------------------
>             Status|UNCONFIRMED                 |RESOLVED
>         Resolution|                            |FIXED
>
>
>
>
> --
> Configure bugmail: https://bugs.kde.org/userprefs.cgi?tab=email
> ------- You are receiving this mail because: -------
> You are the assignee for the bug.
> _______________________________________________
> Kphotoalbum-bugs mailing list
> Kphotoalbum-bugs@mail.kdab.com
> https://mail.kdab.com/mailman/listinfo/kphotoalbum-bugs

Comment 4 Johannes Zarl-Zierl 2012-02-20 20:48:24 UTC

Created attachment 68969 [details]
Don't fail if old xml file is removed during saving

It seems that the previous patch does not work well with the autosave feature, leading to spurious error-messages:
"Failed to remove old version of image database.
Please try again or replace the file .#index.xml with file .#index.xml.tmp manually!"

This happened to me twice, but I could not trigger it another time.
My best guess would be that the KPA sometimes removes the previous autosave file (.#index.xml) while an autosave is in progress.
The attached patch makes FileWriter::save() robust against a vanishing database file...

Comment 5 Miika Turkia 2012-02-21 04:30:54 UTC

I noticed the same bug but forgot to commit my fix yesterday. Now it is done in your way. However, test whether file exists is done before attempting to remove it. (The file never exists after the removal.)

http://commits.kde.org/kphotoalbum/0aa7f4dcefaf5b01bd7f8c4fd4fb9240bd024b67

Comment 6 Johannes Zarl-Zierl 2012-02-21 23:20:28 UTC

The test has to be done _after_ removal. Otherwise you still have the race-condition (although with a smaller window of opportunity than before)...

Comment 7 Miika Turkia 2012-02-22 04:42:24 UTC

You are absolutely right. I should have read the error message and have another espresso before doing reading the code. Fixed now.