Introduce ProcessError exception class

Until now we used the built in ChildProcessError exception class for our error reporting. This class essentially has two flaws: first, the attributes it uses (derived from OSError) do not match our intention. Instead of an 'errno' value we have a status code. The error string ('strerror') is the failed process' command line instead, and the 'filename' in our case is the gathered stderr output. Second, and much more importantly, using the 'filename' attribute for transferring the standard error message to the user has a significant drawback: it does not interpret newline characters as line breaks but rather prints the string '\n' instead. This is not only ugly but also unreadable for longer outputs such as Python exception backtraces. This change introduces a new exception class that we use throughout the program: ProcessError. The class provides a string formatting method that properly interprets newline characters, resulting in much nicer backtraces. On the downside, the exception is not built-in and has to be made available explicitly. However, since it is derived from ChildProcessError (as opposed to OSError) it can at the very least be used instead of the latter, which should be specific enough to not catch any unwanted exceptions. import subrepo ./:execute at 5d3a418640120eb0dedad672c1c62cb0f5c33eed import subrepo ./:cleanup at 30129de755f5f7a5b9647b792ca1373b94e15dcb

Author: d-e-s-o

btrfs-backup/src/deso/btrfs/main.py

@@ -29,6 +29,9 @@
 from deso.btrfs.commands import (
  checkFileString,
 )
+from deso.execute import (
+ ProcessError,
+)
 from sys import (
  argv as sysargv,
  stderr,
@@ -83,7 +86,7 @@ def run(method, subvolumes, src_repo, dst_repo, debug=False, **kwargs):
  program = Program(subvolumes, src_repo, dst_repo)
  method(program)(**kwargs)
  return 0
- except ChildProcessError as e:
+ except ProcessError as e:
  if debug:
  raise
  print("Execution failure:\n\"%s\"" % e, file=stderr)

btrfs-backup/src/deso/btrfs/repository.py

@@ -50,6 +50,7 @@
 )
 from deso.execute import (
  execute,
+ ProcessError,
 )
 from os import (
  curdir,
@@ -190,7 +191,7 @@ def isDir(directory):
 
  execute(*cmd, stderr=repository.stderr)
  return True
- except ChildProcessError:
+ except ProcessError:
  return False
 
  # TODO: We might want to move this invocation into _findRoot to avoid
@@ -210,7 +211,7 @@ def isDir(directory):
  # actually succeeds), and in case of the root directory it will be
  # matched here.
  return len(output) == 1 and output[0].endswith(_SHOW_IS_ROOT)
- except ChildProcessError:
+ except ProcessError:
  # Starting with btrfs-progs-3.17.3 the show command will return an
  # error code in case the supplied directory is not a btrfs
  # subvolume which will manifest as an exception here.

btrfs-backup/src/deso/btrfs/test/testMain.py

@@ -54,6 +54,7 @@
  execute,
  findCommand,
  pipeline,
+ ProcessError,
 )
 from os import (
  environ,
@@ -105,7 +106,7 @@ def testDurationParsingFailure(self):
  def testInvokeNoArguments(self):
  """Verify the intended output is printed when the program is run without arguments."""
  regex = "the following arguments are required"
- with self.assertRaisesRegex(ChildProcessError, regex):
+ with self.assertRaisesRegex(ProcessError, regex):
  execute(executable, "-m", "deso.btrfs.main")
 
 
@@ -139,7 +140,7 @@ def runAndTest(*args):
  runMain(*args)
  # The command should fail due to missing arguments.
  self.assertFalse(True)
- except ChildProcessError as e:
+ except ProcessError as e:
  string = str(e)
 
  self.assertRegex(string, "Usage:")
@@ -175,15 +176,15 @@ def runMain(*args):
  execute(executable, "-m", "deso.btrfs.main", *args)
 
  regex = r"Extension must not start"
- with self.assertRaisesRegex(ChildProcessError, regex):
+ with self.assertRaisesRegex(ProcessError, regex):
  runMain("backup", "--snapshot-ext=%senc" % extsep)
 
  regex = r"The last receive filter must contain"
- with self.assertRaisesRegex(ChildProcessError, regex):
+ with self.assertRaisesRegex(ProcessError, regex):
  runMain("backup", "--snapshot-ext=gz", "--recv-filter", "/bin/gzip")
 
  regex = r"The first send filter must contain"
- with self.assertRaisesRegex(ChildProcessError, regex):
+ with self.assertRaisesRegex(ProcessError, regex):
  runMain("restore", "--snapshot-ext=gz", "--send-filter", "/bin/gzip")
 
  # TODO: We should likely add a test to verify that the {file}
@@ -194,12 +195,12 @@ def runMain(*args):
  # snapshot-ext option but other arguments are missing so we still
  # bail out.
  regex = r"arguments are required"
- with self.assertRaisesRegex(ChildProcessError, regex):
+ with self.assertRaisesRegex(ProcessError, regex):
  runMain("backup", "--snapshot-ext=gz", "--recv-filter=/bin/gzip",
  "--recv-filter", "/bin/dd of={file}")
 
  regex = r"arguments are required"
- with self.assertRaisesRegex(ChildProcessError, regex):
+ with self.assertRaisesRegex(ProcessError, regex):
  runMain("restore", "--snapshot-ext=gz",
  "--send-filter=/bin/dd if={file}",
  "--send-filter", "/bin/gzip")
@@ -363,7 +364,7 @@ def testNoStderrRead(self):
  # followed by an error string containing the program's stderr
  # output.
  regex = r"subvol-2015-01-29_20:59:00$"
- with self.assertRaisesRegex(ChildProcessError, regex):
+ with self.assertRaisesRegex(ProcessError, regex):
  btrfsMain([argv[0]] + args.split())
 
 

btrfs-backup/src/deso/btrfs/test/testRepository.py

@@ -60,6 +60,7 @@
 from deso.execute import (
  execute,
  findCommand,
+ ProcessError,
 )
 from glob import (
  glob,
@@ -431,7 +432,7 @@ def testRepositorySyncFailsForNonExistentSubvolume(self):
  src = Repository(snaps)
  dst = Repository(backup)
 
- with self.assertRaisesRegex(ChildProcessError, regex):
+ with self.assertRaisesRegex(ProcessError, regex):
  syncRepos([m.path(directory)], src, dst)
 
  self.assertEqual(glob(join(snaps, "*")), [])
@@ -755,7 +756,7 @@ def syncAndRestore(send_filters=None, recv_filters=None):
  # decompressed by bzip2 which is bound to fail. This is our
  # way of verifying that actual compression and decompression
  # happens.
- with self.assertRaises(ChildProcessError):
+ with self.assertRaises(ProcessError):
  syncAndRestore(send_filters, list(reversed(recv_filters)))
 
  # Case 3) Sync and restore with properly ordered filters. Everything
@@ -787,7 +788,7 @@ def testRepositoryRestoreFailExists(self):
  make(m, "root", data=b"root")
 
  regex = r"%s.*exists and it is not a directory" % m.path("root")
- with self.assertRaisesRegex(ChildProcessError, regex):
+ with self.assertRaisesRegex(ProcessError, regex):
  restore([m.path("root")], bak, src)
 
 

execute/src/deso/execute/__init__.py

@@ -24,6 +24,7 @@
  execute,
  formatCommands,
  pipeline,
+ ProcessError,
  spring,
 )
 from deso.execute.util import (

execute/src/deso/execute/execute_.py

@@ -83,6 +83,56 @@
 )
 
 
+class ProcessError(ChildProcessError):
+ """A class enhancing OSError with proper attributes for our use case.
+
+ The OSError attributes errno, filename, and filename2 do not really
+ describe our use case. Most importantly, however, OSError does not
+ interpret the filename arguments in any way, meaning newline
+ characters will be printed directly as '\n' instead of resulting in
+ a line break.
+ """
+ def __init__(self, status, name, stderr=None):
+ super().__init__()
+
+ self._status = status
+ self._name = name
+ self._stderr = stderr
+
+
+ def __str__(self):
+ """Convert the error into a human readable string."""
+ s = "[Status {status:d}] {name}"
+ if self._stderr:
+ s += ": '{stderr}'"
+
+ # Note that even if _stderr is None (and the string does not contain
+ # the {stderr} string) we can apply the formatting for this key
+ # anyways.
+ s = s.format(status=self._status,
+ name=self._name,
+ stderr=self._stderr)
+ return s
+
+
+ @property
+ def status(self):
+ """Retrieve the status code of the failed process."""
+ return self._status
+
+
+ @property
+ def name(self):
+ """Retrieve the name/command of the process that failed."""
+ return self._name
+
+
+ @property
+ def stderr(self):
+ """Retrieve the stderr output, if any, of the process that failed."""
+ return self._stderr
+
+
 def execute(*args, stdin=None, stdout=None, stderr=b""):
  """Execute a program synchronously."""
  # Note that 'args' is a tuple. We do not want that so explicitly
@@ -242,7 +292,7 @@ def _wait(pids, commands, data_err, failed=None):
 
  if failed:
  error = data_err.decode("utf-8") if data_err else None
- raise ChildProcessError(status, failed, error)
+ raise ProcessError(status, failed, error)
 
 
 def _write(data):

execute/src/deso/execute/test/testExecute.py

@@ -24,6 +24,7 @@
  findCommand,
  formatCommands,
  pipeline as pipeline_,
+ ProcessError,
  spring as spring_
 )
 from deso.execute.execute_ import (
@@ -86,6 +87,29 @@ def spring(commands, stdout=None, stderr=None):
 
 class TestExecute(TestCase):
  """A test case for command execution functionality."""
+ def testProcessErrorNoStderr(self):
+ """Verify that when not reading stderr output we get a rather generic error report."""
+ tmp_file = mktemp()
+ regex = r"^\[Status [0-9]+\] %s %s$" % (_CAT, tmp_file)
+
+ with self.assertRaisesRegex(ProcessError, regex):
+ # Do not read from stderr here.
+ execute(_CAT, tmp_file)
+
+
+ def testProcessErrorFormattingStderr(self):
+ """Verify that the ProcessError class properly handles new lines."""
+ tmp_file = mktemp()
+ # Note that when using ChildProcessError the newline would not
+ # appear as a true newline but rather as the actual text '\n' (i.e.,
+ # \\n in a string). With ProcessError we are supposed to get a
+ # properly interpreted newline.
+ regex = r".*No such file or directory\n"
+
+ with self.assertRaisesRegex(ProcessError, regex):
+ execute(_CAT, tmp_file, stderr=b"")
+
+
  def testExecuteErrorEventToStringSingle(self):
  """Verify that our event to string conversion works as expected."""
  self.assertEqual(eventToString(POLLERR), "ERR")
@@ -152,7 +176,7 @@ def testExecuteAndRedirectError(self):
  regex = r"No such file or directory"
 
  with TemporaryFile() as file_:
- with self.assertRaises(ChildProcessError):
+ with self.assertRaises(ProcessError):
  execute(_CAT, path, stderr=file_.fileno())
 
  file_.seek(0)
@@ -184,7 +208,7 @@ def testExecuteRedirectAll(self):
 
  def testExecuteThrowsOnCommandFailure(self):
  """Verify that a failing command causes an exception to be raised."""
- with self.assertRaises(ChildProcessError):
+ with self.assertRaises(ProcessError):
  execute(_FALSE)
 
 
@@ -196,14 +220,14 @@ def testExecuteThrowsAndReportsError(self):
  with self.assertRaises(AssertionError):
  # This command should fail the assertion because reading from
  # standard error is not turned on and as a result the error message
- # printed on stderr will not be contained in the ChildProcessError
+ # printed on stderr will not be contained in the ProcessError
  # error message.
- with self.assertRaisesRegex(ChildProcessError, regex):
+ with self.assertRaisesRegex(ProcessError, regex):
  execute(_CAT, path)
 
  # When reading from stderr is turned on the exception must contain
  # the above phrase.
- with self.assertRaisesRegex(ChildProcessError, regex):
+ with self.assertRaisesRegex(ProcessError, regex):
  execute(_CAT, path, stderr=b"")
 
 
@@ -222,7 +246,7 @@ def testPipelineThrowsForFirstFailure(self):
  [cmd],
  ]
 
- with self.assertRaisesRegex(ChildProcessError, regex):
+ with self.assertRaisesRegex(ProcessError, regex):
  pipeline(commands, stderr=b"")
 
 
@@ -367,7 +391,7 @@ def testPipelineWithFailingCommand(self):
  # Run twice, once with failing command at the end and once somewhere
  # in the middle.
  for _ in range(2):
- with self.assertRaises(ChildProcessError):
+ with self.assertRaises(ProcessError):
  pipeline(commands)
 
  commands += [identity]
@@ -431,7 +455,7 @@ def testSpringError(self):
  cmd2,
  ]
 
- with self.assertRaisesRegex(ChildProcessError, regex):
+ with self.assertRaisesRegex(ProcessError, regex):
  spring(commands, stderr=b"")