From 7aa8d74ad48ae17fcbc3cd073a273ecbc91ed333 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Wed, 12 Jul 2017 10:41:22 +0200 Subject: [PATCH 1/6] tests: fix some FD leaks There are still much more of them... --- qubes/tests/__init__.py | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/qubes/tests/__init__.py b/qubes/tests/__init__.py index 0dac54f2..73b02e4f 100644 --- a/qubes/tests/__init__.py +++ b/qubes/tests/__init__.py @@ -692,7 +692,7 @@ class SystemTestsMixin(object): try: vm.remove_from_disk() - except: # pylint: disable=bare-except + except: # pylint: disable=bare-except pass del app.domains[vm.qid] @@ -706,10 +706,11 @@ class SystemTestsMixin(object): try: conn = libvirt.open(qubes.config.defaults['libvirt_uri']) dom = conn.lookupByName(vmname) - except: # pylint: disable=bare-except + except: # pylint: disable=bare-except pass else: cls._remove_vm_libvirt(dom) + conn.close() cls._remove_vm_disk(vmname) @@ -753,7 +754,7 @@ class SystemTestsMixin(object): subprocess.check_call(['sudo', 'lvremove', '-f'] + [vol.strip() for vol in volumes.splitlines() if ('/' + prefix) in vol], - stdout=open(os.devnull, 'w')) + stdout=subprocess.DEVNULL) except subprocess.CalledProcessError: pass @@ -827,7 +828,7 @@ class SystemTestsMixin(object): wait_count = 0 while subprocess.call(['xdotool', 'search', '--name', title], - stdout=open(os.path.devnull, 'w'), stderr=subprocess.STDOUT) \ + stdout=subprocess.DEVNULL, stderr=subprocess.STDOUT) \ == int(show): wait_count += 1 if wait_count > timeout*10: @@ -873,7 +874,7 @@ class SystemTestsMixin(object): # create a single partition p = subprocess.Popen(['sfdisk', '-q', '-L', vm.storage.root_img], stdin=subprocess.PIPE, - stdout=open(os.devnull, 'w'), + stdout=subprocess.DEVNULL, stderr=subprocess.STDOUT) p.communicate('2048,\n') assert p.returncode == 0, 'sfdisk failed' @@ -892,7 +893,7 @@ class SystemTestsMixin(object): '--target', 'i386-pc', '--modules', 'part_msdos ext2', '--boot-directory', mountpoint, loopdev], - stderr=open(os.devnull, 'w') + stderr=subprocess.DEVNULL ) grub_cfg = '{}/grub2/grub.cfg'.format(mountpoint) subprocess.check_call( @@ -927,7 +928,7 @@ class SystemTestsMixin(object): subprocess.check_call( ['dracut'] + dracut_args + [os.path.join(mountpoint, 'initrd')], - stderr=open(os.devnull, 'w') + stderr=subprocess.DEVNULL ) finally: subprocess.check_call(['sudo', 'umount', mountpoint]) From d7b968d867491c3f105758fa89c35354066a3aa2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Wed, 12 Jul 2017 10:42:05 +0200 Subject: [PATCH 2/6] tests: fix block devices tests when running on real system --- qubes/tests/devices_block.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/qubes/tests/devices_block.py b/qubes/tests/devices_block.py index 33620060..3ff76c14 100644 --- a/qubes/tests/devices_block.py +++ b/qubes/tests/devices_block.py @@ -110,6 +110,8 @@ class TestApp(object): self.env = jinja2.Environment( loader=jinja2.FileSystemLoader([ 'templates', + '/etc/qubes/templates', + '/usr/share/qubes/templates', ]), undefined=jinja2.StrictUndefined) self.domains = {} From e6d5337fa7cfd71bbd9982dfc8fd8ea5a1713ca5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Wed, 12 Jul 2017 10:42:47 +0200 Subject: [PATCH 3/6] api: cleanup already started servers when some later failed --- qubes/api/__init__.py | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/qubes/api/__init__.py b/qubes/api/__init__.py index b6c6dcb7..c2046ebb 100644 --- a/qubes/api/__init__.py +++ b/qubes/api/__init__.py @@ -398,7 +398,18 @@ def create_servers(*args, force=False, loop=None, **kwargs): shutil.chown(sock.getsockname(), group='qubes') servers.append(server) - + except: + for server in servers: + for sock in server.sockets: + try: + os.unlink(sock.getsockname()) + except FileNotFoundError: + pass + server.close() + if servers: + yield from asyncio.wait([ + server.wait_closed() for server in servers]) + raise finally: os.umask(old_umask) From f2f89c7b0c8d31d33bab54763fb55f6169825423 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Wed, 12 Jul 2017 10:43:48 +0200 Subject: [PATCH 4/6] storage: do not use deepcopy on volume configs There may be Pool or Volume object references, which is intentional to keep them as is - not copy whole Pool/Volume objects. --- qubes/storage/__init__.py | 1 + qubes/vm/appvm.py | 4 ++-- qubes/vm/dispvm.py | 4 +--- qubes/vm/qubesvm.py | 4 +--- 4 files changed, 5 insertions(+), 8 deletions(-) diff --git a/qubes/storage/__init__.py b/qubes/storage/__init__.py index 63ce351b..b15e9c0e 100644 --- a/qubes/storage/__init__.py +++ b/qubes/storage/__init__.py @@ -349,6 +349,7 @@ class Storage(object): if hasattr(vm, 'volume_config'): for name, conf in self.vm.volume_config.items(): + conf = conf.copy() if 'source' in conf: template = getattr(vm, 'template', None) if template: diff --git a/qubes/vm/appvm.py b/qubes/vm/appvm.py index 8d22e5db..e8d63e40 100644 --- a/qubes/vm/appvm.py +++ b/qubes/vm/appvm.py @@ -93,7 +93,7 @@ class AppVM(qubes.vm.qubesvm.QubesVM): # in case the template vm has more volumes add them to own # config if name not in self.volume_config: - self.volume_config[name] = copy.deepcopy(config) + self.volume_config[name] = config.copy() if 'vid' in self.volume_config[name]: del self.volume_config[name]['vid'] @@ -123,7 +123,7 @@ class AppVM(qubes.vm.qubesvm.QubesVM): for volume_name, conf in self.default_volume_config.items(): if conf.get('snap_on_start', False) and \ conf.get('source', None) is None: - config = copy.deepcopy(conf) + config = conf.copy() template_volume = newvalue.volumes[volume_name] self.volume_config[volume_name] = \ self.config_volume_from_source( diff --git a/qubes/vm/dispvm.py b/qubes/vm/dispvm.py index 97d67d49..c9d129e2 100644 --- a/qubes/vm/dispvm.py +++ b/qubes/vm/dispvm.py @@ -21,8 +21,6 @@ ''' A disposable vm implementation ''' -import copy - import qubes.vm.qubesvm import qubes.vm.appvm import qubes.config @@ -89,7 +87,7 @@ class DispVM(qubes.vm.qubesvm.QubesVM): # in case the template vm has more volumes add them to own # config if name not in self.volume_config: - self.volume_config[name] = copy.deepcopy(config) + self.volume_config[name] = config.copy() if 'vid' in self.volume_config[name]: del self.volume_config[name]['vid'] diff --git a/qubes/vm/qubesvm.py b/qubes/vm/qubesvm.py index 5699d554..797dd442 100644 --- a/qubes/vm/qubesvm.py +++ b/qubes/vm/qubesvm.py @@ -24,7 +24,6 @@ from __future__ import absolute_import import asyncio -import copy import base64 import datetime import os @@ -1799,8 +1798,7 @@ def _clean_volume_config(config): common_attributes = ['name', 'pool', 'size', 'revisions_to_keep', 'rw', 'snap_on_start', 'save_on_stop', 'source'] - config_copy = copy.deepcopy(config) - return {k: v for k, v in config_copy.items() if k in common_attributes} + return {k: v for k, v in config.items() if k in common_attributes} def _patch_pool_config(config, pool=None, pools=None): From 2df81adab12922410fc9cd203299010e8828deb7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Wed, 12 Jul 2017 12:40:43 +0200 Subject: [PATCH 5/6] tests/vm: simplify AppVM storage test Use minimal TestPool(), instead of Mock(). This allow effectively compare Volume instances (init_volume with the same parameters return equal Volume instance). --- qubes/tests/vm/appvm.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/qubes/tests/vm/appvm.py b/qubes/tests/vm/appvm.py index 4c6bdbf0..2a4f3f37 100644 --- a/qubes/tests/vm/appvm.py +++ b/qubes/tests/vm/appvm.py @@ -20,6 +20,7 @@ from unittest import mock +import qubes.storage import qubes.tests import qubes.tests.vm.qubesvm import qubes.vm.appvm @@ -48,12 +49,18 @@ class TestVM(object): def is_running(self): return self.running +class TestPool(qubes.storage.Pool): + def init_volume(self, vm, volume_config): + vid = '{}/{}'.format(vm.name, volume_config['name']) + assert volume_config.pop('pool', None) == self + return qubes.storage.Volume(vid=vid, pool=self, **volume_config) + + class TC_90_AppVM(qubes.tests.vm.qubesvm.QubesVMTestsMixin, qubes.tests.QubesTestCase): def setUp(self): super().setUp() - self.app.pools['default'] = mock.Mock(**{ - 'init_volume.return_value.pool': 'default'}) + self.app.pools['default'] = TestPool('default') self.app.pools['linux-kernel'] = mock.Mock(**{ 'init_volume.return_value.pool': 'linux-kernel'}) self.template = qubes.vm.templatevm.TemplateVM(self.app, None, From 66b88cc943c7d591b96e498928ff25a1e297135b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Wed, 12 Jul 2017 12:48:34 +0200 Subject: [PATCH 6/6] api: extract function to make pylint happy --- qubes/api/__init__.py | 35 +++++++++++++++++++++-------------- 1 file changed, 21 insertions(+), 14 deletions(-) diff --git a/qubes/api/__init__.py b/qubes/api/__init__.py index c2046ebb..2783e486 100644 --- a/qubes/api/__init__.py +++ b/qubes/api/__init__.py @@ -345,6 +345,26 @@ class QubesDaemonProtocol(asyncio.Protocol): self.transport.write(str(exc).encode('utf-8') + b'\0') +def cleanup_socket(sockpath, force): + '''Remove socket if stale, or force=True + :param sockpath: path to a socket + :param force: should remove even if still used + ''' + if force: + os.unlink(sockpath) + else: + sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) + try: + sock.connect(sockpath) + except ConnectionRefusedError: + # dead socket, remove it anyway + os.unlink(sockpath) + else: + # woops, someone is listening + sock.close() + raise FileExistsError(errno.EEXIST, + 'socket already exists: {!r}'.format(sockpath)) + @asyncio.coroutine def create_servers(*args, force=False, loop=None, **kwargs): '''Create multiple Qubes API servers @@ -375,20 +395,7 @@ def create_servers(*args, force=False, loop=None, **kwargs): type(handler).__name__) if os.path.exists(sockpath): - if force: - os.unlink(sockpath) - else: - sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) - try: - sock.connect(sockpath) - except ConnectionRefusedError: - # dead socket, remove it anyway - os.unlink(sockpath) - else: - # woops, someone is listening - sock.close() - raise FileExistsError(errno.EEXIST, - 'socket already exists: {!r}'.format(sockpath)) + cleanup_socket(sockpath, force) server = yield from loop.create_unix_server( functools.partial(QubesDaemonProtocol, handler, **kwargs),