[Box Backup-dev] BackupDaemon refactoring

[Chris Wilson]

Hi Ben,

>>  I'd like to reduce the code duplication, but I don't really want to create 
>>  an instance of the BackupDaemon. I was wondering if it might be OK to move 
>>  the code that sets up locations and calls BackupClient* out of 
>>  BackupDaemon and into a new class, maybe BackupManager, that I can use 
>>  instead?
>
> Can you be a bit more specific about what you want to do? Do you need 
> something it does over and above directly talking to the server using the 
> autogenerated Protocol code?

I want to run a backup when the user clicks on the Backup button in Boxi. 
I want to bypass the automatic backup scheduling in BackupDaemon, and I 
don't really want to fake an argv, have it intercept signals for me, or 
run a background thread to listen to a command socket on Win32. I also 
can't use BackupDaemon::Location as it's private.

What I've done is to copy SetupLocations from BackupDaemon, customised it 
to use my own (identical) copy of BackupDaemon::Location, and my own 
(different) configuration structures, and then start a backup using some 
code extracted from BackupDaemon::Run2:

 	SetupLocations(clientContext);

 	// Get some ID maps going
 	SetupIDMapsForSync();

 	[...]

 	DeleteUnusedRootDirEntries(clientContext);

 	[...]

 	typedef const std::vector<LocationRecord *> tLocationRecords;

 	// Go through the records, counting files and bytes
 	for (tLocationRecords::const_iterator i = mLocations.begin();
 		i != mLocations.end(); i++)
 	{
 		LocationRecord* pLocRecord = *i;
 		// Set exclude lists (context doesn't take ownership)
 		clientContext.SetExcludeLists(
 			pLocRecord->mpExcludeFiles,
 			pLocRecord->mpExcludeDirs);

 		CountDirectory(clientContext, pLocRecord->mPath);
 	}

 	[...]

 	// Go through the records, syncing them
 	for (tLocationRecords::const_iterator i = mLocations.begin();
 		i != mLocations.end(); i++)
 	{
 		LocationRecord* pLocRecord = *i;
 		int IDMapIndex = pLocRecord->mIDMapIndex;

 		[...]

 		// Set current and new ID map pointers in the context
 		clientContext.SetIDMaps(
 			mCurrentIDMaps[IDMapIndex],
 			mNewIDMaps    [IDMapIndex]);

 		// Set exclude lists (context doesn't take ownership)
 		clientContext.SetExcludeLists(
 			pLocRecord->mpExcludeFiles,
 			pLocRecord->mpExcludeDirs);

 		// Sync the directory

 		pLocRecord->mpDirectoryRecord->SyncDirectory(params,
 			BackupProtocolClientListDirectory::RootDirectory,
 			pLocRecord->mPath);

 		// Unset exclude lists (just in case)
 		clientContext.SetExcludeLists(0, 0);
 	}

The [...] sections are GUI code that I've inserted to let the user know 
what's going on while the backup is running, and allow them to cancel it 
by clicking on another button that causes StopRun() to return true.

> I have no problem with stuff being moved into more generic classes, but 
> refactoring does mean there's potential for introducing bugs.

Well, your test suite is very comprehensive, and we have a lot of testing 
to do on the win32 port especially, so I hope we can get the refactoring 
done and merged soon, so that any bugs we do introduce will get thrashed 
out by that testing.

Also, I was hoping that the refactor could be as simple as moving some 
methods and members from BackupDaemon to a new class, which should mean 
that the compiler will pick up any mistakes we make. The amount of new 
code to be written should be trivial and easy to check.

> BTW, I try to avoid calling classes *Manager, because this never 
> actually describes what it does. *Factory, *Interface, are far better.

OK, good point. How about BackupRunner, or just Backup?

Cheers, Chris.
-- 
_ ___ __     _
  / __/ / ,__(_)_  | Chris Wilson <0000 at qwirx.com> - Cambs UK |
/ (_/ ,\/ _/ /_ \ | Security/C/C++/Java/Perl/SQL/HTML Developer |
\ _/_/_/_//_/___/ | We are GNU-free your mind-and your software |