From c7253b157eab4edeb7960b8c0b6c8f8443e37275 Mon Sep 17 00:00:00 2001
From: "mingzhe.zou@easystack.cn" <mingzhe.zou@easystack.cn>
Date: Thu, 21 Oct 2021 13:07:42 +0800
Subject: [PATCH] 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 phantom 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 <mingzhe.zou@easystack.cn>
---
 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):