Pass std::auto_ptr objects to Protocol for upload.

Passing raw pointers is bad C++ style, and dangerous, because Protocol
will free the passed-in pointers after uploading them, so we should not
keep using them.

Reduce code duplication in BackupClientDirectoryRecord patch/normal upload.

Return a std::auto_ptr<BackupStoreFileEncodeStream> instead of a
std::auto_ptr<IOStream> from BackupStoreFile::EncodeFile* functions.

7 files changed, 90 insertions, 112 deletions

diff --git a/bin/bbackupd/BackupClientDirectoryRecord.cpp b/bin/bbackupd/BackupClientDirectoryRecord.cpp
index 7ae85fcf..90caf2e7 100644
--- a/bin/bbackupd/BackupClientDirectoryRecord.cpp
+++ b/bin/bbackupd/BackupClientDirectoryRecord.cpp
@@ -28,6 +28,7 @@
 #include "BackupStoreException.h"
 #include "BackupStoreFile.h"
 #include "BackupStoreFileEncodeStream.h"
+#include "BufferedStream.h"
 #include "CommonException.h"
 #include "CollectInBufferStream.h"
 #include "FileModificationTime.h"
@@ -727,7 +728,7 @@ void BackupClientDirectoryRecord::UpdateAttributes(BackupClientDirectoryRecord::
 		BackupProtocolClient &connection(rParams.mrContext.GetConnection());
 
 		// Exception thrown if this doesn't work
-		MemBlockStream attrStream(attr);
+		std::auto_ptr<IOStream> attrStream(new MemBlockStream(attr));
 		connection.QueryChangeDirAttributes(mObjectID, attrModTime, attrStream);
 	}
 }
@@ -1195,8 +1196,10 @@ bool BackupClientDirectoryRecord::UpdateItems(
 					
 					// Update store
 					BackupClientFileAttributes attr;
-					attr.ReadAttributes(filename.c_str(), false /* put mod times in the attributes, please */);
-					MemBlockStream attrStream(attr);
+					attr.ReadAttributes(filename,
+						false /* put mod times in the attributes, please */);
+					std::auto_ptr<IOStream> attrStream(
+						new MemBlockStream(attr));
 					connection.QuerySetReplacementFileAttributes(mObjectID, attributesHash, storeFilename, attrStream);
 					fileSynced = true;
 				}
@@ -1458,7 +1461,7 @@ bool BackupClientDirectoryRecord::UpdateItems(
 				// in the else if(...) above will be correct.
 
 				// Build attribute stream for sending
-				MemBlockStream attrStream(attr);
+				std::auto_ptr<IOStream> attrStream(new MemBlockStream(attr));
 
 				if(renameDir)
 				{
@@ -1688,12 +1691,14 @@ int64_t BackupClientDirectoryRecord::UploadFile(
 
 	// Info
 	int64_t objID = 0;
-	bool doNormalUpload = true;
 	int64_t uploadedSize = -1;
 	
 	// Use a try block to catch store full errors
 	try
 	{
+		std::auto_ptr<BackupStoreFileEncodeStream> apStreamToUpload;
+		int64_t diffFromID = 0;
+
 		// Might an old version be on the server, and is the file
 		// size over the diffing threshold?
 		if(!NoPreviousVersionOnServer &&
@@ -1702,7 +1707,7 @@ int64_t BackupClientDirectoryRecord::UploadFile(
 			// YES -- try to do diff, if possible
 			// First, query the server to see if there's an old version available
 			std::auto_ptr<BackupProtocolSuccess> getBlockIndex(connection.QueryGetBlockIndexByName(mObjectID, rStoreFilename));
-			int64_t diffFromID = getBlockIndex->GetObjectID();
+			diffFromID = getBlockIndex->GetObjectID();
 			
 			if(diffFromID != 0)
 			{
@@ -1721,95 +1726,67 @@ int64_t BackupClientDirectoryRecord::UploadFile(
 
 				bool isCompletelyDifferent = false;
 
-				std::auto_ptr<IOStream> patchStream(
-					BackupStoreFile::EncodeFileDiff(
-						rFilename.c_str(),
-						mObjectID,	/* containing directory */
-						rStoreFilename, diffFromID, *blockIndexStream,
-						connection.GetTimeout(), 
-						&rContext, // DiffTimer implementation
-						0 /* not interested in the modification time */, 
-						&isCompletelyDifferent));
-	
-				rContext.UnManageDiffProcess();
-				rContext.SetNiceMode(true);
-
-				RateLimitingStream rateLimit(*patchStream,
-					rParams.mMaxUploadRate);
-				IOStream* pStreamToUpload;
+				apStreamToUpload = BackupStoreFile::EncodeFileDiff(
+					rFilename, 
+					mObjectID, /* containing directory */
+					rStoreFilename, diffFromID, *blockIndexStream,
+					connection.GetTimeout(), 
+					&rContext, // DiffTimer implementation
+					0 /* not interested in the modification time */, 
+					&isCompletelyDifferent);
 
-				if(rParams.mMaxUploadRate > 0)
+				if(isCompletelyDifferent)
 				{
-					pStreamToUpload = &rateLimit;
+					diffFromID = 0;
 				}
-				else
-				{
-					pStreamToUpload = patchStream.get();
-				}
-
-				//
-				// Upload the patch to the store
-				//
-				std::auto_ptr<BackupProtocolSuccess> stored(connection.QueryStoreFile(mObjectID, ModificationTime,
-						AttributesHash, isCompletelyDifferent?(0):(diffFromID), rStoreFilename, *pStreamToUpload));
-
-				rContext.SetNiceMode(false);
-				
-				// Get object ID from the result		
-				objID = stored->GetObjectID();
-
-				// Don't attempt to upload it again!
-				doNormalUpload = false;
-
-				// Capture number of bytes sent
-				uploadedSize = ((BackupStoreFileEncodeStream &)
-					*patchStream).GetTotalBytesSent();
+	
+				rContext.UnManageDiffProcess();
 			} 
 		}
 	
-		if(doNormalUpload)
+		if(!apStreamToUpload.get()) // No patch upload, so do a normal upload
 		{
 			// below threshold or nothing to diff from, so upload whole
 			rNotifier.NotifyFileUploading(this, rNonVssFilePath);
 			
 			// Prepare to upload, getting a stream which will encode the file as we go along
-			std::auto_ptr<IOStream> upload(
-				BackupStoreFile::EncodeFile(rFilename.c_str(),
-					mObjectID, rStoreFilename, NULL,
-					&rParams,
-					&(rParams.mrRunStatusProvider)));
+			apStreamToUpload = BackupStoreFile::EncodeFile(
+				rFilename, mObjectID, /* containing directory */
+				rStoreFilename, NULL, &rParams, 
+				&(rParams.mrRunStatusProvider));
+		}
 
-			rContext.SetNiceMode(true);
+		rContext.SetNiceMode(true);
+		std::auto_ptr<IOStream> apWrappedStream;
 
-			RateLimitingStream rateLimit(*upload,
-				rParams.mMaxUploadRate);
-			IOStream* pStreamToUpload;
+		if(rParams.mMaxUploadRate > 0)
+		{
+			apWrappedStream.reset(new RateLimitingStream(
+				*apStreamToUpload, rParams.mMaxUploadRate));
+		}
+		else
+		{
+			// Wrap the stream in *something*, so that
+			// QueryStoreFile() doesn't delete the original
+			// stream (upload object) and we can retrieve
+			// the byte counter.
+			apWrappedStream.reset(new BufferedStream(
+				*apStreamToUpload));
+		}
 
-			if(rParams.mMaxUploadRate > 0)
-			{
-				pStreamToUpload = &rateLimit;
-			}
-			else
-			{
-				pStreamToUpload = upload.get();
-			}
-	
-			// Send to store
-			std::auto_ptr<BackupProtocolSuccess> stored(
-				connection.QueryStoreFile(
-					mObjectID, ModificationTime,
-					AttributesHash, 
-					0 /* no diff from file ID */, 
-					rStoreFilename, *pStreamToUpload));
-
-			rContext.SetNiceMode(false);
-	
-			// Get object ID from the result		
-			objID = stored->GetObjectID();
+		// Send to store
+		std::auto_ptr<BackupProtocolSuccess> stored(
+			connection.QueryStoreFile(
+				mObjectID, ModificationTime,
+				AttributesHash, 
+				diffFromID,
+				rStoreFilename, apWrappedStream));
 
-			uploadedSize = ((BackupStoreFileEncodeStream &)
-				*upload).GetTotalBytesSent();
-		}
+		rContext.SetNiceMode(false);
+
+		// Get object ID from the result		
+		objID = stored->GetObjectID();
+		uploadedSize = apStreamToUpload->GetTotalBytesSent();
 	}
 	catch(BoxException &e)
 	{
diff --git a/bin/bbackupd/BackupDaemon.cpp b/bin/bbackupd/BackupDaemon.cpp
index 8133aec3..39bb98e3 100644
--- a/bin/bbackupd/BackupDaemon.cpp
+++ b/bin/bbackupd/BackupDaemon.cpp
@@ -2460,7 +2460,8 @@ void BackupDaemon::SetupLocations(BackupClientContext &rClientContext, const Con
 				// Execute create directory command
 				try
 				{
-					MemBlockStream attrStream(attr);
+					std::auto_ptr<IOStream> attrStream(
+						new MemBlockStream(attr));
 					std::auto_ptr<BackupProtocolSuccess>
 						dirCreate(connection.QueryCreateDirectory(
 						BackupProtocolListDirectory::RootDirectory,
diff --git a/lib/backupstore/BackupCommands.cpp b/lib/backupstore/BackupCommands.cpp
index aa4d3823..341c7368 100644
--- a/lib/backupstore/BackupCommands.cpp
+++ b/lib/backupstore/BackupCommands.cpp
@@ -207,7 +207,7 @@ std::auto_ptr<BackupProtocolMessage> BackupProtocolListDirectory::DoCommand(Back
 	stream->SetForReading();
 	
 	// Get the protocol to send the stream
-	rProtocol.SendStreamAfterCommand(stream.release());
+	rProtocol.SendStreamAfterCommand(static_cast< std::auto_ptr<IOStream> > (stream));
 
 	return std::auto_ptr<BackupProtocolMessage>(
 		new BackupProtocolSuccess(mObjectID));
@@ -300,7 +300,7 @@ std::auto_ptr<BackupProtocolMessage> BackupProtocolGetObject::DoCommand(BackupPr
 	std::auto_ptr<IOStream> object(rContext.OpenObject(mObjectID));
 
 	// Stream it to the peer
-	rProtocol.SendStreamAfterCommand(object.release());
+	rProtocol.SendStreamAfterCommand(object);
 
 	// Tell the caller what the file was
 	return std::auto_ptr<BackupProtocolMessage>(new BackupProtocolSuccess(mObjectID));
@@ -463,11 +463,8 @@ std::auto_ptr<BackupProtocolMessage> BackupProtocolGetFile::DoCommand(BackupProt
 	}
 
 	// Stream the reordered stream to the peer
-	rProtocol.SendStreamAfterCommand(stream.get());
+	rProtocol.SendStreamAfterCommand(stream);
 	
-	// Don't delete the stream here
-	stream.release();
-
 	// Tell the caller what the file was
 	return std::auto_ptr<BackupProtocolMessage>(new BackupProtocolSuccess(mObjectID));
 }
@@ -831,7 +828,7 @@ std::auto_ptr<BackupProtocolMessage> BackupProtocolGetObjectName::DoCommand(Back
 		// Get the stream ready to go
 		stream->SetForReading();	
 		// Tell the protocol to send the stream
-		rProtocol.SendStreamAfterCommand(stream.release());
+		rProtocol.SendStreamAfterCommand(static_cast< std::auto_ptr<IOStream> >(stream));
 	}
 
 	// Make reply
@@ -859,7 +856,7 @@ std::auto_ptr<BackupProtocolMessage> BackupProtocolGetBlockIndexByID::DoCommand(
 	BackupStoreFile::MoveStreamPositionToBlockIndex(*stream);
 	
 	// Return the stream to the client
-	rProtocol.SendStreamAfterCommand(stream.release());
+	rProtocol.SendStreamAfterCommand(stream);
 
 	// Return the object ID
 	return std::auto_ptr<BackupProtocolMessage>(new BackupProtocolSuccess(mObjectID));
@@ -911,7 +908,7 @@ std::auto_ptr<BackupProtocolMessage> BackupProtocolGetBlockIndexByName::DoComman
 	BackupStoreFile::MoveStreamPositionToBlockIndex(*stream);
 	
 	// Return the stream to the client
-	rProtocol.SendStreamAfterCommand(stream.release());
+	rProtocol.SendStreamAfterCommand(stream);
 
 	// Return the object ID
 	return std::auto_ptr<BackupProtocolMessage>(new BackupProtocolSuccess(objectID));
diff --git a/lib/backupstore/BackupStoreFile.cpp b/lib/backupstore/BackupStoreFile.cpp
index fe25eaee..519305ff 100644
--- a/lib/backupstore/BackupStoreFile.cpp
+++ b/lib/backupstore/BackupStoreFile.cpp
@@ -73,7 +73,7 @@ BackupStoreFileStats BackupStoreFile::msStats = {0,0,0};
 //		Created: 2003/08/28
 //
 // --------------------------------------------------------------------------
-std::auto_ptr<IOStream> BackupStoreFile::EncodeFile(
+std::auto_ptr<BackupStoreFileEncodeStream> BackupStoreFile::EncodeFile(
 	const std::string& Filename, int64_t ContainerID,
 	const BackupStoreFilename &rStoreFilename,
 	int64_t *pModificationTime,
@@ -81,13 +81,12 @@ std::auto_ptr<IOStream> BackupStoreFile::EncodeFile(
 	RunStatusProvider* pRunStatusProvider)
 {
 	// Create the stream
-	std::auto_ptr<IOStream> stream(new BackupStoreFileEncodeStream);
+	std::auto_ptr<BackupStoreFileEncodeStream> stream(
+		new BackupStoreFileEncodeStream);
 
 	// Do the initial setup
-	((BackupStoreFileEncodeStream*)stream.get())->Setup(Filename,
-		0 /* no recipe, just encode */,
-		ContainerID, rStoreFilename, pModificationTime, pLogger,
-		pRunStatusProvider);
+	stream->Setup(Filename, 0 /* no recipe, just encode */, ContainerID,
+		rStoreFilename, pModificationTime, pLogger, pRunStatusProvider);
 	
 	// Return the stream for the caller
 	return stream;
diff --git a/lib/backupstore/BackupStoreFile.h b/lib/backupstore/BackupStoreFile.h
index e24ff8cb..7c72e010 100644
--- a/lib/backupstore/BackupStoreFile.h
+++ b/lib/backupstore/BackupStoreFile.h
@@ -60,6 +60,8 @@ public:
 	virtual bool IsManaged() = 0;
 };
 
+class BackupStoreFileEncodeStream;
+
 // --------------------------------------------------------------------------
 //
 // Class
@@ -119,7 +121,7 @@ public:
 
 
 	// Main interface
-	static std::auto_ptr<IOStream> EncodeFile
+	static std::auto_ptr<BackupStoreFileEncodeStream> EncodeFile
 	(
 		const std::string& Filename,
 		int64_t ContainerID, const BackupStoreFilename &rStoreFilename,
@@ -127,7 +129,7 @@ public:
 		ReadLoggingStream::Logger* pLogger = NULL,
 		RunStatusProvider* pRunStatusProvider = NULL
 	);
-	static std::auto_ptr<IOStream> EncodeFileDiff
+	static std::auto_ptr<BackupStoreFileEncodeStream> EncodeFileDiff
 	(
 		const std::string& Filename, int64_t ContainerID,
 		const BackupStoreFilename &rStoreFilename, 
diff --git a/lib/backupstore/BackupStoreFileDiff.cpp b/lib/backupstore/BackupStoreFileDiff.cpp
index c8a28b78..fa8cb892 100644
--- a/lib/backupstore/BackupStoreFileDiff.cpp
+++ b/lib/backupstore/BackupStoreFileDiff.cpp
@@ -123,7 +123,7 @@ void BackupStoreFile::MoveStreamPositionToBlockIndex(IOStream &rStream)
 //		Created: 12/1/04
 //
 // --------------------------------------------------------------------------
-std::auto_ptr<IOStream> BackupStoreFile::EncodeFileDiff
+std::auto_ptr<BackupStoreFileEncodeStream> BackupStoreFile::EncodeFileDiff
 (
 	const std::string& Filename, int64_t ContainerID,
 	const BackupStoreFilename &rStoreFilename, int64_t DiffFromObjectID, 
@@ -206,10 +206,11 @@ std::auto_ptr<IOStream> BackupStoreFile::EncodeFileDiff
 		// foundBlocks no longer required
 		
 		// Create the stream
-		std::auto_ptr<IOStream> stream(new BackupStoreFileEncodeStream);
+		std::auto_ptr<BackupStoreFileEncodeStream> stream(
+			new BackupStoreFileEncodeStream);
 	
 		// Do the initial setup
-		((BackupStoreFileEncodeStream*)stream.get())->Setup(Filename, precipe, ContainerID, rStoreFilename, pModificationTime);
+		stream->Setup(Filename, precipe, ContainerID, rStoreFilename, pModificationTime);
 		precipe = 0;	// Stream has taken ownership of this
 		
 		// Tell user about completely different status?
diff --git a/lib/server/makeprotocol.pl.in b/lib/server/makeprotocol.pl.in
index ddf0509b..a074b435 100755
--- a/lib/server/makeprotocol.pl.in
+++ b/lib/server/makeprotocol.pl.in
@@ -541,7 +541,7 @@ public:
 
 	virtual std::auto_ptr<IOStream> ReceiveStream() = 0;
 	virtual int GetTimeout() = 0;
-	void SendStreamAfterCommand(IOStream *pStream);
+	void SendStreamAfterCommand(std::auto_ptr<IOStream> apStream);
 	
 protected:
 	std::list<IOStream*> mStreamsToSend;
@@ -573,10 +573,10 @@ $replyable_base_class\::$replyable_base_class()
 $replyable_base_class\::~$replyable_base_class()
 { }
 
-void $replyable_base_class\::SendStreamAfterCommand(IOStream *pStream)
+void $replyable_base_class\::SendStreamAfterCommand(std::auto_ptr<IOStream> apStream)
 {
-	ASSERT(pStream != NULL);
-	mStreamsToSend.push_back(pStream);
+	ASSERT(apStream.get() != NULL);
+	mStreamsToSend.push_back(apStream.release());
 }
 
 void $replyable_base_class\::DeleteStreamsToSend()
@@ -585,6 +585,7 @@ void $replyable_base_class\::DeleteStreamsToSend()
 	{
 		delete (*i);
 	}
+
 	mStreamsToSend.clear();
 }
 
@@ -668,8 +669,8 @@ for my $cmd (@cmd_list)
 	if(obj_is_type($cmd,'Command'))
 	{
 		my $has_stream = obj_is_type($cmd,'StreamWithCommand');
-		my $argextra = $has_stream?', IOStream &rStream':'';
-		my $queryextra = $has_stream?', rStream':'';
+		my $argextra = $has_stream?', std::auto_ptr<IOStream> apStream':'';
+		my $queryextra = $has_stream?', apStream':'';
 		my $request_class = $cmd_class{$cmd};
 		my $reply_class = $cmd_class{obj_get_type_params($cmd,'Command')};
 		
@@ -772,8 +773,8 @@ __E
 			if(obj_is_type($cmd,'Command'))
 			{
 				my $has_stream = obj_is_type($cmd,'StreamWithCommand');
-				my $argextra = $has_stream?', IOStream &rStream':'';
-				my $queryextra = $has_stream?', rStream':'';
+				my $argextra = $has_stream?', std::auto_ptr<IOStream> apStream':'';
+				my $queryextra = $has_stream?', apStream':'';
 				my $request_class = $cmd_class{$cmd};
 				my $reply_class = $cmd_class{obj_get_type_params($cmd,'Command')};
 				print H "\tstd::auto_ptr<$reply_class> Query(const $request_class &rQuery$argextra);\n";
@@ -1021,7 +1022,7 @@ __E
 				my $reply_class = $cmd_class{$reply_msg};
 				my $reply_id = $cmd_id{$reply_msg};
 				my $has_stream = obj_is_type($cmd,'StreamWithCommand');
-				my $argextra = $has_stream?', IOStream &rStream':'';
+				my $argextra = $has_stream?', std::auto_ptr<IOStream> apStream':'';
 				my $send_stream_extra = '';
 				my $send_stream_method = $writing_client ? "SendStream"
 					: "SendStreamAfterCommand";
@@ -1032,7 +1033,7 @@ __E
 					{
 						$send_stream_extra = <<__E;
 	// Send stream after the command
-	SendStream(rStream);
+	SendStream(*apStream);
 __E
 					}
 					
@@ -1059,7 +1060,7 @@ __E
 					{
 						$send_stream_extra = <<__E;
 	// Send stream after the command
-	SendStreamAfterCommand(&rStream);
+	SendStreamAfterCommand(apStream);
 __E
 					}