This thread is as a result of another thread requesting backup functionality within KeePassX. The original thread can be found here:
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:
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.
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?