Convert RaidFileWrite to use FileStream to write stripes

Enable exclusive locking of stripe files on all platforms, not just Windows and O_EXLOCK.
boxbackup · qris · May 15, 2017 · May 15, 2017 · May 15, 2017 · May 15, 2017
commit d6714507bd965b4b37a335328e9ce07dfad21fbb
diff --git a/lib/common/CommonException.txt b/lib/common/CommonException.txt
@@ -60,5 +60,4 @@ InvalidConfiguration			52	Some required values are missing or incorrect in the c
 IOStreamTimedOut			53	A network operation timed out
 SignalReceived				54	The function call returned because the process received a signal
 NamedLockFailed				55	Failed to acquire a named lock
-FileLockFailed				56	Failed to acquire a lock on a file for an unexpected reason
-FileLockingConflict			57	The file is already locked by another process
+FileLockingConflict			56	The file is already locked by another process
diff --git a/lib/common/FileStream.cpp b/lib/common/FileStream.cpp
@@ -8,12 +8,21 @@
 // --------------------------------------------------------------------------
 
 #include "Box.h"
+
+#include <errno.h>
+
+#ifdef HAVE_FCNTL_G
+	#include <fcntl.h>
+#endif
+
+#ifdef HAVE_SYS_FILE_H
+	#include <sys/file.h>
+#endif
+
 #include "FileStream.h"
 #include "CommonException.h"
 #include "Logging.h"
 
-#include <errno.h>
-
 #include "MemLeakFindOn.h"
 
 // --------------------------------------------------------------------------
@@ -24,11 +33,11 @@
 //		Created: 2003/07/31
 //
 // --------------------------------------------------------------------------
-FileStream::FileStream(const std::string& rFilename, int flags, int mode,
+FileStream::FileStream(const std::string& mFileName, int flags, int mode,
 	lock_mode_t lock_mode)
 : mOSFileHandle(INVALID_FILE),
   mIsEOF(false),
-  mFileName(rFilename)
+  mFileName(mFileName)
 {
 	OpenFile(flags, mode, lock_mode);
 }
@@ -144,63 +153,59 @@ void FileStream::OpenFile(int flags, int mode, lock_mode_t lock_mode)
 		}
 	}
 
-	try
-	{
-		bool lock_failed = false;
+	bool lock_failed = false;
 
 #ifdef BOX_LOCK_TYPE_FLOCK
-		BOX_TRACE("Trying to lock " << rFilename << " " << lock_message);
-		if(::flock(fd, (lock_mode == SHARED ? LOCK_SH : LOCK_EX) | LOCK_NB) != 0)
+	BOX_TRACE("Trying to lock " << mFileName << " " << lock_message);
+	if(::flock(mOSFileHandle, (lock_mode == SHARED ? LOCK_SH : LOCK_EX) | LOCK_NB) != 0)
+	{
+		Close();
+
+		if(errno == EWOULDBLOCK)
 		{
-			if(errno == EWOULDBLOCK)
-			{
-				lock_failed = true;
-			}
-			else
-			{
-				THROW_SYS_FILE_ERROR("Failed to lock lockfile " << lock_method_name,
-					mFileName, CommonException, OSFileError);
-			}
+			lock_failed = true;
 		}
+		else
+		{
+			THROW_SYS_FILE_ERROR("Failed to lock lockfile " << lock_method_name,
+				mFileName, CommonException, OSFileError);
+		}
+	}
 #elif defined BOX_LOCK_TYPE_F_SETLK || defined BOX_LOCK_TYPE_F_OFD_SETLK
-		struct flock desc;
-		desc.l_type = (lock_mode == SHARED ? F_RDLCK : F_WRLCK);
-		desc.l_whence = SEEK_SET;
-		desc.l_start = 0;
-		desc.l_len = 0;
-		desc.l_pid = 0;
-		BOX_TRACE("Trying to lock " << mFileName << " " << lock_message);
+	struct flock desc;
+	desc.l_type = (lock_mode == SHARED ? F_RDLCK : F_WRLCK);
+	desc.l_whence = SEEK_SET;
+	desc.l_start = 0;
+	desc.l_len = 0;
+	desc.l_pid = 0;
+	BOX_TRACE("Trying to lock " << mFileName << " " << lock_message);
 #	if defined BOX_LOCK_TYPE_F_OFD_SETLK
-		if(::fcntl(fd, F_OFD_SETLK, &desc) != 0)
+	if(::fcntl(mOSFileHandle, F_OFD_SETLK, &desc) != 0)
 #	else // BOX_LOCK_TYPE_F_SETLK
-		if(::fcntl(fd, F_SETLK, &desc) != 0)
+	if(::fcntl(mOSFileHandle, F_SETLK, &desc) != 0)
 #	endif
+	{
+		Close();
+
+		if(errno == EAGAIN)
 		{
-			if(errno == EAGAIN)
-			{
-				lock_failed = true;
-			}
-			else
-			{
-				THROW_SYS_FILE_ERROR("Failed to lock lockfile " << lock_method_name,
-					mFileName, CommonException, OSFileError);
-			}
+			lock_failed = true;
 		}
-#endif
-
-		if(lock_failed)
+		else
 		{
-			// We failed to lock the file, which means that it's locked by someone else.
-			// Need to throw a specific exception that's expected by NamedLock, which
-			// should just return false in this case (and only this case).
-			THROW_EXCEPTION_MESSAGE(CommonException, FileLockingConflict,
-				BOX_FILE_MESSAGE(mFileName, "File already locked by another process"));
+			THROW_SYS_FILE_ERROR("Failed to lock lockfile " << lock_method_name,
+				mFileName, CommonException, OSFileError);
 		}
 	}
-	catch(BoxException &e)
+#endif
+
+	if(lock_failed)
 	{
-		THROW_FILE_ERROR("Failed to lock file: " << e.what(), mFileName,
-			CommonException, FileLockFailed);
+		// We failed to lock the file, which means that it's locked by someone else.
+		// Need to throw a specific exception that's expected by NamedLock, which
+		// should just return false in this case (and only this case).
+		THROW_EXCEPTION_MESSAGE(CommonException, FileLockingConflict,
+			BOX_FILE_MESSAGE(mFileName, "File already locked by another process"));
 	}
 }
 

diff --git a/lib/common/NamedLock.cpp b/lib/common/NamedLock.cpp
@@ -10,17 +10,6 @@
 
 #include "Box.h"
 
-#include <fcntl.h>
-#include <errno.h>
-
-#ifdef HAVE_UNISTD_H
-	#include <unistd.h>
-#endif
-
-#ifdef HAVE_FLOCK
-	#include <sys/file.h>
-#endif
-
 #include "CommonException.h"
 #include "NamedLock.h"
 #include "Utils.h"

diff --git a/lib/raidfile/RaidFileWrite.cpp b/lib/raidfile/RaidFileWrite.cpp
@@ -527,15 +527,12 @@ void RaidFileWrite::TransformToRaidStorage()
 	// Then open them all for writing (in strict order)
 	try
 	{
-#if HAVE_DECL_O_EXLOCK
-		FileHandleGuard<(O_WRONLY | O_CREAT | O_EXCL | O_EXLOCK | O_BINARY)> stripe1(stripe1FilenameW.c_str());
-		FileHandleGuard<(O_WRONLY | O_CREAT | O_EXCL | O_EXLOCK | O_BINARY)> stripe2(stripe2FilenameW.c_str());
-		FileHandleGuard<(O_WRONLY | O_CREAT | O_EXCL | O_EXLOCK | O_BINARY)> parity(parityFilenameW.c_str());
-#else
-		FileHandleGuard<(O_WRONLY | O_CREAT | O_EXCL | O_BINARY)> stripe1(stripe1FilenameW.c_str());
-		FileHandleGuard<(O_WRONLY | O_CREAT | O_EXCL | O_BINARY)> stripe2(stripe2FilenameW.c_str());
-		FileHandleGuard<(O_WRONLY | O_CREAT | O_EXCL | O_BINARY)> parity(parityFilenameW.c_str());
-#endif
+		FileStream stripe1(stripe1FilenameW, O_WRONLY | O_CREAT | O_EXCL | O_BINARY, 0755,
+			FileStream::EXCLUSIVE);
+		FileStream stripe2(stripe2FilenameW, O_WRONLY | O_CREAT | O_EXCL | O_BINARY, 0755,
+			FileStream::EXCLUSIVE);
+		FileStream parity(parityFilenameW, O_WRONLY | O_CREAT | O_EXCL | O_BINARY, 0755,
+			FileStream::EXCLUSIVE);
 
 		// Then... read in data...
 		int bytesRead = -1;
@@ -615,10 +612,7 @@ void RaidFileWrite::TransformToRaidStorage()
 				}
 
 				// Write block
-				if(::write(parity, parityBuffer, parityWriteSize) != parityWriteSize)
-				{
-					THROW_EXCEPTION(RaidFileException, OSError)
-				}
+				parity.Write(parityBuffer, parityWriteSize);
 			}
 
 			// Write stripes
@@ -629,11 +623,14 @@ void RaidFileWrite::TransformToRaidStorage()
 				int toWrite = (l == (blocksToDo - 1))
 								?(bytesRead - ((blocksToDo-1)*blockSize))
 								:blockSize;
-				if(::write(((l&1)==0)?stripe1:stripe2, writeFrom, toWrite) != toWrite)
+				if((l & 1) == 0)
+				{
+					stripe1.Write(writeFrom, toWrite);
+				}
+				else
 				{
-					THROW_SYS_FILE_ERROR("Failed to write to permanent RAID file",
-						writeFilename, RaidFileException, OSError);
-				}			
+					stripe2.Write(writeFrom, toWrite);
+				}
 
 				// Next block
 				writeFrom += blockSize;
@@ -642,6 +639,7 @@ void RaidFileWrite::TransformToRaidStorage()
 			// Count of blocks done
 			blocksDone += blocksToDo;
 		}
+
 		// Error on read?
 		if(bytesRead == -1)
 		{
@@ -661,12 +659,8 @@ void RaidFileWrite::TransformToRaidStorage()
 		{
 			ASSERT(sizeof(writeFileStat.st_size) <= sizeof(RaidFileRead::FileSizeType));
 			RaidFileRead::FileSizeType sw = box_hton64(writeFileStat.st_size);
-			ASSERT((::lseek(parity, 0, SEEK_CUR) % blockSize) == 0);
-			if(::write(parity, &sw, sizeof(sw)) != sizeof(sw))
-			{
-				THROW_SYS_FILE_ERROR("Failed to write to file", writeFilename,
-					RaidFileException, OSError);
-			}
+			ASSERT((parity.GetPosition() % blockSize) == 0);
+			parity.Write(&sw, sizeof(sw));
 		}
 
 		// Then close the written files (note in reverse order of opening)
@@ -675,7 +669,7 @@ void RaidFileWrite::TransformToRaidStorage()
 		stripe1.Close();
 
 #ifdef WIN32
-		// Must delete before renaming
+		// Must delete any existing file before renaming over it
 		#define CHECK_UNLINK(file) \
 		{ \
 			if (EMU_UNLINK(file) != 0 && errno != ENOENT) \