Merge pull request #5048 from finefoot/patch-8

Option to bypass locking to use sensible borg commands with read-only repositories

docs/usage/general.rst

@@ -31,6 +31,20 @@ All Borg commands share these options:
 
 .. include:: common-options.rst.inc
 
+Option ``--bypass-lock`` allows you to access the repository while bypassing
+borg's locking mechanism. This is necessary if your repository is on a read-only
+storage where you don't have write permissions or capabilities and therefore
+cannot create a lock. Examples are repositories stored on a Bluray disc or a
+read-only network storage. Avoid this option if you are able to use locks as
+that is the safer way; see the warning below.
+
+.. warning::
+
+    If you do use ``--bypass-lock``, you are responsible to ensure that no other
+    borg instances have write access to the repository. Otherwise, you might
+    experience errors and read broken data if changes to that repository are
+    being made at the same time.
+
 Examples
 ~~~~~~~~
 ::

src/borg/archiver.py

@@ -134,9 +134,19 @@ def with_repository(fake=False, invert_fake=False, create=False, lock=True,
         if create:
             compatibility = Manifest.NO_OPERATION_CHECK
 
+    # To process the `--bypass-lock` option if specified, we need to
+    # modify `lock` inside `wrapper`. Therefore we cannot use the
+    # `nonlocal` statement to access `lock` as modifications would also
+    # affect the scope outside of `wrapper`. Subsequent calls would
+    # only see the overwritten value of `lock`, not the original one.
+    # The solution is to define a place holder variable `_lock` to
+    # propagate the value into `wrapper`.
+    _lock = lock
+
     def decorator(method):
         @functools.wraps(method)
         def wrapper(self, args, **kwargs):
+            lock = getattr(args, 'lock', _lock)
             location = args.location  # note: 'location' must be always present in args
             append_only = getattr(args, 'append_only', False)
             storage_quota = getattr(args, 'storage_quota', None)
@@ -2561,6 +2571,9 @@ def define_common_options(add_common_option):
                               help='Output one JSON object per log line instead of formatted text.')
             add_common_option('--lock-wait', metavar='SECONDS', dest='lock_wait', type=int, default=1,
                               help='wait at most SECONDS for acquiring a repository/cache lock (default: %(default)d).')
+            add_common_option('--bypass-lock', dest='lock', action='store_false',
+                              default=argparse.SUPPRESS,  # only create args attribute if option is specified
+                              help='Bypass locking mechanism')
             add_common_option('--show-version', dest='show_version', action='store_true',
                               help='show/log the borg version')
             add_common_option('--show-rc', dest='show_rc', action='store_true',
@@ -4346,6 +4359,12 @@ def parse_args(self, args=None):
         if func == self.do_create and not args.paths:
             # need at least 1 path but args.paths may also be populated from patterns
             parser.error('Need at least one PATH argument.')
+        if not getattr(args, 'lock', True):  # Option --bypass-lock sets args.lock = False
+            bypass_allowed = {self.do_check, self.do_config, self.do_diff,
+                              self.do_export_tar, self.do_extract, self.do_info,
+                              self.do_list, self.do_mount, self.do_umount}
+            if func not in bypass_allowed:
+                raise Error('Not allowed to bypass locking mechanism for chosen command')
         return args
 
     def prerun_checks(self, logger, is_serve):

src/borg/repository.py

@@ -308,8 +308,15 @@ def save_config(self, path, config):
                 # some python ports have no os.link, see #4901
                 logger.warning(link_error_msg)
 
-        with SaveFile(config_path) as fd:
-            config.write(fd)
+        try:
+            with SaveFile(config_path) as fd:
+                config.write(fd)
+        except PermissionError as e:
+            # error is only a problem if we even had a lock
+            if self.do_lock:
+                raise
+            logger.warning("%s: Failed writing to '%s'. This is expected when working on "
+                           "read-only repositories." % (e.strerror, e.filename))
 
         if os.path.isfile(old_config_path):
             secure_erase(old_config_path)
@@ -325,7 +332,7 @@ def load_key(self):
         return keydata.encode('utf-8')  # remote repo: msgpack issue #99, returning bytes
 
     def get_free_nonce(self):
-        if not self.lock.got_exclusive_lock():
+        if self.do_lock and not self.lock.got_exclusive_lock():
             raise AssertionError("bug in code, exclusive lock should exist here")
 
         nonce_path = os.path.join(self.path, 'nonce')
@@ -336,14 +343,21 @@ def get_free_nonce(self):
             return None
 
     def commit_nonce_reservation(self, next_unreserved, start_nonce):
-        if not self.lock.got_exclusive_lock():
+        if self.do_lock and not self.lock.got_exclusive_lock():
             raise AssertionError("bug in code, exclusive lock should exist here")
 
         if self.get_free_nonce() != start_nonce:
             raise Exception("nonce space reservation with mismatched previous state")
         nonce_path = os.path.join(self.path, 'nonce')
-        with SaveFile(nonce_path, binary=False) as fd:
-            fd.write(bin_to_hex(next_unreserved.to_bytes(8, byteorder='big')))
+        try:
+            with SaveFile(nonce_path, binary=False) as fd:
+                fd.write(bin_to_hex(next_unreserved.to_bytes(8, byteorder='big')))
+        except PermissionError as e:
+            # error is only a problem if we even had a lock
+            if self.do_lock:
+                raise
+            logger.warning("%s: Failed writing to '%s'. This is expected when working on "
+                           "read-only repositories." % (e.strerror, e.filename))
 
     def destroy(self):
         """Destroy the repository at `self.path`
@@ -504,7 +518,7 @@ def open_index(self, transaction_id, auto_recover=True):
 
     def prepare_txn(self, transaction_id, do_cleanup=True):
         self._active_txn = True
-        if not self.lock.got_exclusive_lock():
+        if self.do_lock and not self.lock.got_exclusive_lock():
             if self.exclusive is not None:
                 # self.exclusive is either True or False, thus a new client is active here.
                 # if it is False and we get here, the caller did not use exclusive=True although

src/borg/testsuite/__init__.py

@@ -235,26 +235,66 @@ def _assert_dirs_equal_cmp(self, diff, ignore_flags=False, ignore_xattrs=False,
             self._assert_dirs_equal_cmp(sub_diff, ignore_flags=ignore_flags, ignore_xattrs=ignore_xattrs, ignore_ns=ignore_ns)
 
     @contextmanager
-    def fuse_mount(self, location, mountpoint, *options):
-        os.mkdir(mountpoint)
-        args = ['mount', location, mountpoint] + list(options)
-        self.cmd(*args, fork=True)
-        self.wait_for_mount(mountpoint)
+    def fuse_mount(self, location, mountpoint=None, *options, **kwargs):
+        if mountpoint is None:
+            mountpoint = tempfile.mkdtemp()
+        else:
+            os.mkdir(mountpoint)
+        if 'fork' not in kwargs:
+            # For a successful mount, `fork = True` is required for
+            # the borg mount daemon to work properly or the tests
+            # will just freeze. Therefore, if argument `fork` is not
+            # specified, the default value is `True`, regardless of
+            # `FORK_DEFAULT`. However, leaving the possibilty to run
+            # the command with `fork = False` is still necessary for
+            # testing for mount failures, for example attempting to
+            # mount a read-only repo.
+            kwargs['fork'] = True
+        self.cmd('mount', location, mountpoint, *options, **kwargs)
+        self.wait_for_mountstate(mountpoint, mounted=True)
         yield
         umount(mountpoint)
+        self.wait_for_mountstate(mountpoint, mounted=False)
         os.rmdir(mountpoint)
         # Give the daemon some time to exit
-        time.sleep(.2)
+        time.sleep(0.2)
 
-    def wait_for_mount(self, path, timeout=5):
-        """Wait until a filesystem is mounted on `path`
-        """
+    def wait_for_mountstate(self, mountpoint, *, mounted, timeout=5):
+        """Wait until a path meets specified mount point status"""
         timeout += time.time()
         while timeout > time.time():
-            if os.path.ismount(path):
+            if os.path.ismount(mountpoint) == mounted:
                 return
-            time.sleep(.1)
-        raise Exception('wait_for_mount(%s) timeout' % path)
+            time.sleep(0.1)
+        message = 'Waiting for %s of %s' % ('mount' if mounted else 'umount', mountpoint)
+        raise TimeoutError(message)
+
+    @contextmanager
+    def read_only(self, path):
+        """Some paths need to be made read-only for testing
+
+        Using chmod to remove write permissions is not enough due to
+        the tests running with root privileges. Instead, the folder is
+        rendered immutable with chattr or chflags, respectively.
+        """
+        if sys.platform.startswith('linux'):
+            cmd_immutable = 'chattr +i "%s"' % path
+            cmd_mutable = 'chattr -i "%s"' % path
+        elif sys.platform.startswith(('darwin', 'freebsd', 'netbsd', 'openbsd')):
+            cmd_immutable = 'chflags uchg "%s"' % path
+            cmd_mutable = 'chflags nouchg "%s"' % path
+        elif sys.platform.startswith('sunos'):  # openindiana
+            cmd_immutable = 'chmod S+vimmutable "%s"' % path
+            cmd_mutable = 'chmod S-vimmutable "%s"' % path
+        else:
+            message = 'Testing read-only repos is not supported on platform %s' % sys.platform
+            self.skipTest(message)
+        try:
+            os.system(cmd_immutable)
+            yield
+        finally:
+            # Restore permissions to ensure clean-up doesn't fail
+            os.system(cmd_mutable)
 
 
 class changedir:

src/borg/testsuite/archiver.py

@@ -51,6 +51,7 @@
 from ..nanorst import RstToTextLazy, rst_to_terminal
 from ..patterns import IECommand, PatternMatcher, parse_pattern
 from ..item import Item, ItemDiff
+from ..locking import LockFailed
 from ..logger import setup_logging
 from ..remote import RemoteRepository, PathNotAllowed
 from ..repository import Repository
@@ -1528,17 +1529,116 @@ def test_corrupted_repository(self):
         output = self.cmd('check', '--info', self.repository_location, exit_code=1)
         self.assert_in('Starting repository check', output)  # --info given for root logger
 
-    # we currently need to be able to create a lock directory inside the repo:
-    @pytest.mark.xfail(reason="we need to be able to create the lock directory inside the repo")
-    def test_readonly_repository(self):
+    def test_readonly_check(self):
         self.cmd('init', '--encryption=repokey', self.repository_location)
         self.create_src_archive('test')
-        os.system('chmod -R ugo-w ' + self.repository_path)
-        try:
-            self.cmd('extract', '--dry-run', self.repository_location + '::test')
-        finally:
-            # Restore permissions so shutil.rmtree is able to delete it
-            os.system('chmod -R u+w ' + self.repository_path)
+        with self.read_only(self.repository_path):
+            # verify that command normally doesn't work with read-only repo
+            if self.FORK_DEFAULT:
+                self.cmd('check', '--verify-data', self.repository_location, exit_code=EXIT_ERROR)
+            else:
+                with pytest.raises((LockFailed, RemoteRepository.RPCError)) as excinfo:
+                    self.cmd('check', '--verify-data', self.repository_location)
+                if isinstance(excinfo.value, RemoteRepository.RPCError):
+                    assert excinfo.value.exception_class == 'LockFailed'
+            # verify that command works with read-only repo when using --bypass-lock
+            self.cmd('check', '--verify-data', self.repository_location, '--bypass-lock')
+
+    def test_readonly_diff(self):
+        self.cmd('init', '--encryption=repokey', self.repository_location)
+        self.create_src_archive('a')
+        self.create_src_archive('b')
+        with self.read_only(self.repository_path):
+            # verify that command normally doesn't work with read-only repo
+            if self.FORK_DEFAULT:
+                self.cmd('diff', '%s::a' % self.repository_location, 'b', exit_code=EXIT_ERROR)
+            else:
+                with pytest.raises((LockFailed, RemoteRepository.RPCError)) as excinfo:
+                    self.cmd('diff', '%s::a' % self.repository_location, 'b')
+                if isinstance(excinfo.value, RemoteRepository.RPCError):
+                    assert excinfo.value.exception_class == 'LockFailed'
+            # verify that command works with read-only repo when using --bypass-lock
+            self.cmd('diff', '%s::a' % self.repository_location, 'b', '--bypass-lock')
+
+    def test_readonly_export_tar(self):
+        self.cmd('init', '--encryption=repokey', self.repository_location)
+        self.create_src_archive('test')
+        with self.read_only(self.repository_path):
+            # verify that command normally doesn't work with read-only repo
+            if self.FORK_DEFAULT:
+                self.cmd('export-tar', '%s::test' % self.repository_location, 'test.tar', exit_code=EXIT_ERROR)
+            else:
+                with pytest.raises((LockFailed, RemoteRepository.RPCError)) as excinfo:
+                    self.cmd('export-tar', '%s::test' % self.repository_location, 'test.tar')
+                if isinstance(excinfo.value, RemoteRepository.RPCError):
+                    assert excinfo.value.exception_class == 'LockFailed'
+            # verify that command works with read-only repo when using --bypass-lock
+            self.cmd('export-tar', '%s::test' % self.repository_location, 'test.tar', '--bypass-lock')
+
+    def test_readonly_extract(self):
+        self.cmd('init', '--encryption=repokey', self.repository_location)
+        self.create_src_archive('test')
+        with self.read_only(self.repository_path):
+            # verify that command normally doesn't work with read-only repo
+            if self.FORK_DEFAULT:
+                self.cmd('extract', '%s::test' % self.repository_location, exit_code=EXIT_ERROR)
+            else:
+                with pytest.raises((LockFailed, RemoteRepository.RPCError)) as excinfo:
+                    self.cmd('extract', '%s::test' % self.repository_location)
+                if isinstance(excinfo.value, RemoteRepository.RPCError):
+                    assert excinfo.value.exception_class == 'LockFailed'
+            # verify that command works with read-only repo when using --bypass-lock
+            self.cmd('extract', '%s::test' % self.repository_location, '--bypass-lock')
+
+    def test_readonly_info(self):
+        self.cmd('init', '--encryption=repokey', self.repository_location)
+        self.create_src_archive('test')
+        with self.read_only(self.repository_path):
+            # verify that command normally doesn't work with read-only repo
+            if self.FORK_DEFAULT:
+                self.cmd('info', self.repository_location, exit_code=EXIT_ERROR)
+            else:
+                with pytest.raises((LockFailed, RemoteRepository.RPCError)) as excinfo:
+                    self.cmd('info', self.repository_location)
+                if isinstance(excinfo.value, RemoteRepository.RPCError):
+                    assert excinfo.value.exception_class == 'LockFailed'
+            # verify that command works with read-only repo when using --bypass-lock
+            self.cmd('info', self.repository_location, '--bypass-lock')
+
+    def test_readonly_list(self):
+        self.cmd('init', '--encryption=repokey', self.repository_location)
+        self.create_src_archive('test')
+        with self.read_only(self.repository_path):
+            # verify that command normally doesn't work with read-only repo
+            if self.FORK_DEFAULT:
+                self.cmd('list', self.repository_location, exit_code=EXIT_ERROR)
+            else:
+                with pytest.raises((LockFailed, RemoteRepository.RPCError)) as excinfo:
+                    self.cmd('list', self.repository_location)
+                if isinstance(excinfo.value, RemoteRepository.RPCError):
+                    assert excinfo.value.exception_class == 'LockFailed'
+            # verify that command works with read-only repo when using --bypass-lock
+            self.cmd('list', self.repository_location, '--bypass-lock')
+
+    @unittest.skipUnless(has_llfuse, 'llfuse not installed')
+    def test_readonly_mount(self):
+        self.cmd('init', '--encryption=repokey', self.repository_location)
+        self.create_src_archive('test')
+        with self.read_only(self.repository_path):
+            # verify that command normally doesn't work with read-only repo
+            if self.FORK_DEFAULT:
+                with self.fuse_mount(self.repository_location, exit_code=EXIT_ERROR):
+                    pass
+            else:
+                with pytest.raises((LockFailed, RemoteRepository.RPCError)) as excinfo:
+                    # self.fuse_mount always assumes fork=True, so for this test we have to manually set fork=False
+                    with self.fuse_mount(self.repository_location, fork=False):
+                        pass
+                if isinstance(excinfo.value, RemoteRepository.RPCError):
+                    assert excinfo.value.exception_class == 'LockFailed'
+            # verify that command works with read-only repo when using --bypass-lock
+            with self.fuse_mount(self.repository_location, None, '--bypass-lock'):
+                pass
 
     @pytest.mark.skipif('BORG_TESTS_IGNORE_MODES' in os.environ, reason='modes unreliable')
     def test_umask(self):