Merge #1573

1573: Handle FS failures while updating (fixes #1560) r=heinrich5991 a=def-

Not sure if I caught all locations

Co-authored-by: def <dennis@felsin9.de>

src/engine/client/http.cpp

@@ -83,17 +83,25 @@ CRequest::CRequest(const char *pUrl, bool CanTimeout) :
 
 void CRequest::Run()
 {
-	if(BeforeInit())
+	if(!BeforeInit())
 	{
 		m_State = HTTP_ERROR;
+		OnCompletion();
 		return;
 	}
 
 	CURL *pHandle = curl_easy_init();
+	m_State = RunImpl(pHandle);
+	curl_easy_cleanup(pHandle);
+
+	OnCompletion();
+}
+
+int CRequest::RunImpl(CURL *pHandle)
+{
 	if(!pHandle)
 	{
-		m_State = HTTP_ERROR;
-		return;
+		return HTTP_ERROR;
 	}
 
 	if(g_Config.m_DbgCurl)
@@ -142,31 +150,28 @@ void CRequest::Run()
 	curl_easy_setopt(pHandle, CURLOPT_PROGRESSDATA, this);
 	curl_easy_setopt(pHandle, CURLOPT_PROGRESSFUNCTION, ProgressCallback);
 
-	if(AfterInit(pHandle))
+	if(!AfterInit(pHandle))
 	{
-		curl_easy_cleanup(pHandle);
-		m_State = HTTP_ERROR;
-		return;
+		return HTTP_ERROR;
 	}
 
 	dbg_msg("http", "http %s", m_aUrl);
 	m_State = HTTP_RUNNING;
 	int Ret = curl_easy_perform(pHandle);
-	BeforeCompletion();
+	if(!BeforeCompletion())
+	{
+		return HTTP_ERROR;
+	}
 	if(Ret != CURLE_OK)
 	{
 		dbg_msg("http", "task failed. libcurl error: %s", aErr);
-		m_State = (Ret == CURLE_ABORTED_BY_CALLBACK) ? HTTP_ABORTED : HTTP_ERROR;
+		return (Ret == CURLE_ABORTED_BY_CALLBACK) ? HTTP_ABORTED : HTTP_ERROR;
 	}
 	else
 	{
 		dbg_msg("http", "task done %s", m_aUrl);
-		m_State = HTTP_DONE;
+		return HTTP_DONE;
 	}
-
-	curl_easy_cleanup(pHandle);
-
-	OnCompletion();
 }
 
 size_t CRequest::WriteCallback(char *pData, size_t Size, size_t Number, void *pUser)
@@ -265,7 +270,7 @@ CGetFile::CGetFile(IStorage *pStorage, const char *pUrl, const char *pDest, int
 	str_copy(m_aDest, pDest, sizeof(m_aDest));
 }
 
-int CGetFile::BeforeInit()
+bool CGetFile::BeforeInit()
 {
 	char aPath[512];
 	if(m_StorageType == -2)
@@ -274,15 +279,18 @@ int CGetFile::BeforeInit()
 		m_pStorage->GetCompletePath(m_StorageType, m_aDest, aPath, sizeof(aPath));
 
 	if(fs_makedir_rec_for(aPath) < 0)
+	{
 		dbg_msg("http", "i/o error, cannot create folder for: %s", aPath);
+		return false;
+	}
 
 	m_File = io_open(aPath, IOFLAG_WRITE);
 	if(!m_File)
 	{
 		dbg_msg("http", "i/o error, cannot open file: %s", m_aDest);
-		return 1;
+		return false;
 	}
-	return 0;
+	return true;
 }
 
 size_t CGetFile::OnData(char *pData, size_t DataSize)
@@ -290,9 +298,9 @@ size_t CGetFile::OnData(char *pData, size_t DataSize)
 	return io_write(m_File, pData, DataSize);
 }
 
-void CGetFile::BeforeCompletion()
+bool CGetFile::BeforeCompletion()
 {
-	io_close(m_File);
+	return io_close(m_File) == 0;
 }
 
 CPostJson::CPostJson(const char *pUrl, bool CanTimeout, const char *pJson)
@@ -301,7 +309,7 @@ CPostJson::CPostJson(const char *pUrl, bool CanTimeout, const char *pJson)
 	str_copy(m_aJson, pJson, sizeof(m_aJson));
 }
 
-int CPostJson::AfterInit(void *pCurl)
+bool CPostJson::AfterInit(void *pCurl)
 {
 	CURL *pHandle = (CURL *)pCurl;
 
@@ -310,5 +318,5 @@ int CPostJson::AfterInit(void *pCurl)
 	curl_easy_setopt(pHandle, CURLOPT_HTTPHEADER, pHeaders);
 	curl_easy_setopt(pHandle, CURLOPT_POSTFIELDS, m_aJson);
 
-	return 0;
+	return true;
 }

src/engine/client/http.h

@@ -6,6 +6,7 @@
 #include <engine/kernel.h>
 
 typedef struct _json_value json_value;
+typedef void CURL;
 
 enum
 {
@@ -21,12 +22,12 @@ class CRequest : public IJob
 	// Abort the request with an error if `BeforeInit()` or `AfterInit()`
 	// returns something nonzero. Also abort the request if `OnData()`
 	// returns something other than `DataSize`.
-	virtual int BeforeInit() { return 0; }
-	virtual int AfterInit(void *pCurl) { return 0; }
+	virtual bool BeforeInit() { return true; }
+	virtual bool AfterInit(void *pCurl) { return true; }
 	virtual size_t OnData(char *pData, size_t DataSize) = 0;
 
 	virtual void OnProgress() { }
-	virtual void BeforeCompletion() { }
+	virtual bool BeforeCompletion() { return true; }
 	virtual void OnCompletion() { }
 
 	char m_aUrl[256];
@@ -43,6 +44,7 @@ class CRequest : public IJob
 	static size_t WriteCallback(char *pData, size_t Size, size_t Number, void *pUser);
 
 	void Run();
+	int RunImpl(CURL *pHandle);
 
 public:
 	CRequest(const char *pUrl, bool CanTimeout);
@@ -75,8 +77,8 @@ public:
 class CGetFile : public CRequest
 {
 	virtual size_t OnData(char *pData, size_t DataSize);
-	virtual int BeforeInit();
-	virtual void BeforeCompletion();
+	virtual bool BeforeInit();
+	virtual bool BeforeCompletion();
 
 	IStorage *m_pStorage;
 
@@ -94,7 +96,7 @@ public:
 class CPostJson : public CRequest
 {
 	virtual size_t OnData(char *pData, size_t DataSize) { return DataSize; }
-	virtual int AfterInit(void *pCurl);
+	virtual bool AfterInit(void *pCurl);
 
 	char m_aJson[1024];
 

src/engine/client/updater.cpp

@@ -135,23 +135,26 @@ void CUpdater::FetchFile(const char *pFile, const char *pDestPath)
 	m_pEngine->AddJob(std::make_shared<CUpdaterFetchTask>(this, pFile, pDestPath));
 }
 
-void CUpdater::MoveFile(const char *pFile)
+bool CUpdater::MoveFile(const char *pFile)
 {
 	char aBuf[256];
 	size_t len = str_length(pFile);
+	bool Success = true;
 
 	if(!str_comp_nocase(pFile + len - 4, ".dll") || !str_comp_nocase(pFile + len - 4, ".ttf"))
 	{
 		str_format(aBuf, sizeof(aBuf), "%s.old", pFile);
-		m_pStorage->RenameBinaryFile(pFile, aBuf);
+		Success &= m_pStorage->RenameBinaryFile(pFile, aBuf);
 		str_format(aBuf, sizeof(aBuf), "update/%s", pFile);
-		m_pStorage->RenameBinaryFile(aBuf, pFile);
+		Success &= m_pStorage->RenameBinaryFile(aBuf, pFile);
 	}
 	else
 	{
 		str_format(aBuf, sizeof(aBuf), "update/%s", pFile);
-		m_pStorage->RenameBinaryFile(aBuf, pFile);
+		Success &= m_pStorage->RenameBinaryFile(aBuf, pFile);
 	}
+
+	return Success;
 }
 
 void CUpdater::Update()
@@ -174,43 +177,53 @@ void CUpdater::AddFileJob(const char *pFile, bool Job)
 	m_FileJobs[string(pFile)] = Job;
 }
 
-void CUpdater::ReplaceClient()
+bool CUpdater::ReplaceClient()
 {
 	dbg_msg("updater", "replacing " PLAT_CLIENT_EXEC);
+	bool Success = true;
 
 	// Replace running executable by renaming twice...
 	if(!m_IsWinXP)
 	{
 		m_pStorage->RemoveBinaryFile("DDNet.old");
-		m_pStorage->RenameBinaryFile(PLAT_CLIENT_EXEC, "DDNet.old");
-		m_pStorage->RenameBinaryFile("update/DDNet.tmp", PLAT_CLIENT_EXEC);
+		Success &= m_pStorage->RenameBinaryFile(PLAT_CLIENT_EXEC, "DDNet.old");
+		Success &= m_pStorage->RenameBinaryFile("update/DDNet.tmp", PLAT_CLIENT_EXEC);
 	}
 	#if !defined(CONF_FAMILY_WINDOWS)
 		char aPath[512];
 		m_pStorage->GetBinaryPath(PLAT_CLIENT_EXEC, aPath, sizeof aPath);
 		char aBuf[512];
 		str_format(aBuf, sizeof aBuf, "chmod +x %s", aPath);
-		if (system(aBuf))
+		if(system(aBuf))
+		{
 			dbg_msg("updater", "ERROR: failed to set client executable bit");
+			Success &= false;
+		}
 	#endif
+	return Success;
 }
 
-void CUpdater::ReplaceServer()
+bool CUpdater::ReplaceServer()
 {
 	dbg_msg("updater", "replacing " PLAT_SERVER_EXEC);
+	bool Success = true;
 
 	//Replace running executable by renaming twice...
 	m_pStorage->RemoveBinaryFile("DDNet-Server.old");
-	m_pStorage->RenameBinaryFile(PLAT_SERVER_EXEC, "DDNet-Server.old");
-	m_pStorage->RenameBinaryFile("update/DDNet-Server.tmp", PLAT_SERVER_EXEC);
+	Success &= m_pStorage->RenameBinaryFile(PLAT_SERVER_EXEC, "DDNet-Server.old");
+	Success &= m_pStorage->RenameBinaryFile("update/DDNet-Server.tmp", PLAT_SERVER_EXEC);
 	#if !defined(CONF_FAMILY_WINDOWS)
 		char aPath[512];
 		m_pStorage->GetBinaryPath(PLAT_SERVER_EXEC, aPath, sizeof aPath);
 		char aBuf[512];
 		str_format(aBuf, sizeof aBuf, "chmod +x %s", aPath);
 		if (system(aBuf))
+		{
 			dbg_msg("updater", "ERROR: failed to set server executable bit");
+			Success &= false;
+		}
 	#endif
+	return Success;
 }
 
 void CUpdater::ParseUpdate()
@@ -323,15 +336,19 @@ void CUpdater::PerformUpdate()
 
 void CUpdater::CommitUpdate()
 {
+	bool Success = true;
+
 	for(map<std::string, bool>::iterator it = m_FileJobs.begin(); it != m_FileJobs.end(); ++it)
 		if(it->second)
-			MoveFile(it->first.c_str());
+			Success &= MoveFile(it->first.c_str());
 
 	if(m_ClientUpdate)
-		ReplaceClient();
+		Success &= ReplaceClient();
 	if(m_ServerUpdate)
-		ReplaceServer();
-	if(m_pClient->State() == IClient::STATE_ONLINE || m_pClient->EditorHasUnsavedData())
+		Success &= ReplaceServer();
+	if(!Success)
+		m_State = FAIL;
+	else if(m_pClient->State() == IClient::STATE_ONLINE || m_pClient->EditorHasUnsavedData())
 		m_State = NEED_RESTART;
 	else
 	{

src/engine/client/updater.h

@@ -56,14 +56,14 @@ class CUpdater : public IUpdater
 
 	void AddFileJob(const char *pFile, bool Job);
 	void FetchFile(const char *pFile, const char *pDestPath = 0);
-	void MoveFile(const char *pFile);
+	bool MoveFile(const char *pFile);
 
 	void ParseUpdate();
 	void PerformUpdate();
 	void CommitUpdate();
 
-	void ReplaceClient();
-	void ReplaceServer();
+	bool ReplaceClient();
+	bool ReplaceServer();
 
 	void SetCurrentState(int NewState);