From be4b57059f242a14ef3c956fcb8f3c5f78f73372 Mon Sep 17 00:00:00 2001 From: Bram Duvigneau Date: Sat, 30 Apr 2016 15:40:56 +0200 Subject: [PATCH 1/5] Save configuration profiles and gesture maps as an atomic operation (refs issue #3165) Instead of directly writing the file, a temporary file is created and moved to the final location. This prevents corrupt configuration files being written. Added a fileUtils module with just a FaultTolerantFile context manager for now to facilitate this pattern. Please update miscDependencies submodule to a revision that includes the osmove module. --- source/config/__init__.py | 10 +++++++--- source/fileUtils.py | 16 ++++++++++++++++ source/inputCore.py | 4 +++- 3 files changed, 26 insertions(+), 4 deletions(-) create mode 100644 source/fileUtils.py diff --git a/source/config/__init__.py b/source/config/__init__.py index 97c2c055c5f..bb63688fe87 100644 --- a/source/config/__init__.py +++ b/source/config/__init__.py @@ -12,6 +12,7 @@ import ctypes import ctypes.wintypes import os +import osreplace import sys from cStringIO import StringIO import itertools @@ -25,6 +26,7 @@ import shlobj import baseObject import easeOfAccess +from fileUtils import FaultTolerantFile import winKernel import profileUpgrader from .configSpec import confspec @@ -498,10 +500,12 @@ def save(self): # Never save the config if running securely. return try: - self.profiles[0].write() + with FaultTolerantFile(self.profiles[0].filename) as f: + self.profiles[0].write(f) log.info("Base configuration saved") for name in self._dirtyProfiles: - self._profileCache[name].write() + with FaultTolerantFile(self._profileCache[name].filename) as f: + self._profileCache[name].write(f) log.info("Saved configuration profile %s" % name) self._dirtyProfiles.clear() except Exception as e: @@ -598,7 +602,7 @@ def renameProfile(self, oldName, newName): if oldName.lower() != newName.lower() and os.path.isfile(newFn): raise ValueError("A profile with the same name already exists: %s" % newName) - os.rename(oldFn, newFn) + osreplace.replace(oldFn, newFn) # Update any associated triggers. allTriggers = self.triggersToProfiles saveTrigs = False diff --git a/source/fileUtils.py b/source/fileUtils.py new file mode 100644 index 00000000000..8a26cf9bcf8 --- /dev/null +++ b/source/fileUtils.py @@ -0,0 +1,16 @@ +import os +import osreplace +from contextlib import contextmanager +from tempfile import NamedTemporaryFile +from logHandler import log + +@contextmanager +def FaultTolerantFile(name): + dirpath, filename = os.path.split(name) + with NamedTemporaryFile(dir=dirpath, prefix=filename, suffix='.tmp', delete=False) as f: + log.debug(f.name) + yield f + f.flush() + os.fsync(f) + f.close() + osreplace.replace(f.name, name) \ No newline at end of file diff --git a/source/inputCore.py b/source/inputCore.py index a3b37420400..8b815b0f44c 100644 --- a/source/inputCore.py +++ b/source/inputCore.py @@ -23,6 +23,7 @@ import speech import characterProcessing import config +from fileUtils import FaultTolerantFile import watchdog from logHandler import log import globalVars @@ -360,7 +361,8 @@ def save(self): else: outSect[script] = [outVal, gesture] - out.write() + with FaultTolerantFile(out.filename) as f: + out.write(f) class InputManager(baseObject.AutoPropertyObject): """Manages functionality related to input from the user. From 7940568969c0734d028469d224f7369946304e95 Mon Sep 17 00:00:00 2001 From: Bram Duvigneau Date: Thu, 16 Jun 2016 15:19:45 +0200 Subject: [PATCH 2/5] fileUtils: add copyright/license header --- source/fileUtils.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/source/fileUtils.py b/source/fileUtils.py index 8a26cf9bcf8..39a5a74c15f 100644 --- a/source/fileUtils.py +++ b/source/fileUtils.py @@ -1,3 +1,8 @@ +#fileUtils.py +#A part of NonVisual Desktop Access (NVDA) +#Copyright (C) 2006-2016 NV Access Limited +#This file is covered by the GNU General Public License. +#See the file COPYING for more details. import os import osreplace from contextlib import contextmanager From 99145700436e5f563f937b74a2f273ec0e52be40 Mon Sep 17 00:00:00 2001 From: Bram Duvigneau Date: Thu, 16 Jun 2016 15:20:30 +0200 Subject: [PATCH 3/5] Just use os.rename for the renaming of profiles, osmove.move is not needed here --- source/config/__init__.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/source/config/__init__.py b/source/config/__init__.py index bb63688fe87..743625d53ed 100644 --- a/source/config/__init__.py +++ b/source/config/__init__.py @@ -12,7 +12,6 @@ import ctypes import ctypes.wintypes import os -import osreplace import sys from cStringIO import StringIO import itertools @@ -602,7 +601,7 @@ def renameProfile(self, oldName, newName): if oldName.lower() != newName.lower() and os.path.isfile(newFn): raise ValueError("A profile with the same name already exists: %s" % newName) - osreplace.replace(oldFn, newFn) + os.rename(oldFn, newFn) # Update any associated triggers. allTriggers = self.triggersToProfiles saveTrigs = False From 4698e8b4fbea2472b01f3813ee5f8b1e306c2309 Mon Sep 17 00:00:00 2001 From: Reef Turner Date: Fri, 17 Mar 2017 12:54:50 +0800 Subject: [PATCH 4/5] use ctypes to call moveFileEx instead of osreplace So that no miscDeps update is required, use moveFileEx through ctypes. --- source/fileUtils.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/source/fileUtils.py b/source/fileUtils.py index 39a5a74c15f..993b12d5e40 100644 --- a/source/fileUtils.py +++ b/source/fileUtils.py @@ -4,7 +4,7 @@ #This file is covered by the GNU General Public License. #See the file COPYING for more details. import os -import osreplace +import ctypes from contextlib import contextmanager from tempfile import NamedTemporaryFile from logHandler import log @@ -18,4 +18,7 @@ def FaultTolerantFile(name): f.flush() os.fsync(f) f.close() - osreplace.replace(f.name, name) \ No newline at end of file + MOVEFILE_REPLACE_EXISTING = 1 + moveFileResult = ctypes.windll.kernel32.MoveFileExW(f.name, name, MOVEFILE_REPLACE_EXISTING) + if moveFileResult == 0: + raise ctypes.WinError() From 700db42565e9c7cc1260a99baf54a76a708fb76a Mon Sep 17 00:00:00 2001 From: Reef Turner Date: Fri, 17 Mar 2017 14:15:23 +0800 Subject: [PATCH 5/5] Review actions for #6987 --- source/fileUtils.py | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/source/fileUtils.py b/source/fileUtils.py index 993b12d5e40..cb34679af60 100644 --- a/source/fileUtils.py +++ b/source/fileUtils.py @@ -1,6 +1,6 @@ #fileUtils.py #A part of NonVisual Desktop Access (NVDA) -#Copyright (C) 2006-2016 NV Access Limited +#Copyright (C) 2017 NV Access Limited, Bram Duvigneau #This file is covered by the GNU General Public License. #See the file COPYING for more details. import os @@ -9,8 +9,23 @@ from tempfile import NamedTemporaryFile from logHandler import log +#: Constant; flag for MoveFileEx(). If a file with the destination filename already exists, it is overwritten. +MOVEFILE_REPLACE_EXISTING = 1 + @contextmanager def FaultTolerantFile(name): + '''Used to write out files in a more fault tolerant way. A temporary file is used, and replaces the + file `name' when the context manager scope ends and the the context manager __exit__ is called. This + means writing out the complete file can be performed with less concern of corrupting the original file + if the process is interrupted by windows shutting down. + + Usage: + with FaultTolerantFile("myFile.txt") as f: + f.write("This is a test") + + This creates a temporary file, and the writes actually happen on this temp file. At the end of the + `with` block, when `f` goes out of context the temporary file is closed and, this temporary file replaces "myFile.txt" + ''' dirpath, filename = os.path.split(name) with NamedTemporaryFile(dir=dirpath, prefix=filename, suffix='.tmp', delete=False) as f: log.debug(f.name) @@ -18,7 +33,6 @@ def FaultTolerantFile(name): f.flush() os.fsync(f) f.close() - MOVEFILE_REPLACE_EXISTING = 1 moveFileResult = ctypes.windll.kernel32.MoveFileExW(f.name, name, MOVEFILE_REPLACE_EXISTING) if moveFileResult == 0: raise ctypes.WinError()