Merge remote-tracking branch 'origin/pr/389'

* origin/pr/389:
  app: save qubes.xml with utils.replace_file()
  app: use suppress() in simple cases
  firewall: save firewall.xml with utils.replace_file()
  utils: take tweaked helper functions from storage/reflink
  storage/reflink: quote logged filenames

qubes/app.py

@@ -29,10 +29,10 @@ import logging
 import os
 import random
 import sys
-import tempfile
 import time
 import traceback
 import uuid
+from contextlib import suppress
 
 import asyncio
 import jinja2
@@ -280,11 +280,9 @@ class QubesHost:
 
         self.app.log.debug('QubesHost: no_cpus={} memory_total={}'.format(
             self.no_cpus, self.memory_total))
-        try:
+        with suppress(NotImplementedError):
             self.app.log.debug('QubesHost: xen_free_memory={}'.format(
                 self.get_free_xen_memory()))
-        except NotImplementedError:
-            pass
 
     @property
     def memory_total(self):
@@ -1103,18 +1101,12 @@ class Qubes(qubes.PropertyHolder):
         if not self.__locked_fh:
             self._acquire_lock(for_save=True)
 
-        fh_new = tempfile.NamedTemporaryFile(
+        with qubes.utils.replace_file(self._store, permissions=0o660,
-            prefix=self._store, delete=False)
+                                      close_on_success=False) as fh_new:
             lxml.etree.ElementTree(self.__xml__()).write(
                 fh_new, encoding='utf-8', pretty_print=True)
-        fh_new.flush()
+            with suppress(KeyError):  # group not found
-        try:
+                os.fchown(fh_new.fileno(), -1, grp.getgrnam('qubes').gr_gid)
-            os.chown(fh_new.name, -1, grp.getgrnam('qubes').gr_gid)
-            os.chmod(fh_new.name, 0o660)
-        except KeyError:  # group 'qubes' not found
-            # don't change mode if no 'qubes' group in the system
-            pass
-        os.rename(fh_new.name, self._store)
 
         # update stored mtime, in case of multiple save() calls without
         # loading qubes.xml again
@@ -1324,10 +1316,8 @@ class Qubes(qubes.PropertyHolder):
         """
 
         # first search for index, verbatim
-        try:
+        with suppress(KeyError):
             return self.labels[label]
-        except KeyError:
-            pass
 
         # then search for name
         for i in self.labels.values():
@@ -1335,10 +1325,8 @@ class Qubes(qubes.PropertyHolder):
                 return i
 
         # last call, if label is a number represented as str, search in indices
-        try:
+        with suppress(KeyError, ValueError):
             return self.labels[int(label)]
-        except (KeyError, ValueError):
-            pass
 
         raise qubes.exc.QubesLabelNotFoundError(label)
 
@@ -1477,7 +1465,7 @@ class Qubes(qubes.PropertyHolder):
                 # allow removed VM to reference itself
                 continue
             for prop in obj.property_list():
-                try:
+                with suppress(AttributeError):
                     if isinstance(prop, qubes.vm.VMProperty) and \
                             getattr(obj, prop.__name__) == vm:
                         self.log.error(
@@ -1489,8 +1477,6 @@ class Qubes(qubes.PropertyHolder):
                             "see 'journalctl -u qubesd -e' in dom0 for "
                             'details'.format(
                                 vm.name))
-                except AttributeError:
-                    pass
 
         assignments = vm.get_provided_assignments()
         if assignments:
@@ -1512,11 +1498,9 @@ class Qubes(qubes.PropertyHolder):
                 'updatevm',
                 'default_template',
         ):
-            try:
+            with suppress(AttributeError):
                 if getattr(self, propname) == vm:
                     delattr(self, propname)
-            except AttributeError:
-                pass
 
     @qubes.events.handler('property-pre-set:clockvm')
     def on_property_pre_set_clockvm(self, event, name, newvalue, oldvalue=None):

qubes/firewall.py

@@ -31,6 +31,7 @@ import asyncio
 import lxml.etree
 
 import qubes
+import qubes.utils
 import qubes.vm.qubesvm
 
 
@@ -577,14 +578,13 @@ class Firewall:
         xml_tree = lxml.etree.ElementTree(xml_root)
 
         try:
-            old_umask = os.umask(0o002)
+            with qubes.utils.replace_file(firewall_conf,
-            with open(firewall_conf, 'wb') as firewall_xml:
+                                          permissions=0o664) as tmp_io:
-                xml_tree.write(firewall_xml, encoding="UTF-8",
+                xml_tree.write(tmp_io, encoding='UTF-8', pretty_print=True)
-                    pretty_print=True)
-            os.umask(old_umask)
         except EnvironmentError as err:
-            self.vm.log.error("save error: {}".format(err))
+            msg='firewall save error: {}'.format(err)
-            raise qubes.exc.QubesException('save error: {}'.format(err))
+            self.vm.log.error(msg)
+            raise qubes.exc.QubesException(msg)
 
         self.vm.fire_event('firewall-changed')
 

qubes/storage/reflink.py

@@ -34,7 +34,7 @@ import subprocess
 import tempfile
 import platform
 import sys
-from contextlib import contextmanager, suppress
+from contextlib import suppress
 
 import qubes.storage
 import qubes.utils
@@ -234,7 +234,7 @@ class ReflinkVolume(qubes.storage.Volume):
     def _commit(self, path_from):
         self._add_revision()
         self._prune_revisions()
-        _fsync_path(path_from)
+        qubes.utils.fsync_path(path_from)
         _rename_file(path_from, self._path_clean)
 
     def _add_revision(self):
@@ -364,32 +364,16 @@ class ReflinkVolume(qubes.storage.Volume):
         return 0
 
 
-@contextmanager
 def _replace_file(dst):
-    ''' Yield a tempfile whose name starts with dst, creating the last
+    _make_dir(os.path.dirname(dst))
-        directory component if necessary. If the block does not raise
+    return qubes.utils.replace_file(
-        an exception, safely rename the tempfile to dst.
+        dst, permissions=0o600, log_level=logging.INFO)
-    '''
-    tmp_dir, prefix = os.path.split(dst + '~')
-    _make_dir(tmp_dir)
-    tmp = tempfile.NamedTemporaryFile(dir=tmp_dir, prefix=prefix, delete=False)
-    try:
-        yield tmp
-        tmp.flush()
-        os.fsync(tmp.fileno())
-        tmp.close()
-        _rename_file(tmp.name, dst)
-    except:
-        tmp.close()
-        _remove_file(tmp.name)
-        raise
 
-def _fsync_path(path):
+_rename_file = functools.partial(
-    fd = os.open(path, os.O_RDONLY)  # works for a file or a directory
+    qubes.utils.rename_file, log_level=logging.INFO)
-    try:
+
-        os.fsync(fd)
+_remove_file = functools.partial(
-    finally:
+    qubes.utils.remove_file, log_level=logging.INFO)
-        os.close(fd)
 
 def _make_dir(path):
     ''' mkdir path, ignoring FileExistsError; return whether we
@@ -397,35 +381,20 @@ def _make_dir(path):
     '''
     with suppress(FileExistsError):
         os.mkdir(path)
-        _fsync_path(os.path.dirname(path))
+        qubes.utils.fsync_path(os.path.dirname(path))
-        LOGGER.info('Created directory: %s', path)
+        LOGGER.info('Created directory: %r', path)
         return True
     return False
 
-def _remove_file(path):
-    with suppress(FileNotFoundError):
-        os.remove(path)
-        _fsync_path(os.path.dirname(path))
-        LOGGER.info('Removed file: %s', path)
-
 def _remove_empty_dir(path):
     try:
         os.rmdir(path)
-        _fsync_path(os.path.dirname(path))
+        qubes.utils.fsync_path(os.path.dirname(path))
-        LOGGER.info('Removed empty directory: %s', path)
+        LOGGER.info('Removed empty directory: %r', path)
     except OSError as ex:
         if ex.errno not in (errno.ENOENT, errno.ENOTEMPTY):
             raise
 
-def _rename_file(src, dst):
-    os.rename(src, dst)
-    dst_dir = os.path.dirname(dst)
-    src_dir = os.path.dirname(src)
-    _fsync_path(dst_dir)
-    if src_dir != dst_dir:
-        _fsync_path(src_dir)
-    LOGGER.info('Renamed file: %s -> %s', src, dst)
-
 def _resize_file(path, size):
     ''' Resize an existing file. '''
     with open(path, 'rb+') as file:
@@ -436,7 +405,7 @@ def _create_sparse_file(path, size):
     ''' Create an empty sparse file. '''
     with _replace_file(path) as tmp:
         tmp.truncate(size)
-        LOGGER.info('Created sparse file: %s', tmp.name)
+        LOGGER.info('Created sparse file: %r', tmp.name)
 
 def _update_loopdev_sizes(img):
     ''' Resolve img; update the size of loop devices backed by it. '''
@@ -466,9 +435,9 @@ def _copy_file(src, dst):
     with _replace_file(dst) as tmp_io:
         with open(src, 'rb') as src_io:
             if _attempt_ficlone(src_io, tmp_io):
-                LOGGER.info('Reflinked file: %s -> %s', src, tmp_io.name)
+                LOGGER.info('Reflinked file: %r -> %r', src, tmp_io.name)
                 return True
-        LOGGER.info('Copying file: %s -> %s', src, tmp_io.name)
+        LOGGER.info('Copying file: %r -> %r', src, tmp_io.name)
         cmd = 'cp', '--sparse=always', src, tmp_io.name
         p = subprocess.run(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE,
                            check=False)

qubes/utils.py

@@ -22,12 +22,16 @@
 
 import asyncio
 import hashlib
+import logging
 import random
 import string
 import os
+import os.path
 import re
 import socket
 import subprocess
+import tempfile
+from contextlib import contextmanager, suppress
 
 import pkg_resources
 
@@ -36,6 +40,8 @@ import docutils.core
 import docutils.io
 import qubes.exc
 
+LOGGER = logging.getLogger('qubes.utils')
+
 
 def get_timezone():
     # fc18
@@ -186,6 +192,58 @@ def match_vm_name_with_special(vm, name):
         return name[len('@type:'):] == vm.__class__.__name__
     return name == vm.name
 
+@contextmanager
+def replace_file(dst, *, permissions, close_on_success=True,
+                 logger=LOGGER, log_level=logging.DEBUG):
+    ''' Yield a tempfile whose name starts with dst. If the block does
+        not raise an exception, apply permissions and persist the
+        tempfile to dst (which is allowed to already exist). Otherwise
+        ensure that the tempfile is cleaned up.
+    '''
+    tmp_dir, prefix = os.path.split(dst + '~')
+    tmp = tempfile.NamedTemporaryFile(dir=tmp_dir, prefix=prefix, delete=False)
+    try:
+        yield tmp
+        tmp.flush()
+        os.fchmod(tmp.fileno(), permissions)
+        os.fsync(tmp.fileno())
+        if close_on_success:
+            tmp.close()
+        rename_file(tmp.name, dst, logger=logger, log_level=log_level)
+    except:
+        try:
+            tmp.close()
+        finally:
+            remove_file(tmp.name, logger=logger, log_level=log_level)
+        raise
+
+def rename_file(src, dst, *, logger=LOGGER, log_level=logging.DEBUG):
+    ''' Durably rename src to dst. '''
+    os.rename(src, dst)
+    dst_dir = os.path.dirname(dst)
+    src_dir = os.path.dirname(src)
+    fsync_path(dst_dir)
+    if src_dir != dst_dir:
+        fsync_path(src_dir)
+    logger.log(log_level, 'Renamed file: %r -> %r', src, dst)
+
+def remove_file(path, *, logger=LOGGER, log_level=logging.DEBUG):
+    ''' Durably remove the file at path, if it exists. Return whether
+        we removed it. '''
+    with suppress(FileNotFoundError):
+        os.remove(path)
+        fsync_path(os.path.dirname(path))
+        logger.log(log_level, 'Removed file: %r', path)
+        return True
+    return False
+
+def fsync_path(path):
+    fd = os.open(path, os.O_RDONLY)  # works for a file or a directory
+    try:
+        os.fsync(fd)
+    finally:
+        os.close(fd)
+
 @asyncio.coroutine
 def coro_maybe(value):
     if asyncio.iscoroutine(value):