[Box Backup-dev] COMMIT r325 - in box/chris/diff-timeout-and-ssl-keepalive: bin/bbackupd lib/backupclient test/backupstore test/backupstorepatch

boxbackup-dev@fluffy.co.uk boxbackup-dev@fluffy.co.uk
Thu, 19 Jan 2006 22:53:14 +0000 (GMT)


Author: chris
Date: 2006-01-19 22:53:07 +0000 (Thu, 19 Jan 2006)
New Revision: 325

Modified:
   box/chris/diff-timeout-and-ssl-keepalive/bin/bbackupd/BackupClientContext.cpp
   box/chris/diff-timeout-and-ssl-keepalive/bin/bbackupd/BackupClientContext.h
   box/chris/diff-timeout-and-ssl-keepalive/bin/bbackupd/BackupClientDirectoryRecord.cpp
   box/chris/diff-timeout-and-ssl-keepalive/lib/backupclient/BackupStoreFile.h
   box/chris/diff-timeout-and-ssl-keepalive/lib/backupclient/BackupStoreFileDiff.cpp
   box/chris/diff-timeout-and-ssl-keepalive/test/backupstore/testbackupstore.cpp
   box/chris/diff-timeout-and-ssl-keepalive/test/backupstorepatch/testbackupstorepatch.cpp
Log:
* diff-timeout-and-ssl-keepalive
- Refactored the diff timeout checking code out of the signal handler and
  into BackupStoreFile. Comments welcome. All tests pass on FC2 i386.


Modified: box/chris/diff-timeout-and-ssl-keepalive/bin/bbackupd/BackupClientContext.cpp
===================================================================
--- box/chris/diff-timeout-and-ssl-keepalive/bin/bbackupd/BackupClientContext.cpp	2006-01-18 23:34:07 UTC (rev 324)
+++ box/chris/diff-timeout-and-ssl-keepalive/bin/bbackupd/BackupClientContext.cpp	2006-01-19 22:53:07 UTC (rev 325)
@@ -56,7 +56,10 @@
 	  mStorageLimitExceeded(false),
 	  mpExcludeFiles(0),
 	  mpExcludeDirs(0),
-	  mbIsManaged(false)
+	  mbIsManaged(false),
+	  mTimeMgmtEpoch(0),
+	  mMaximumDiffTime(600),
+	  mKeepAliveTime(0)
 {
 }
 
@@ -465,8 +468,6 @@
 static int sMaximumDiffTime = 600;
 // maximum time of SSL inactivity (keep-alive interval)
 static int sKeepAliveTime = 0;
-// total time elapsed to keep track of what is going on
-static time_t sTimeMgmtEpoch = 0;
 
 void BackupClientContext::SetMaximumDiffingTime(int iSeconds)
 {
@@ -480,8 +481,6 @@
 	TRACE1("Set keep-alive time to %d seconds\n", sKeepAliveTime);
 }
 
-static BackupClientContext* pThisCtxInst = NULL;
-
 // --------------------------------------------------------------------------
 //
 // Function
@@ -490,23 +489,9 @@
 //		Created: 19/3/04
 //
 // --------------------------------------------------------------------------
-void TimerSigHandler(int iUnused)
+static void TimerSigHandler(int iUnused)
 {
-	ASSERT(pThisCtxInst);
-	ASSERT(sTimeMgmtEpoch > 0);
-
-	time_t tTotalRunIntvl = time(NULL) - sTimeMgmtEpoch;
-	if (sMaximumDiffTime > 0 && tTotalRunIntvl >= sMaximumDiffTime)
-	{
-		TRACE0("MaximumDiffingTime reached - suspending file diff\n");
-		BackupStoreFile::SuspendFileDiff();
-	}
-	else if (sKeepAliveTime > 0)
-	{
-		// well, we have only two sources of timer events...
-		TRACE0("KeepAliveTime reached - initiating keep-alive\n");
-		pThisCtxInst->DoKeepAlive();
-	}
+	BackupStoreFile::DiffTimerExpired();	
 }
 
 // --------------------------------------------------------------------------
@@ -522,8 +507,7 @@
 	if (mbIsManaged || !mpConnection)
 		return;
 
-	ASSERT(pThisCtxInst == NULL);
-	ASSERT(sTimeMgmtEpoch == 0);
+	ASSERT(mTimeMgmtEpoch == 0);
 
 #ifdef PLATFORM_CYGWIN
 	::signal(SIGALRM, TimerSigHandler);
@@ -554,8 +538,7 @@
 	}
 
 	// avoid race
-	pThisCtxInst = this;
-	sTimeMgmtEpoch = time(NULL);
+	mTimeMgmtEpoch = time(NULL);
 
 #ifdef PLATFORM_CYGWIN
 	if(::setitimer(ITIMER_REAL, &timeout, NULL) != 0)
@@ -563,8 +546,7 @@
 	if(::setitimer(ITIMER_VIRTUAL, &timeout, NULL) != 0)
 #endif // PLATFORM_CYGWIN
 	{
-		sTimeMgmtEpoch = 0;
-		pThisCtxInst = NULL;
+		mTimeMgmtEpoch = 0;
 
 		TRACE0("WARNING: couldn't set file diff control timeout\n");
 		THROW_EXCEPTION(BackupStoreException, Internal)
@@ -587,8 +569,6 @@
 	if (!mbIsManaged /* don't test for active connection, just do it */)
 		return;
 
-	ASSERT(pThisCtxInst != NULL);
-
 	struct itimerval timeout;
 	memset(&timeout, 0, sizeof(timeout));
 
@@ -603,8 +583,7 @@
 	}
 
 	mbIsManaged = false;
-	pThisCtxInst = NULL;
-	sTimeMgmtEpoch = 0;
+	mTimeMgmtEpoch = 0;
 
 	TRACE0("Suspended timer for file diff control\n");
 }
@@ -624,3 +603,27 @@
 
 	mpConnection->QueryGetIsAlive();
 }
+
+// --------------------------------------------------------------------------
+//
+// Function
+//		Name:    BackupClientContext::GetTimeMgmtEpoch()
+//		Purpose: Returns the unix time when the diff was started, or zero
+//				 if the diff process is unmanaged.
+//		Created: 04/19/2005
+//
+// --------------------------------------------------------------------------
+time_t BackupClientContext::GetTimeMgmtEpoch() 
+{
+	return mTimeMgmtEpoch;
+}
+
+int BackupClientContext::GetMaximumDiffingTime() 
+{
+	return mMaximumDiffTime;
+}
+
+int BackupClientContext::GetKeepaliveTime() 
+{
+	return mKeepAliveTime;
+}

Modified: box/chris/diff-timeout-and-ssl-keepalive/bin/bbackupd/BackupClientContext.h
===================================================================
--- box/chris/diff-timeout-and-ssl-keepalive/bin/bbackupd/BackupClientContext.h	2006-01-18 23:34:07 UTC (rev 324)
+++ box/chris/diff-timeout-and-ssl-keepalive/bin/bbackupd/BackupClientContext.h	2006-01-19 22:53:07 UTC (rev 325)
@@ -12,6 +12,7 @@
 
 #include "BoxTime.h"
 #include "BackupClientDeleteList.h"
+#include "BackupStoreFile.h"
 #include "ExcludeList.h"
 
 class TLSContext;
@@ -31,12 +32,12 @@
 //		Created: 2003/10/08
 //
 // --------------------------------------------------------------------------
-class BackupClientContext
+class BackupClientContext : public DiffTimer
 {
 public:
 	BackupClientContext(BackupDaemon &rDaemon, TLSContext &rTLSContext, const std::string &rHostname,
 		int32_t AccountNumber, bool ExtendedLogging);
-	~BackupClientContext();
+	virtual ~BackupClientContext();
 private:
 	BackupClientContext(const BackupClientContext &);
 public:
@@ -178,11 +179,16 @@
 	//
 	// Function
 	//		Name:    BackupClientContext::DoKeepAlive()
-	//		Purpose: Does something inconsequential over the SSL link to keep it up
+	//		Purpose: Does something inconsequential over the SSL link to 
+	//				 keep it up, implements DiffTimer interface
 	//		Created: 04/19/2005
 	//
 	// --------------------------------------------------------------------------
-	void DoKeepAlive();
+	virtual void   DoKeepAlive();
+	virtual time_t GetTimeMgmtEpoch();
+	virtual int    GetMaximumDiffingTime();
+	virtual int    GetKeepaliveTime();
+	
 private:
 	BackupDaemon &mrDaemon;
 	TLSContext &mrTLSContext;
@@ -200,8 +206,14 @@
 	ExcludeList *mpExcludeDirs;
 
 	bool mbIsManaged;
+	// unix time when diff was started
+	time_t mTimeMgmtEpoch;
+	// maximum time to spend diffing, in seconds
+	int mMaximumDiffTime;
+	// maximum time of SSL inactivity (keep-alive interval), in seconds
+	int mKeepAliveTime;
+
 };
 
 
 #endif // BACKUPCLIENTCONTEXT__H
-

Modified: box/chris/diff-timeout-and-ssl-keepalive/bin/bbackupd/BackupClientDirectoryRecord.cpp
===================================================================
--- box/chris/diff-timeout-and-ssl-keepalive/bin/bbackupd/BackupClientDirectoryRecord.cpp	2006-01-18 23:34:07 UTC (rev 324)
+++ box/chris/diff-timeout-and-ssl-keepalive/bin/bbackupd/BackupClientDirectoryRecord.cpp	2006-01-19 22:53:07 UTC (rev 325)
@@ -1100,10 +1100,15 @@
 				rParams.mrContext.ManageDiffProcess();
 
 				bool isCompletelyDifferent = false;
-				std::auto_ptr<IOStream> patchStream(BackupStoreFile::EncodeFileDiff(rFilename.c_str(),
+				std::auto_ptr<IOStream> patchStream(
+					BackupStoreFile::EncodeFileDiff(
+						rFilename.c_str(),
 						mObjectID,	/* containing directory */
 						rStoreFilename, diffFromID, *blockIndexStream,
-						connection.GetTimeout(), 0 /* not interested in the modification time */, &isCompletelyDifferent));
+						connection.GetTimeout(), 
+						&rParams.mrContext, // DiffTimer implementation
+						0 /* not interested in the modification time */, 
+						&isCompletelyDifferent));
 	
 				rParams.mrContext.UnManageDiffProcess();
 
@@ -1221,6 +1226,3 @@
 BackupClientDirectoryRecord::SyncParams::~SyncParams()
 {
 }
-
-
-

Modified: box/chris/diff-timeout-and-ssl-keepalive/lib/backupclient/BackupStoreFile.h
===================================================================
--- box/chris/diff-timeout-and-ssl-keepalive/lib/backupclient/BackupStoreFile.h	2006-01-18 23:34:07 UTC (rev 324)
+++ box/chris/diff-timeout-and-ssl-keepalive/lib/backupclient/BackupStoreFile.h	2006-01-19 22:53:07 UTC (rev 325)
@@ -38,6 +38,24 @@
 // --------------------------------------------------------------------------
 //
 // Class
+//		Name:    DiffTimer
+//		Purpose: Interface for classes that can keep track of diffing time,
+//				 and send SSL keepalive messages
+//		Created: 2006/01/19
+//
+// --------------------------------------------------------------------------
+class DiffTimer
+{
+public:
+	virtual void   DoKeepAlive() = 0;
+	virtual time_t GetTimeMgmtEpoch() = 0;
+	virtual int    GetMaximumDiffingTime() = 0;
+	virtual int    GetKeepaliveTime() = 0;
+};
+
+// --------------------------------------------------------------------------
+//
+// Class
 //		Name:    BackupStoreFile
 //		Purpose: Class to hold together utils for maniplating files.
 //		Created: 2003/08/28
@@ -95,9 +113,16 @@
 
 	// Main interface
 	static std::auto_ptr<IOStream> EncodeFile(const char *Filename, int64_t ContainerID, const BackupStoreFilename &rStoreFilename, int64_t *pModificationTime = 0);
-	static std::auto_ptr<IOStream> EncodeFileDiff(const char *Filename, int64_t ContainerID,
-		const BackupStoreFilename &rStoreFilename, int64_t DiffFromObjectID, IOStream &rDiffFromBlockIndex,
-		int Timeout, int64_t *pModificationTime = 0, bool *pIsCompletelyDifferent = 0);
+	static std::auto_ptr<IOStream> EncodeFileDiff
+	(
+		const char *Filename, int64_t ContainerID,
+		const BackupStoreFilename &rStoreFilename, 
+		int64_t DiffFromObjectID, IOStream &rDiffFromBlockIndex,
+		int Timeout, 
+		DiffTimer *pDiffTimer,
+		int64_t *pModificationTime = 0, 
+		bool *pIsCompletelyDifferent = 0
+	);
 	static bool VerifyEncodedFileFormat(IOStream &rFile, int64_t *pDiffFromObjectIDOut = 0, int64_t *pContainerIDOut = 0);
 	static void CombineFile(IOStream &rDiff, IOStream &rDiff2, IOStream &rFrom, IOStream &rOut);
 	static void CombineDiffs(IOStream &rDiff1, IOStream &rDiff2, IOStream &rDiff2b, IOStream &rOut);
@@ -144,17 +169,7 @@
 		free(a);
 	}
 
-	// --------------------------------------------------------------------------
-	//
-	// Function
-	//		Name:    BackupStoreFile::SuspendFileDiff()
-	//		Purpose: Notifies BackupStoreFile object that a diff operation should be
-	//				 terminated ASAP. Usually called from an external timer.
-	//
-	//		Created: 12/1/04
-	//
-	// --------------------------------------------------------------------------
-	static void SuspendFileDiff();
+	static void DiffTimerExpired();
 
 	// Building blocks
 	class EncodingBuffer
@@ -201,4 +216,3 @@
 #include "MemLeakFindOff.h"
 
 #endif // BACKUPSTOREFILE__H
-

Modified: box/chris/diff-timeout-and-ssl-keepalive/lib/backupclient/BackupStoreFileDiff.cpp
===================================================================
--- box/chris/diff-timeout-and-ssl-keepalive/lib/backupclient/BackupStoreFileDiff.cpp	2006-01-18 23:34:07 UTC (rev 324)
+++ box/chris/diff-timeout-and-ssl-keepalive/lib/backupclient/BackupStoreFileDiff.cpp	2006-01-19 22:53:07 UTC (rev 325)
@@ -43,29 +43,37 @@
 
 static void LoadIndex(IOStream &rBlockIndex, int64_t ThisID, BlocksAvailableEntry **ppIndex, int64_t &rNumBlocksOut, int Timeout, bool &rCanDiffFromThis);
 static void FindMostUsedSizes(BlocksAvailableEntry *pIndex, int64_t NumBlocks, int32_t Sizes[BACKUP_FILE_DIFF_MAX_BLOCK_SIZES]);
-static void SearchForMatchingBlocks(IOStream &rFile, std::map<int64_t, int64_t> &rFoundBlocks, BlocksAvailableEntry *pIndex, int64_t NumBlocks, int32_t Sizes[BACKUP_FILE_DIFF_MAX_BLOCK_SIZES]);
+static void SearchForMatchingBlocks(IOStream &rFile, 
+	std::map<int64_t, int64_t> &rFoundBlocks, BlocksAvailableEntry *pIndex, 
+	int64_t NumBlocks, int32_t Sizes[BACKUP_FILE_DIFF_MAX_BLOCK_SIZES],
+	DiffTimer *pDiffTimer);
 static void SetupHashTable(BlocksAvailableEntry *pIndex, int64_t NumBlocks, int32_t BlockSize, BlocksAvailableEntry **pHashTable);
 static bool SecondStageMatch(BlocksAvailableEntry *pFirstInHashList, RollingChecksum &fastSum, uint8_t *pBeginnings, uint8_t *pEndings, int Offset, int32_t BlockSize, int64_t FileBlockNumber,
 BlocksAvailableEntry *pIndex, std::map<int64_t, int64_t> &rFoundBlocks);
 static void GenerateRecipe(BackupStoreFileEncodeStream::Recipe &rRecipe, BlocksAvailableEntry *pIndex, int64_t NumBlocks, std::map<int64_t, int64_t> &rFoundBlocks, int64_t SizeOfInputFile);
 
-// control whether a currently active diff should be terminated ASAP
-static bool sDiffTimedOut = false;
+// sDiffTimerExpired flags when the diff timer has expired. When true, the 
+// diff routine should check the wall clock as soon as possible, to determine 
+// whether it's time for a keepalive to be sent, or whether the diff has been 
+// running for too long and should be terminated.
+static bool sDiffTimerExpired = false;
 
 
 // --------------------------------------------------------------------------
 //
 // Function
-//		Name:    BackupStoreFile::SuspendFileDiff()
-//		Purpose: Notifies BackupStoreFile object that a diff operation should be
-//				 terminated ASAP. Usually called from an external timer.
+//		Name:    BackupStoreFile::DiffTimerExpired()
+//		Purpose: Notifies BackupStoreFile object that the diff operation
+//				 timer has expired, which may mean that a keepalive should
+//				 be sent, or the diff should be terminated. Called from an
+//				 external timer, so it should not do more than set a flag.
 //
-//		Created: 12/1/04
+//		Created: 19/1/06
 //
 // --------------------------------------------------------------------------
-void BackupStoreFile::SuspendFileDiff()
+void BackupStoreFile::DiffTimerExpired()
 {
-	sDiffTimedOut = true;
+	sDiffTimerExpired = true;
 }
 
 
@@ -135,9 +143,12 @@
 //		Created: 12/1/04
 //
 // --------------------------------------------------------------------------
-std::auto_ptr<IOStream> BackupStoreFile::EncodeFileDiff(const char *Filename, int64_t ContainerID,
-	const BackupStoreFilename &rStoreFilename, int64_t DiffFromObjectID, IOStream &rDiffFromBlockIndex,
-	int Timeout, int64_t *pModificationTime, bool *pIsCompletelyDifferent)
+std::auto_ptr<IOStream> BackupStoreFile::EncodeFileDiff
+(
+	const char *Filename, int64_t ContainerID,
+	const BackupStoreFilename &rStoreFilename, int64_t DiffFromObjectID, 
+	IOStream &rDiffFromBlockIndex, int Timeout, DiffTimer *pDiffTimer, 
+	int64_t *pModificationTime, bool *pIsCompletelyDifferent)
 {
 	// Is it a symlink?
 	{
@@ -197,7 +208,8 @@
 				// Get size of file
 				sizeOfInputFile = file.BytesLeftToRead();
 				// Find all those lovely matching blocks
-				SearchForMatchingBlocks(file, foundBlocks, pindex, blocksInIndex, sizesToScan);
+				SearchForMatchingBlocks(file, foundBlocks, pindex, 
+					blocksInIndex, sizesToScan, pDiffTimer);
 				
 				// Is it completely different?
 				completelyDifferent = (foundBlocks.size() == 0);
@@ -468,8 +480,20 @@
 //
 // --------------------------------------------------------------------------
 static void SearchForMatchingBlocks(IOStream &rFile, std::map<int64_t, int64_t> &rFoundBlocks,
-	BlocksAvailableEntry *pIndex, int64_t NumBlocks, int32_t Sizes[BACKUP_FILE_DIFF_MAX_BLOCK_SIZES])
+	BlocksAvailableEntry *pIndex, int64_t NumBlocks, 
+	int32_t Sizes[BACKUP_FILE_DIFF_MAX_BLOCK_SIZES], DiffTimer *pDiffTimer)
 {
+	time_t TimeMgmtEpoch   = 0;
+	int MaximumDiffingTime = 0;
+	int KeepAliveTime      = 0;
+
+	if (pDiffTimer)
+	{
+		TimeMgmtEpoch      = pDiffTimer->GetTimeMgmtEpoch();
+		MaximumDiffingTime = pDiffTimer->GetMaximumDiffingTime();
+		KeepAliveTime      = pDiffTimer->GetKeepaliveTime();
+	}
+	
 	std::map<int64_t, int32_t> goodnessOfFit;
 
 	// Allocate the hash lookup table
@@ -636,9 +660,38 @@
 							// End this loop, so the final byte isn't used again
 							break;
 						}
+
+						bool DiffTimedOut = false;
 						
-						if(static_cast<int64_t>(rFoundBlocks.size()) > (NumBlocks * BACKUP_FILE_DIFF_MAX_BLOCK_FIND_MULTIPLE) || sDiffTimedOut)
+						if(sDiffTimerExpired)
 						{
+							ASSERT(TimeMgmtEpoch > 0);
+							ASSERT(pDiffTimer != NULL);
+							
+							time_t tTotalRunIntvl = time(NULL) - TimeMgmtEpoch;
+							
+							if(MaximumDiffingTime > 0 && 
+								tTotalRunIntvl >= MaximumDiffingTime)
+							{
+								TRACE0("MaximumDiffingTime reached - "
+									"suspending file diff\n");
+								DiffTimedOut = true;
+							}
+							else if(KeepAliveTime > 0)
+							{
+								TRACE0("KeepAliveTime reached - "
+									"initiating keep-alive\n");
+								pDiffTimer->DoKeepAlive();
+							}
+						}
+
+						int64_t NumBlocksFound = static_cast<int64_t>(
+							rFoundBlocks.size());
+						int64_t MaxBlocksFound = NumBlocks * 
+							BACKUP_FILE_DIFF_MAX_BLOCK_FIND_MULTIPLE;
+						
+						if(NumBlocksFound > MaxBlocksFound || DiffTimedOut)
+						{
 							abortSearch = true;
 							break;
 						}

Modified: box/chris/diff-timeout-and-ssl-keepalive/test/backupstore/testbackupstore.cpp
===================================================================
--- box/chris/diff-timeout-and-ssl-keepalive/test/backupstore/testbackupstore.cpp	2006-01-18 23:34:07 UTC (rev 324)
+++ box/chris/diff-timeout-and-ssl-keepalive/test/backupstore/testbackupstore.cpp	2006-01-19 22:53:07 UTC (rev 325)
@@ -1072,9 +1072,16 @@
 			// Do the patching
 			bool isCompletelyDifferent = false;
 			int64_t modtime;
-			std::auto_ptr<IOStream> patchstream(BackupStoreFile::EncodeFileDiff(TEST_FILE_FOR_PATCHING ".mod", BackupProtocolClientListDirectory::RootDirectory,
-					uploads[UPLOAD_PATCH_EN].name, uploads[UPLOAD_PATCH_EN].allocated_objid, *blockIndexStream,
-					IOStream::TimeOutInfinite, &modtime, &isCompletelyDifferent));
+			std::auto_ptr<IOStream> patchstream(
+				BackupStoreFile::EncodeFileDiff(
+					TEST_FILE_FOR_PATCHING ".mod", 
+					BackupProtocolClientListDirectory::RootDirectory,
+					uploads[UPLOAD_PATCH_EN].name, 
+					uploads[UPLOAD_PATCH_EN].allocated_objid, 
+					*blockIndexStream,
+					IOStream::TimeOutInfinite, 
+					NULL, // pointer to DiffTimer impl
+					&modtime, &isCompletelyDifferent));
 			TEST_THAT(isCompletelyDifferent == false);
 			// Sent this to a file, so we can check the size, rather than uploading it directly
 			{

Modified: box/chris/diff-timeout-and-ssl-keepalive/test/backupstorepatch/testbackupstorepatch.cpp
===================================================================
--- box/chris/diff-timeout-and-ssl-keepalive/test/backupstorepatch/testbackupstorepatch.cpp	2006-01-18 23:34:07 UTC (rev 324)
+++ box/chris/diff-timeout-and-ssl-keepalive/test/backupstorepatch/testbackupstorepatch.cpp	2006-01-19 22:53:07 UTC (rev 325)
@@ -376,10 +376,17 @@
 					char filename[64];
 					::sprintf(filename, "testfiles/%d.test", f);
 					bool isCompletelyDifferent = false;
-					std::auto_ptr<IOStream> patchStream(BackupStoreFile::EncodeFileDiff(filename,
+					std::auto_ptr<IOStream> patchStream(
+						BackupStoreFile::EncodeFileDiff(
+							filename,
 							BackupProtocolClientListDirectory::RootDirectory,	/* containing directory */
-							storeFilename, diffFromID, *blockIndexStream,
-							protocol.GetTimeout(), 0 /* not interested in the modification time */, &isCompletelyDifferent));
+							storeFilename, 
+							diffFromID, 
+							*blockIndexStream,
+							protocol.GetTimeout(), 
+							NULL, // DiffTimer impl
+							0 /* not interested in the modification time */, 
+							&isCompletelyDifferent));
 		
 					// Upload the patch to the store
 					std::auto_ptr<BackupProtocolClientSuccess> stored(protocol.QueryStoreFile(