I'm not sure this new approach is more readable than the previous one, even if it is more PEP8 compliant. Having one None assignment and then two defs on undo_immutable in an already quite complex control flow is not ideal.
One alternative approach is to have an undo_cmd which can be [], ['chattr', '-i', filename], or ['chflags', 'nouchg', filename]?
More generally, I think this function requires a make_immutable contextmanager to be more idiomatic, but that's a bigger overhaul:
<details>
<summary>git diff on e864d2e1b4</summary>
diff --git a/test/functional/feature_reindex_readonly.py b/test/functional/feature_reindex_readonly.py
index 3a16e4786e..c0ee89adb4 100755
--- a/test/functional/feature_reindex_readonly.py
+++ b/test/functional/feature_reindex_readonly.py
@@ -6,6 +6,7 @@
- Start a node, generate blocks, then restart with -reindex after setting blk files to read-only
"""
+import contextlib
import os
import stat
import subprocess
@@ -19,32 +20,18 @@ class BlockstoreReindexTest(BitcoinTestFramework):
self.num_nodes = 1
self.extra_args = [["-fastprune"]]
- def reindex_readonly(self):
- self.log.debug("Generate block big enough to start second block file")
- fastprune_blockfile_size = 0x10000
- opreturn = "6a"
- nulldata = fastprune_blockfile_size * "ff"
- self.generateblock(self.nodes[0], output=f"raw({opreturn}{nulldata})", transactions=[])
- block_count = self.nodes[0].getblockcount()
- self.stop_node(0)
-
- assert (self.nodes[0].chain_path / "blocks" / "blk00000.dat").exists()
- assert (self.nodes[0].chain_path / "blocks" / "blk00001.dat").exists()
-
- self.log.debug("Make the first block file read-only")
- filename = self.nodes[0].chain_path / "blocks" / "blk00000.dat"
- filename.chmod(stat.S_IREAD)
-
- undo_immutable = None
- should_reindex = True
+ [@contextlib](/bitcoin-bitcoin/contributor/contextlib/).contextmanager
+ def make_immutable(self, filename):
+ """Try to make filename immutable, undo on exit. Yields whether it succeeded."""
+ undo_cmd = None
+ made_immutable = False
# Linux
try:
subprocess.run(['chattr'], stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL)
try:
subprocess.run(['chattr', '+i', filename], capture_output=True, check=True)
- def undo_immutable():
- subprocess.check_call(['chattr', '-i', filename])
-
+ undo_cmd = ['chattr', '-i', filename]
+ made_immutable = True
self.log.info("Made file immutable with chattr")
except subprocess.CalledProcessError as e:
self.log.warning(str(e))
@@ -53,19 +40,16 @@ class BlockstoreReindexTest(BitcoinTestFramework):
if e.stderr:
self.log.warning(f"stderr: {e.stderr}")
if os.getuid() == 0:
- self.log.warning("Return early on Linux under root, because chattr failed.")
self.log.warning("This should only happen due to missing capabilities in a container.")
self.log.warning("Make sure to --cap-add LINUX_IMMUTABLE if you want to run this test.")
- should_reindex = False
except Exception:
# macOS, and *BSD
try:
subprocess.run(['chflags'], stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL)
try:
subprocess.run(['chflags', 'uchg', filename], capture_output=True, check=True)
- def undo_immutable():
- subprocess.check_call(['chflags', 'nouchg', filename])
-
+ undo_cmd = ['chflags', 'nouchg', filename]
+ made_immutable = True
self.log.info("Made file immutable with chflags")
except subprocess.CalledProcessError as e:
self.log.warning(str(e))
@@ -73,19 +57,39 @@ class BlockstoreReindexTest(BitcoinTestFramework):
self.log.warning(f"stdout: {e.stdout}")
if e.stderr:
self.log.warning(f"stderr: {e.stderr}")
- if os.getuid() == 0:
- self.log.warning("Return early on BSD under root, because chflags failed.")
- should_reindex = False
except Exception:
pass
- if should_reindex:
- self.log.debug("Attempt to restart and reindex the node with the unwritable block file")
- with self.nodes[0].assert_debug_log(["Reindexing finished"], timeout=60):
- self.start_node(0, extra_args=['-reindex', '-fastprune'])
- assert_equal(block_count, self.nodes[0].getblockcount())
- if undo_immutable is not None:
- undo_immutable()
+ try:
+ yield made_immutable
+ finally:
+ if undo_cmd is not None:
+ subprocess.check_call(undo_cmd)
+
+ def reindex_readonly(self):
+ self.log.debug("Generate block big enough to start second block file")
+ fastprune_blockfile_size = 0x10000
+ opreturn = "6a"
+ nulldata = fastprune_blockfile_size * "ff"
+ self.generateblock(self.nodes[0], output=f"raw({opreturn}{nulldata})", transactions=[])
+ block_count = self.nodes[0].getblockcount()
+ self.stop_node(0)
+
+ assert (self.nodes[0].chain_path / "blocks" / "blk00000.dat").exists()
+ assert (self.nodes[0].chain_path / "blocks" / "blk00001.dat").exists()
+
+ self.log.debug("Make the first block file read-only")
+ filename = self.nodes[0].chain_path / "blocks" / "blk00000.dat"
+ filename.chmod(stat.S_IREAD)
+
+ with self.make_immutable(filename) as made_immutable:
+ if not made_immutable and os.getuid() == 0:
+ self.log.warning("Skipping reindex test on root without immutable flag support")
+ else:
+ self.log.debug("Attempt to restart and reindex the node with the unwritable block file")
+ with self.nodes[0].assert_debug_log(["Reindexing finished"], timeout=60):
+ self.start_node(0, extra_args=['-reindex', '-fastprune'])
+ assert_equal(block_count, self.nodes[0].getblockcount())
filename.chmod(0o777)
</details>
