[Box Backup-dev] Bug: Compare stopping on locked files

[Ben Summers]

Pascal,

Thanks for the patch, which shows the root cause of the problem. I  
think we need to refine it a bit though.

* You don't actually need to understand the contents of the stream  
from the server, merely discarding all the data it contains is  
sufficient.

* Having said that, we should really be testing for readability  
before opening the file and requesting the data from the server in  
the first place.

* And finally, if there are errors, they need to be reported in the  
summary from the compare.

Ben




On 20 Jun 2006, at 22:50, Pascal Lalonde wrote:

> On win32, when using bbackupquery, a compare command fails for a given
> location when it encounters a locked file, which is so common in
> Windows, especially on multiuser systems.
>
> I never get to see a complete compare.
>
> This is the output from the command-line:
>
> ---------------
> Box Backup Query Tool vtrunk_625, (c) Ben Summers and contributors  
> 2003-2006
> Using configuration file bbackupd.conf
> Connecting to store...
> Handshake with store...
> Login to store...
> Login complete.
>
> Type "help" for a list of commands.
>
> WARNING: Quick compare used -- file attributes are not checked.
> Unable to send message to Event Log: error 6:
> Failed to open file C:\miranda\\blah.dat: error 32
> ERROR: (1/2) during file fetch and comparison for '/miranda/blah.dat'
> ERROR: (7/42) during file fetch and comparison for '/miranda/ 
> miranda32.exe'
> ERROR: (7/42) during file fetch and comparison for '/miranda/ 
> mirandaboot.ini'
>
> [ 0 (of 0) differences probably due to file modifications after the  
> last upload
> ]
> Differences: 0 (0 dirs excluded, 0 files excluded)
> Logging off...
> Exception: Connection Protocol_ObjTooBig (7/42)
> ---------------
>
>
> Error 32 means a sharing violation:
> http://msdn.microsoft.com/library/en-us/debug/base/ 
> system_error_codes__0-499_.asp?frame=true
>
> It should continue even when encountering such a file.
>
> After digging in the code, I noticed that this bug should affect  
> the *nix version as well, but since mandatory locks seldom occur,  
> this bug has never bitten me on Linux and OpenBSD.
>
> The problem occurs in BackupQueries.cpp, at line 1425 (trunk, rev.  
> 626). Since the file is locked, the FileStream() constructor in  
> CompareFileContentsAgainstBlockIndex() throws an exception, which  
> goes all to the catch() block where an error is printed, and then  
> the loop proceeds to the next file. The problem is, before that  
> throw(), the download for our locked file had already been  
> "scheduled". Thus, when reading the stream, I guess what we get is  
> the file from the previous iteration since mrConnection is not  
> flushed on error.
>
> Here is a patch which fixes this issue. I'm not sure if it is the  
> right solution, but it should at least outline the source of the  
> problem.
>
> Pascal
> Index: lib/backupclient/BackupStoreFile.cpp
> ===================================================================
> --- lib/backupclient/BackupStoreFile.cpp	(revision 626)
> +++ lib/backupclient/BackupStoreFile.cpp	(working copy)
> @@ -1225,7 +1225,51 @@
>  }
>
>
> +//  
> ---------------------------------------------------------------------- 
> ----
> +//
> +// Function
> +//		Name:    BackupStoreFile::FlushStream(IOStream &, int)
> +//		Purpose: Flushes the stream. Returns true on success.
> +//		Created: 20/6/06
> +//
> +//  
> ---------------------------------------------------------------------- 
> ----
> +bool BackupStoreFile::FlushStream(IOStream &rBlockIndex, int Timeout)
> +{
> +	// Read header
> +	file_BlockIndexHeader hdr;
> +	if(!rBlockIndex.ReadFullBuffer(&hdr, sizeof(hdr), 0 /* not  
> interested in bytes read if this fails */, Timeout))
> +	{
> +		// Couldn't read header
> +		THROW_EXCEPTION(BackupStoreException,  
> CouldntReadEntireStructureFromStream)
> +	}
>
> +	// Check magic
> +	if(hdr.mMagicValue != (int32_t)htonl 
> (OBJECTMAGIC_FILE_BLOCKS_MAGIC_VALUE_V1)
> +#ifndef BOX_DISABLE_BACKWARDS_COMPATIBILITY_BACKUPSTOREFILE
> +		&& hdr.mMagicValue != (int32_t)htonl 
> (OBJECTMAGIC_FILE_BLOCKS_MAGIC_VALUE_V0)
> +#endif
> +		)
> +	{
> +		THROW_EXCEPTION(BackupStoreException, BadBackupStoreFile)
> +	}
> +
> +	// Get basic information
> +	int64_t numBlocks = box_ntoh64(hdr.mNumBlocks);
> +	
> +	for(int64_t b = 0; b < numBlocks; ++b)
> +	{
> +		// Read an entry from the stream
> +		file_BlockIndexEntry entry;
> +		if(!rBlockIndex.ReadFullBuffer(&entry, sizeof(entry), 0 /* not  
> interested in bytes read if this fails */, Timeout))
> +		{
> +			// Couldn't read entry
> +			THROW_EXCEPTION(BackupStoreException,  
> CouldntReadEntireStructureFromStream)
> +		}	
> +	}
> +	
> +	return true;
> +}
> +
>  //  
> ---------------------------------------------------------------------- 
> ----
>  //
>  // Function
> Index: lib/backupclient/BackupStoreFile.h
> ===================================================================
> --- lib/backupclient/BackupStoreFile.h	(revision 626)
> +++ lib/backupclient/BackupStoreFile.h	(working copy)
> @@ -133,6 +133,7 @@
>  	static void DecodeFile(IOStream &rEncodedFile, const char  
> *DecodedFilename, int Timeout, const BackupClientFileAttributes  
> *pAlterativeAttr = 0);
>  	static std::auto_ptr<BackupStoreFile::DecodedStream>  
> DecodeFileStream(IOStream &rEncodedFile, int Timeout, const  
> BackupClientFileAttributes *pAlterativeAttr = 0);
>  	static bool CompareFileContentsAgainstBlockIndex(const char  
> *Filename, IOStream &rBlockIndex, int Timeout);
> +	static bool FlushStream(IOStream &rBlockIndex, int Timeout);
>  	static std::auto_ptr<IOStream> CombineFileIndices(IOStream  
> &rDiff, IOStream &rFrom, bool DiffIsIndexOnly = false, bool  
> FromIsIndexOnly = false);
>
>  	// Stream manipulation
> Index: bin/bbackupquery/BackupQueries.cpp
> ===================================================================
> --- bin/bbackupquery/BackupQueries.cpp	(revision 626)
> +++ bin/bbackupquery/BackupQueries.cpp	(working copy)
> @@ -1422,7 +1422,15 @@
>  						std::auto_ptr<IOStream> blockIndexStream 
> (mrConnection.ReceiveStream());
>  						
>  						// Compare
> -						equal = BackupStoreFile::CompareFileContentsAgainstBlockIndex 
> (localPath.c_str(), *blockIndexStream, mrConnection.GetTimeout());
> +						try
> +						{
> +							equal =  
> BackupStoreFile::CompareFileContentsAgainstBlockIndex 
> (localPath.c_str(), *blockIndexStream, mrConnection.GetTimeout());
> +						}
> +						catch(...)
> +						{
> +							BackupStoreFile::FlushStream(*blockIndexStream,  
> mrConnection.GetTimeout());
> +							throw;
> +						}
>  					}
>  					else
>  					{