Save configuration files atomically so corruption is less likely (#2417)

* Save configuration files atomically so corruption is less likely
* Save configs only if dirty

3 files changed, 23 insertions, 54 deletions

diff --git a/src/kvilib/ext/KviConfigurationFile.cpp b/src/kvilib/ext/KviConfigurationFile.cpp
index 8b75384a5..27dd54c49 100644
--- a/src/kvilib/ext/KviConfigurationFile.cpp
+++ b/src/kvilib/ext/KviConfigurationFile.cpp
@@ -32,6 +32,7 @@
 
 #include <QColor>
 #include <QRect>
+#include <QSaveFile>
 
 KviConfigurationFile::KviConfigurationFile(const QString & filename, FileMode f, bool bLocal8Bit)
 {
@@ -335,20 +336,11 @@ bool KviConfigurationFile::load()
 	return true;
 }
 
-bool KviConfigurationFile::ensureWritable()
+bool KviConfigurationFile::saveIfDirty()
 {
-	if(m_bReadOnly)
-		return false;
-
-	KviFile f(m_szFileName);
-	if(!f.open(QFile::WriteOnly | QFile::Truncate))
-		return false;
-	if(f.write("# KVIrc configuration file\n", 27) != 27)
-		return false;
-	if(!f.flush())
-		return false;
-	f.close();
-	return true;
+	if(!m_bDirty)
+		return true;
+	return save();
 }
 
 bool KviConfigurationFile::save()
@@ -409,9 +401,11 @@ bool KviConfigurationFile::save()
 	if(m_bReadOnly)
 		return false;
 
-	KviFile f(m_szFileName);
+	QSaveFile f(m_szFileName);
+
 	if(!f.open(QFile::WriteOnly | QFile::Truncate))
 		return false;
+
 	if(f.write("# KVIrc configuration file\n", 27) != 27)
 		return false;
 
@@ -454,7 +448,10 @@ bool KviConfigurationFile::save()
 		}
 		++it;
 	}
-	f.close();
+
+	if(!f.commit())
+		return false;
+
 	m_bDirty = false;
 	return true;
 }
@@ -907,25 +904,3 @@ unsigned char KviConfigurationFile::readUCharEntry(const QString & szKey, unsign
 	unsigned char iVal = (unsigned char)p_str->toUInt(&bOk);
 	return bOk ? iVal : iDefault;
 }
-
-#ifdef COMPILE_ON_WINDOWS
-
-//
-// On windows we need to override new and delete operators
-// to ensure that always the right new/delete pair is called for an object instance
-// This bug is present in all the classes exported by a module that
-// can be instantiated/destroyed from external modules.
-// (this is a well known bug described in Q122675 of MSDN)
-//
-
-void * KviConfigurationFile::operator new(size_t tSize)
-{
-	return KviMemory::allocate(tSize);
-}
-
-void KviConfigurationFile::operator delete(void * p)
-{
-	KviMemory::free(p);
-}
-
-#endif
diff --git a/src/kvilib/ext/KviConfigurationFile.h b/src/kvilib/ext/KviConfigurationFile.h
index 06e625f61..b2264550d 100644
--- a/src/kvilib/ext/KviConfigurationFile.h
+++ b/src/kvilib/ext/KviConfigurationFile.h
@@ -93,7 +93,8 @@ public:
 	bool readOnly() { return m_bReadOnly; };
 	void setReadOnly(bool bReadOnly) { m_bReadOnly = bReadOnly; };
 	bool dirty() { return m_bDirty; };
-	bool ensureWritable();
+	bool saveIfDirty();
+
 	//
 	// This sets the save path for the config file
 	// In this way you can load a system-wide read-only config file
@@ -160,15 +161,6 @@ public:
 	static void getFontProperties(KviCString & buffer, QFont * fnt);
 	static void setFontProperties(KviCString & str, QFont * fnt);
 
-#ifdef COMPILE_ON_WINDOWS
-	// On windows we need to override new and delete operators
-	// to ensure that always the right new/delete pair is called for an object instance
-	// This bug is present in all the classes exported by a module that
-	// can be instantiated/destroyed from external modules.
-	// (this is a well known bug described in Q122675 of MSDN)
-	void * operator new(size_t tSize);
-	void operator delete(void * p);
-#endif
 };
 
 #endif //!_KVI_CONFIG_H_INCLUDED_
diff --git a/src/kvirc/kernel/KviOptions.cpp b/src/kvirc/kernel/KviOptions.cpp
index 29ddf515d..f7e531dd1 100644
--- a/src/kvirc/kernel/KviOptions.cpp
+++ b/src/kvirc/kernel/KviOptions.cpp
@@ -915,15 +915,9 @@ void KviApplication::saveOptions()
 	saveRecentChannels();
 
 	getLocalKvircDirectory(buffer, Config, KVI_CONFIGFILE_MAIN);
+
 	KviConfigurationFile cfg(buffer, KviConfigurationFile::Write);
 
-	if(!cfg.ensureWritable())
-	{
-		QMessageBox::warning(nullptr, __tr2qs("Warning While Writing Configuration - KVIrc"),
-		    __tr2qs("I can't write to the main configuration file:\n\t%1\nPlease ensure the directory exists and that you have the proper permissions before continuing, "
-		            "or else any custom configuration will be lost.")
-		        .arg(buffer));
-	}
 	int i;
 
 #define WRITE_OPTIONS(_num, _table)                       \
@@ -971,6 +965,14 @@ void KviApplication::saveOptions()
 	WRITE_OPTIONS(KVI_NUM_MIRCCOLOR_OPTIONS, g_mirccolorOptionsTable)
 	WRITE_OPTIONS(KVI_NUM_ICCOLOR_OPTIONS, g_iccolorOptionsTable)
 
+	if(!cfg.saveIfDirty())
+	{
+		QMessageBox::warning(nullptr, __tr2qs("Warning While Writing Configuration - KVIrc"),
+		    __tr2qs("I can't write to the main configuration file:\n\t%1\nPlease ensure the directory exists and that you have the proper permissions before continuing, "
+		            "or else any custom configuration will be lost.")
+		        .arg(buffer));
+	}
+
 #undef WRITE_OPTIONS
 }