an initial attempt at backups

[bdmayes]

This thread is as a result of another thread requesting backup functionality within KeePassX. The original thread can be found here:

http://www.keepassx.org/forum/viewtopic.php?t=713

The basic program flow is as follows:
When a database is opened, the function checks the config file. If the backupDef variable is "true" in the config file, then it will make a backup. It does a deep copy of the fully qualified path to the database (just in case...I didn't want to screw anything up by just making a shallow copy and changing the QString), and then spawns a thread to take the backup. The thread will parse the fully qualified path and create a new file called backup-<databaseName>.kdb in the location that the user requested in the settings dialog. The settings dialog is functional, and the "browse" button allows a user to browse to the desired directory to store the backup.

For example, let's suppose the user has backups enabled and has asked to store backups in the directory /mnt/data_drive. Now the user opens the file /home/foo/example.kdb in KeePassX. If the database is opened successfully, the thread will create a copy of example.kdb. It's fully qualified path is /mnt/data_drive/backup-example.kdb. If that file already exists, it is overwritten.

At this point I have created a working patch for revision 196 (the latest in SVN). You can find the patch here:

http://slackbox.homelinux.org/backup.patch

I have tested the patch on revision 196 in both Ubuntu 7.10 and Mac OS 10.5.2. Here's what is working as of right now:

- the new option appears in the settings dialog, under Advanced.
- the getter and setter functions for the option are properly updating the config file.
- the application is copying the database to a new file (when a database is opened).
- the application backs up every database when it is opened, assuming the option is enabled
- the backup versions of the database can successfully be opened by KeePassX.
- a single backup is made each time. It is overwritten when the database is opened again.
- the patch applies cleanly to the latest SVN revision and builds without errors.

I am hoping other developers can help provide some pointers and suggestions for improvement, though I think this is at least a good start. I have a few issues with the code and I will discuss them individually below.


1. The Makefile:
I have never been very savvy with makefiles. As of right now, I added a new header file called backup.h to the source tree. Changes to this file are not picked up until another file (such as mainwindow.cpp) changes. Thus, whenever I want to recompile I have to make a change to another file such as deleting a semi-colon from mainwindow.cpp, adding it back it, and then saving it. The Makefile notices the change in mainwindow.cpp, recompiles, and since mainwindow.cpp has an include statement for backup.h, it recompiles that file as well. If I ONLY change backup.h, then the Makefile will state that there is nothing to be done.

2. The placement:
I put the new option for taking backups on the "Advanced" tab of the settings dialog. I am not sure if that is the best place for it, but it's simple to move it by changing the SettingsDlg.ui file.

3. Subclasses:
I initially planned on having a class called "Backup" which would be a child of QThread. Then I thought the actual implementations (such as backing up by a simple file copy, or SCP, or FTP, etc.) would be children of class Backup. After coding it up, I realized I had nothing in the Backup class (no instance variables of any kind) so I didn't really see what the point of it was anymore. Thus, I created a class called BackupFile which is a direct child of QThread. I figured any further classes could also directly extend QThread (and placed this in the comments at the end of the backup.h file as a template).

If more classes (i.e. different backup types) are defined the future, then a simple variable can be added to the config file to specify the type. Then in mainwindow.cpp a switch statement on the type can be used to spawn the correct thread and take the desired action.

4. Sleep vs. wait (and wait is behaving strangely!):
I thought that by creating a thread and calling it's run method, it would not block the keepassx process. I was apparently wrong. Initially I had the thread sleep while it was not able to find the specified directory. This blocked keepassx and that is very undesirable. I changed it from sleep() to wait(), but it still blocks. I wanted to have a timeout so that if the thread could not find the mount point for 60 seconds it would just quit. So I have a loop execute (120 times) every 500ms to check if the directory exists. If it doesn't, it decrements the counter and waits again. Now, according to the Trolltech documentation for QThread, specifying a time (in milliseconds) as a parameter to the wait method should make it wait that long and then wake up. It doesn't seem to. If you uncomment the cout statement in the while loop (and specify a bad directory name in the config file) you will see the numbers 120 down to 1 print almost immediately when you attempt to open a database. It certainly does not take 1 minute.

Furthermore, if you increase the counter to say 1 million and recompile, the thread will block keepassx. You can tell by the cout statement printing the values of counter all the way down to 1. Only then will the mainwindow actually appear.

So I guess the real issues here are that I have BackupFile as a child of QThread, but calling it's run() method is blocking the keepassx process. Does anyone see why? And furthermore, why isn't wait(500) actually waiting 500ms before trying again? It just seems to go back to the top of the loop instantly.

5. Should we allow multiple backups?
Currently the code only allows one backup file for each database. Is there any reason to have it keep multiple backups? For example, maybe it will keep the 5 latest "versions" of the database? I say "versions" because it's possible that a user opens the database to read it, but does not make any changes. In this case, the process would create another backup (such as backup1-example.kdb and backup2-example.kdb) but the two backups would not actually be different.

6. Backups are always stored in the same place:
It's not currently possible to have example.kdb store its backup in /mnt/data_drive but have example2.kdb store it's backups in /home/foo. If backups are enabled, then there exists a single directory where all backups will be stored (unless the config files are different). Assuming the same config file, then specifying /mnt/data_drive as the backup location for one database implies that it is the backup location for all databases.

The only current way around this is to have multiple config files and then point keepassx to the appropriate config file when launched. Is this a problem, or is this reasonable behavior?

[Tarek]

So I guess the real issues here are that I have BackupFile as a child of QThread, but calling it's run() method is blocking the keepassx process. Does anyone see why?

http://doc.trolltech.com/4.3/qthread.html#start

[bdmayes]

Ah! I should be calling start() instead of run() then. I'll give that a shot when I get home in about and hour or two, and I will post the results. Thanks Tarek!

What about the other areas I mentioned? Any further comments/suggestions/questions? Here are some further comments I have:

1. I know the Makefile still doesn't work, because I don't know how to fix it. I'm much more fluent in Java than C/C++.

2. If anyone has another place they would like to put it besides the Advanced tab that's fine. Just let me know and I'll move it.

3. I'm still thinking this is legitimate. Since my original "Backup" class only contained a constructor, a destructor, and a run method (all of which were blank), I think it's just a waste of space. I don't see any need to have an array of "Backup" threads nor do I see the need to make a pointer of type "Backup" that actually points at a "BackupFile" or "BackupSCP" object. I think having a switch statement that spawns the correct thread seems reasonable.

4. As discussed, I'll try calling start().

5. Still not sure if it's necessary to have a number of different backups.

6. I am starting to think this is sufficient. If there are different users then they should be using a different config file. Furthermore, if a single user did want their backups going to different places, but did not want to use multiple config files for some reason, they could just send all the backups to the same place and then script the movement. A cron job could call a simple script (every minute maybe?) to just move the files from one directory to another. Something like:

Code: Select all

#!/bin/bash

if [ -f /home/foo/foo.kdb ]; then
    if [ -d /mnt/data_drive ]; then
        mv /home/foo/foo.kdb /mnt/data_drive
    fi
fi


If we want to allow this functionality directly in KeePassX then we would have to store both the database name and desired backup location for each database. It seems to me like that should be taken care of by using different config files, and not by writing such options in a single config file.

[bdmayes]

Well I tried calling start() instead of run() in mainwindow.cpp. When the directory specified is available this works fine. However, when the directory is unavailable, the use of start() in the code causes a segmentation fault and the entire keepassx process crashes.

I was able to stop it from seg faulting if I override the start() method in my class, but then all it does it call run(), and this of course still blocks the keepassx process until after it has tried the backup 120 times.

I'll have to do some further research on this.

[bdmayes]

So I think I know why this blocking issue is occurring. I believe the problem is that it is not really spawning a new thread that is independent of the keepassx process. What it is doing is spawning a new thread, but both threads are in the same process and the newly spawned thread is getting control. I thought that wait() would cause it to hand control back over to another thread but it doesn't seem to be doing that (maybe I don't understand the underlying implementation provided by Qt). Thus, there is no context switch to another thread so the new thread blocks.

I will continue to work on this when I can, though I have been incredibly busy lately. I hope to eventually get a working patch submitted. Will post on here and/or sourceforge when that (eventually) happens.