From d0666276f85626cc9f9296457785427d2ce7d60d Mon Sep 17 00:00:00 2001 From: Mingzhe Zou Date: Tue, 19 Oct 2021 15:56:47 +0800 Subject: [PATCH 1/2] fix EOFError when execute targetcli commands concurrently 1.fcntl.LOCK_UN means unlock, but save file must own the exclusive lock. So, we must use fcntl.LOCK_EX(acquire an exclusive lock) flag in here. 2.Then, fcntl.lockf is different from fcntl.flock: a)fcntl.flock always lock for the whole file b)fcntl.lockf can only lock part of the file c)see https://docs.python.org/2.7/library/fcntl.html for details I think is better to call fcntl.flock in here. 3.open file via 'wb' will truncate the file without lock protection, if load prefs.bin at the same time, maybe read empty prefs.bin and raise EOFError. 4.However the patch only avoid EOFError, prefs may be replaced with the default value if read file and truncate file at the same time. A new patch need to commit, and lock before open prefs.bin to solve the prefs consistency problem. Signed-off-by: Zou Mingzhe --- configshell/prefs.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/configshell/prefs.py b/configshell/prefs.py index 0c3bd9e..c567be5 100644 --- a/configshell/prefs.py +++ b/configshell/prefs.py @@ -130,10 +130,12 @@ def save(self, filename=None): if filename is not None: fsock = open(filename, 'wb') - fcntl.lockf(fsock, fcntl.LOCK_UN) try: + fcntl.flock(fsock, fcntl.LOCK_EX) six.moves.cPickle.dump(self._prefs, fsock, 2) + fsock.flush() finally: + fcntl.flock(fsock, fcntl.LOCK_UN) fsock.close() def load(self, filename=None): @@ -144,10 +146,12 @@ def load(self, filename=None): if filename is None: filename = self.filename - if filename is not None and os.path.exists(filename): + if filename is not None and os.path.isfile(filename): fsock = open(filename, 'rb') - fcntl.lockf(fsock, fcntl.LOCK_SH) try: - self._prefs = six.moves.cPickle.load(fsock) + fcntl.flock(fsock, fcntl.LOCK_SH) + if os.path.getsize(filename) > 0: + self._prefs = six.moves.cPickle.load(fsock) finally: + fcntl.flock(fsock, fcntl.LOCK_UN) fsock.close() From 1d0e1ffc83b1f5a144ace042094616b736d54b8a Mon Sep 17 00:00:00 2001 From: "mingzhe.zou@easystack.cn" Date: Thu, 21 Oct 2021 13:07:42 +0800 Subject: [PATCH 2/2] fix prefs maybe replaced with the default value when read and truncate at the same time Open prefs.bin via 'wb' will truncate file without lock protection. So read prefs.bin and truncate prefs.bin at the same time, the preferences maybe replaced with the default value. Add a prefs.lock file to guarantee the atomicity of save and load operation. The patch can not solve old value read when targetcli command concurrency. That is, if change preference at same time as execute another targetcli command, next ConfigShell will load old prefs.bin file before previous ConfigShell save it. To fix this problem, targetcli-fb:scripts\targetcli must be modified. Signed-off-by: Zou Mingzhe --- configshell/prefs.py | 42 +++++++++++++++++++++++++++++------------- configshell/shell.py | 3 ++- 2 files changed, 31 insertions(+), 14 deletions(-) diff --git a/configshell/prefs.py b/configshell/prefs.py index c567be5..4b83f67 100644 --- a/configshell/prefs.py +++ b/configshell/prefs.py @@ -33,7 +33,7 @@ class Prefs(object): autosave = False __borg_state = {} - def __init__(self, filename=None): + def __init__(self, filename=None, lockfile=None): ''' Instanciates the ConfigShell preferences object. @param filename: File to store the preferencces to. @@ -42,6 +42,11 @@ def __init__(self, filename=None): self.__dict__ = self.__borg_state if filename is not None: self.filename = filename + if lockfile is None: + prefs_dir = os.getenv("TARGETCLI_HOME", '~/.targetcli') + prefs_dir = os.path.expanduser(prefs_dir) + lockfile = os.path.join(prefs_dir, 'prefs.lock') + self.lockfile = lockfile def __getitem__(self, key): ''' @@ -129,14 +134,20 @@ def save(self, filename=None): filename = self.filename if filename is not None: - fsock = open(filename, 'wb') + flk = open(self.lockfile, 'wb') + fcntl.flock(flk, fcntl.LOCK_EX) try: - fcntl.flock(fsock, fcntl.LOCK_EX) - six.moves.cPickle.dump(self._prefs, fsock, 2) - fsock.flush() + fpref = open(filename, 'wb') + try: + fcntl.flock(fpref, fcntl.LOCK_EX) + six.moves.cPickle.dump(self._prefs, fpref, 2) + fpref.flush() + finally: + fcntl.flock(fpref, fcntl.LOCK_UN) + fpref.close() finally: - fcntl.flock(fsock, fcntl.LOCK_UN) - fsock.close() + fcntl.flock(flk, fcntl.LOCK_UN) + flk.close() def load(self, filename=None): ''' @@ -147,11 +158,16 @@ def load(self, filename=None): filename = self.filename if filename is not None and os.path.isfile(filename): - fsock = open(filename, 'rb') + flk = open(self.lockfile, 'wb') + fcntl.flock(flk, fcntl.LOCK_SH) try: - fcntl.flock(fsock, fcntl.LOCK_SH) - if os.path.getsize(filename) > 0: - self._prefs = six.moves.cPickle.load(fsock) + fpref = open(filename, 'rb') + try: + fcntl.flock(fpref, fcntl.LOCK_SH) + self._prefs = six.moves.cPickle.load(fpref) + finally: + fcntl.flock(fpref, fcntl.LOCK_UN) + fpref.close() finally: - fcntl.flock(fsock, fcntl.LOCK_UN) - fsock.close() + fcntl.flock(flk, fcntl.LOCK_UN) + flk.close() diff --git a/configshell/shell.py b/configshell/shell.py index 437186d..0e10bcf 100644 --- a/configshell/shell.py +++ b/configshell/shell.py @@ -139,7 +139,8 @@ def __init__(self, preferences_dir=None): if not os.path.exists(preferences_dir): os.makedirs(preferences_dir) self._prefs_file = preferences_dir + '/prefs.bin' - self.prefs = prefs.Prefs(self._prefs_file) + self._prefs_lock = preferences_dir + '/prefs.lock' + self.prefs = prefs.Prefs(self._prefs_file, self._prefs_lock) self._cmd_history = preferences_dir + '/history.txt' self._save_history = True if not os.path.isfile(self._cmd_history):