From fb7bd6823a434c81fe7f9913db05124dfba770b7 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?=
 <marmarek@invisiblethingslab.com>
Date: Thu, 9 Mar 2017 02:42:05 +0100
Subject: [PATCH 01/31] mgmt: implement storage-related methods

QubesOS/qubes-issues#2622
---
 qubes/mgmt.py             | 152 ++++++++++++++++++++++++++++++++++++++
 qubes/storage/__init__.py |   9 +++
 2 files changed, 161 insertions(+)

diff --git a/qubes/mgmt.py b/qubes/mgmt.py
index ae617787..b7631a43 100644
--- a/qubes/mgmt.py
+++ b/qubes/mgmt.py
@@ -24,8 +24,10 @@ Qubes OS Management API
 
 import asyncio
 import reprlib
+import string
 
 import qubes.vm.qubesvm
+import qubes.storage
 
 
 class ProtocolRepr(reprlib.Repr):
@@ -194,3 +196,153 @@ class QubesMgmt(object):
         self.fire_event_for_permission()
 
         delattr(self.dest, self.arg)
+
+    @asyncio.coroutine
+    def vm_volume_list(self, untrusted_payload):
+        assert not self.arg
+        assert not untrusted_payload
+        del untrusted_payload
+
+        volume_names = self.fire_event_for_filter(self.dest.volumes.keys())
+        return ''.join('{}\n'.format(name) for name in volume_names)
+
+    @asyncio.coroutine
+    def vm_volume_info(self, untrusted_payload):
+        assert self.arg in self.dest.volumes.keys()
+        assert not untrusted_payload
+        del untrusted_payload
+
+        self.fire_event_for_permission()
+
+        volume = self.dest.volumes[self.arg]
+        # properties defined in API
+        volume_properties = [
+            'pool', 'vid', 'size', 'usage', 'rw', 'internal', 'source',
+            'save_on_stop', 'snap_on_start']
+        return ''.join('{}={}\n'.format(key, getattr(volume, key)) for key in
+            volume_properties)
+
+    @asyncio.coroutine
+    def vm_volume_listsnapshots(self, untrusted_payload):
+        assert self.arg in self.dest.volumes.keys()
+        assert not untrusted_payload
+        del untrusted_payload
+
+        self.fire_event_for_permission()
+
+        volume = self.dest.volumes[self.arg]
+        return ''.join('{}\n'.format(revision) for revision in volume.revisions)
+
+    @asyncio.coroutine
+    def vm_volume_revert(self, untrusted_payload):
+        assert self.arg in self.dest.volumes.keys()
+        untrusted_revision = untrusted_payload.decode('ascii').strip()
+        del untrusted_payload
+
+        volume = self.dest.volumes[self.arg]
+        snapshots = volume.revisions
+        assert untrusted_revision in snapshots
+        revision = untrusted_revision
+
+        self.fire_event_for_permission(revision=revision)
+
+        self.dest.storage.get_pool(volume).revert(revision)
+
+    @asyncio.coroutine
+    def vm_volume_resize(self, untrusted_payload):
+        assert self.arg in self.dest.volumes.keys()
+        untrusted_size = untrusted_payload.decode('ascii').strip()
+        del untrusted_payload
+        assert untrusted_size.isdigit()  # only digits, forbid '-' too
+        assert len(untrusted_size) <= 20  # limit to about 2^64
+
+        size = int(untrusted_size)
+
+        self.fire_event_for_permission(size=size)
+
+        self.dest.storage.resize(self.arg, size)
+
+    @asyncio.coroutine
+    def pool_list(self, untrusted_payload):
+        assert not self.arg
+        assert self.dest.name == 'dom0'
+        assert not untrusted_payload
+        del untrusted_payload
+
+        pools = self.fire_event_for_filter(self.app.pools)
+
+        return ''.join('{}\n'.format(pool) for pool in pools)
+
+    @asyncio.coroutine
+    def pool_listdrivers(self, untrusted_payload):
+        assert self.dest.name == 'dom0'
+        assert not self.arg
+        assert not untrusted_payload
+        del untrusted_payload
+
+        drivers = self.fire_event_for_filter(qubes.storage.pool_drivers())
+
+        return ''.join('{} {}\n'.format(
+            driver,
+            ' '.join(qubes.storage.driver_parameters(driver)))
+            for driver in drivers)
+
+    @asyncio.coroutine
+    def pool_info(self, untrusted_payload):
+        assert self.dest.name == 'dom0'
+        assert self.arg in self.app.pools.keys()
+        assert not untrusted_payload
+        del untrusted_payload
+
+        pool = self.app.pools[self.arg]
+
+        self.fire_event_for_permission(pool=pool)
+
+        return ''.join('{}={}\n'.format(prop, val)
+            for prop, val in sorted(pool.config.items()))
+
+    @asyncio.coroutine
+    def pool_add(self, untrusted_payload):
+        assert self.dest.name == 'dom0'
+        drivers = qubes.storage.pool_drivers()
+        assert self.arg in drivers
+        untrusted_pool_config = untrusted_payload.decode('ascii').splitlines()
+        del untrusted_payload
+        assert all(('=' in line) for line in untrusted_pool_config)
+        # pairs of (option, value)
+        untrusted_pool_config = [line.split('=', 1)
+            for line in untrusted_pool_config]
+        # reject duplicated options
+        assert len(set(x[0] for x in untrusted_pool_config)) == \
+               len([x[0] for x in untrusted_pool_config])
+        # and convert to dict
+        untrusted_pool_config = dict(untrusted_pool_config)
+
+        assert 'name' in untrusted_pool_config
+        untrusted_pool_name = untrusted_pool_config.pop('name')
+        allowed_chars = string.ascii_letters + string.digits + '-_.'
+        assert all(c in allowed_chars for c in untrusted_pool_name)
+        pool_name = untrusted_pool_name
+        assert pool_name not in self.app.pools
+
+        driver_parameters = qubes.storage.driver_parameters(self.arg)
+        assert all(key in driver_parameters for key in untrusted_pool_config)
+
+        # option names validated, validation of option values is delegated to
+        #  extension (through events mechanism)
+        self.fire_event_for_permission(name=pool_name,
+            untrusted_pool_config=untrusted_pool_config)
+        pool_config = untrusted_pool_config
+
+        self.app.add_pool(name=pool_name, driver=self.arg, **pool_config)
+
+    @asyncio.coroutine
+    def pool_remove(self, untrusted_payload):
+        assert self.dest.name == 'dom0'
+        assert self.arg in self.app.pools.keys()
+        assert not untrusted_payload
+        del untrusted_payload
+
+        self.fire_event_for_permission()
+
+        self.app.remove_pool(self.arg)
\ No newline at end of file
diff --git a/qubes/storage/__init__.py b/qubes/storage/__init__.py
index da57401b..1f8aa44a 100644
--- a/qubes/storage/__init__.py
+++ b/qubes/storage/__init__.py
@@ -635,6 +635,15 @@ def pool_drivers():
             for ep in pkg_resources.iter_entry_points(STORAGE_ENTRY_POINT)]
 
 
+def driver_parameters(name):
+    ''' Get __init__ parameters from a driver with out `self` & `name`. '''
+    init_function = qubes.utils.get_entry_point_one(
+        qubes.storage.STORAGE_ENTRY_POINT, name).__init__
+    params = init_function.func_code.co_varnames
+    ignored_params = ['self', 'name']
+    return [p for p in params if p not in ignored_params]
+
+
 def isodate(seconds=time.time()):
     ''' Helper method which returns an iso date '''
     return datetime.utcfromtimestamp(seconds).isoformat("T")

From 10a07c8726d1dc465581e15cab1438c85704395c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?=
 <marmarek@invisiblethingslab.com>
Date: Fri, 10 Mar 2017 23:53:21 +0100
Subject: [PATCH 02/31] mgmt: allow vm.List call to a particular VM

This allow getting info about a single VM.

QubesOS/qubes-issues#853
---
 qubes/mgmt.py | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/qubes/mgmt.py b/qubes/mgmt.py
index b7631a43..a6f345a6 100644
--- a/qubes/mgmt.py
+++ b/qubes/mgmt.py
@@ -132,12 +132,14 @@ class QubesMgmt(object):
 
     @asyncio.coroutine
     def vm_list(self, untrusted_payload):
-        assert self.dest.name == 'dom0'
         assert not self.arg
         assert not untrusted_payload
         del untrusted_payload
 
-        domains = self.fire_event_for_filter(self.app.domains)
+        if self.dest.name == 'dom0':
+            domains = self.fire_event_for_filter(self.app.domains)
+        else:
+            domains = self.fire_event_for_filter([self.dest])
 
         return ''.join('{} class={} state={}\n'.format(
                 vm.name,

From c41585e2f59a20076689c2851305d3293bdd1db6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?=
 <marmarek@invisiblethingslab.com>
Date: Fri, 10 Mar 2017 23:54:57 +0100
Subject: [PATCH 03/31] Initialize dom0 label

It is required property. Additionally, define icon_path to None,
otherwise it tries to access dom0.dir_path, which isn't set.
---
 qubes/app.py        | 3 ++-
 qubes/vm/adminvm.py | 3 +++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/qubes/app.py b/qubes/app.py
index ae9380f5..a9e23104 100644
--- a/qubes/app.py
+++ b/qubes/app.py
@@ -872,7 +872,8 @@ class Qubes(qubes.PropertyHolder):
             self.pools[name] = self._get_pool(**config)
 
         self.domains.add(
-            qubes.vm.adminvm.AdminVM(self, None, qid=0, name='dom0'))
+            qubes.vm.adminvm.AdminVM(self, None, qid=0, name='dom0',
+                label='black'))
 
     @classmethod
     def create_empty_store(cls, *args, **kwargs):
diff --git a/qubes/vm/adminvm.py b/qubes/vm/adminvm.py
index 13671ff4..a571a3de 100644
--- a/qubes/vm/adminvm.py
+++ b/qubes/vm/adminvm.py
@@ -143,6 +143,9 @@ class AdminVM(qubes.vm.qubesvm.QubesVM):
         '''
         raise qubes.exc.QubesVMError(self, 'Cannot suspend Dom0 fake domain!')
 
+    @property
+    def icon_path(self):
+        return None
 
 #   def __init__(self, **kwargs):
 #       super(QubesAdminVm, self).__init__(qid=0, name="dom0", netid=0,

From f7eabf8eb05cfcd4a4d7c5cd2c02e129482963cd Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?=
 <marmarek@invisiblethingslab.com>
Date: Fri, 10 Mar 2017 23:56:00 +0100
Subject: [PATCH 04/31] tools/qubesd: do not close connection before sending
 response

eof_received callback should return True, if connection should not be
automatically closed just after returning from it.
---
 qubes/tools/qubesd.py | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/qubes/tools/qubesd.py b/qubes/tools/qubesd.py
index 2b1bcb44..b72cd9ba 100644
--- a/qubes/tools/qubesd.py
+++ b/qubes/tools/qubesd.py
@@ -63,6 +63,8 @@ class QubesDaemonProtocol(asyncio.Protocol):
         asyncio.ensure_future(self.respond(
             src, method, dest, arg, untrusted_payload=untrusted_payload))
 
+        return True
+
     @asyncio.coroutine
     def respond(self, src, method, dest, arg, *, untrusted_payload):
         try:

From 772293d0b53d78318338c1966e9116c31630aa8e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?=
 <marmarek@invisiblethingslab.com>
Date: Sat, 11 Mar 2017 00:06:02 +0100
Subject: [PATCH 05/31] vm/qubesvm: define 'updateable' as qubes.property

This will allow property being accessed through management API
---
 qubes/vm/qubesvm.py | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/qubes/vm/qubesvm.py b/qubes/vm/qubesvm.py
index 6af8add1..bab2d012 100644
--- a/qubes/vm/qubesvm.py
+++ b/qubes/vm/qubesvm.py
@@ -488,6 +488,13 @@ class QubesVM(qubes.vm.mix.net.NetVMMixin, qubes.vm.BaseVM):
         default=(lambda self: self.app.default_dispvm),
         doc='Default VM to be used as Disposable VM for service calls.')
 
+
+    updateable = qubes.property('updateable',
+        default=(lambda self: not hasattr(self, 'template')),
+        type=bool,
+        setter=qubes.property.forbidden,
+        doc='True if this machine may be updated on its own.')
+
     #
     # static, class-wide properties
     #
@@ -575,12 +582,6 @@ class QubesVM(qubes.vm.mix.net.NetVMMixin, qubes.vm.BaseVM):
                 self._qdb_connection = qubesdb.QubesDB(self.name)
         return self._qdb_connection
 
-    # XXX shouldn't this go elsewhere?
-    @property
-    def updateable(self):
-        '''True if this machine may be updated on its own.'''
-        return not hasattr(self, 'template')
-
     @property
     def dir_path(self):
         '''Root directory for files related to this domain'''

From 3b36e92b6d36d5dc470a68ebb9c727ec36da4b9b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?=
 <marmarek@invisiblethingslab.com>
Date: Sat, 11 Mar 2017 00:06:55 +0100
Subject: [PATCH 06/31] vm/qubesvm: fix few more keyword arguments for events

---
 qubes/vm/qubesvm.py | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/qubes/vm/qubesvm.py b/qubes/vm/qubesvm.py
index bab2d012..bae91936 100644
--- a/qubes/vm/qubesvm.py
+++ b/qubes/vm/qubesvm.py
@@ -703,7 +703,7 @@ class QubesVM(qubes.vm.mix.net.NetVMMixin, qubes.vm.BaseVM):
         self.app.pools[vm_pool.name] = vm_pool
 
     @qubes.events.handler('property-set:label')
-    def on_property_set_label(self, event, name, new_label, old_label=None):
+    def on_property_set_label(self, event, name, newvalue, oldvalue=None):
         # pylint: disable=unused-argument
         if self.icon_path:
             try:
@@ -711,10 +711,10 @@ class QubesVM(qubes.vm.mix.net.NetVMMixin, qubes.vm.BaseVM):
             except OSError:
                 pass
             if hasattr(os, "symlink"):
-                os.symlink(new_label.icon_path, self.icon_path)
+                os.symlink(newvalue.icon_path, self.icon_path)
                 subprocess.call(['sudo', 'xdg-icon-resource', 'forceupdate'])
             else:
-                shutil.copy(new_label.icon_path, self.icon_path)
+                shutil.copy(newvalue.icon_path, self.icon_path)
 
     @qubes.events.handler('property-pre-set:name')
     def on_property_pre_set_name(self, event, name, newvalue, oldvalue=None):
@@ -737,11 +737,11 @@ class QubesVM(qubes.vm.mix.net.NetVMMixin, qubes.vm.BaseVM):
                                    'qubes-vm@{}.service'.format(oldvalue)])
 
     @qubes.events.handler('property-set:name')
-    def on_property_set_name(self, event, name, new_name, old_name=None):
+    def on_property_set_name(self, event, name, newvalue, oldvalue=None):
         # pylint: disable=unused-argument
         self.init_log()
 
-        self.storage.rename(old_name, new_name)
+        self.storage.rename(oldvalue, newvalue)
 
         if self._libvirt_domain is not None:
             self.libvirt_domain.undefine()
@@ -756,11 +756,11 @@ class QubesVM(qubes.vm.mix.net.NetVMMixin, qubes.vm.BaseVM):
             self.autostart = self.autostart
 
     @qubes.events.handler('property-pre-set:autostart')
-    def on_property_pre_set_autostart(self, event, prop, value,
+    def on_property_pre_set_autostart(self, event, prop, newvalue,
             oldvalue=None):
         # pylint: disable=unused-argument
         # workaround https://bugzilla.redhat.com/show_bug.cgi?id=1181922
-        if value:
+        if newvalue:
             retcode = subprocess.call(
                 ["sudo", "ln", "-sf",
                  "/usr/lib/systemd/system/qubes-vm@.service",

From dbf2066dfdfb52780c9b180c082a9d65e34a48f8 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?=
 <marmarek@invisiblethingslab.com>
Date: Sat, 11 Mar 2017 19:04:50 +0100
Subject: [PATCH 07/31] mgmt: encode property type in property.Get

This also require having property.type public.

QubesOS/qubes-issues#2622
---
 qubes/__init__.py |  6 +++---
 qubes/mgmt.py     | 20 +++++++++++++++++---
 2 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/qubes/__init__.py b/qubes/__init__.py
index 697605ce..cd0c34bf 100644
--- a/qubes/__init__.py
+++ b/qubes/__init__.py
@@ -193,7 +193,7 @@ class property(object): # pylint: disable=redefined-builtin,invalid-name
         self._setter = setter
         self._saver = saver if saver is not None else (
             lambda self, prop, value: str(value))
-        self._type = type
+        self.type = type
         self._default = default
         self._write_once = write_once
         self.order = order
@@ -245,8 +245,8 @@ class property(object): # pylint: disable=redefined-builtin,invalid-name
 
         if self._setter is not None:
             value = self._setter(instance, self, value)
-        if self._type not in (None, type(value)):
-            value = self._type(value)
+        if self.type not in (None, type(value)):
+            value = self.type(value)
 
         if has_oldvalue:
             instance.fire_event_pre('property-pre-set:' + self.__name__,
diff --git a/qubes/mgmt.py b/qubes/mgmt.py
index a6f345a6..76cf04b4 100644
--- a/qubes/mgmt.py
+++ b/qubes/mgmt.py
@@ -165,14 +165,28 @@ class QubesMgmt(object):
 
         self.fire_event_for_permission()
 
+        property_def = self.dest.property_get_def(self.arg)
+        # explicit list to be sure that it matches protocol spec
+        if isinstance(property_def, qubes.vm.VMProperty):
+            property_type = 'vm'
+        elif property_def.type is int:
+            property_type = 'int'
+        elif property_def.type is bool:
+            property_type = 'bool'
+        elif self.arg == 'label':
+            property_type = 'label'
+        else:
+            property_type = 'str'
+
         try:
             value = getattr(self.dest, self.arg)
         except AttributeError:
-            return 'default=True '
+            return 'default=True type={} '.format(property_type)
         else:
-            return 'default={} {}'.format(
+            return 'default={} type={} {}'.format(
                 str(self.dest.property_is_default(self.arg)),
-                self.repr(value))
+                property_type,
+                str(value) if value is not None else '')
 
     @asyncio.coroutine
     def vm_property_help(self, untrusted_payload):

From a036e2a8a065e253cf5f5f2ee067a08956ee2dfb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?=
 <marmarek@invisiblethingslab.com>
Date: Sat, 11 Mar 2017 19:20:22 +0100
Subject: [PATCH 08/31] vm/qubesvm: improve name property setter

Split it into two functions: validate_name - context-less verification,
and actual _setter_name which perform additional verification in
context of actual VM.
Switch to qubes.exc.* exceptions where appropriate.
---
 qubes/vm/qubesvm.py | 33 ++++++++++++++++++++++++---------
 1 file changed, 24 insertions(+), 9 deletions(-)

diff --git a/qubes/vm/qubesvm.py b/qubes/vm/qubesvm.py
index bae91936..8a5ae3f9 100644
--- a/qubes/vm/qubesvm.py
+++ b/qubes/vm/qubesvm.py
@@ -75,20 +75,35 @@ def _setter_qid(self, prop, value):
     return value
 
 
-def _setter_name(self, prop, value):
-    ''' Helper for setting the domain name '''
+def validate_name(holder, prop, value):
+    ''' Check if value is syntactically correct VM name '''
     if not isinstance(value, str):
-        raise TypeError('{} value must be string, {!r} found'.format(
-            prop.__name__, type(value).__name__))
+        raise TypeError('VM name must be string, {!r} found'.format(
+            type(value).__name__))
     if len(value) > 31:
-        raise ValueError('{} value must be shorter than 32 characters'.format(
-            prop.__name__))
+        if holder is not None and prop is not None:
+            raise qubes.exc.QubesPropertyValueError(holder, prop, value,
+                '{} value must be shorter than 32 characters'.format(
+                    prop.__name__))
+        else:
+            raise qubes.exc.QubesValueError(
+                'VM name must be shorter than 32 characters')
 
     # this regexp does not contain '+'; if it had it, we should specifically
     # disallow 'lost+found' #1440
     if re.match(r"^[a-zA-Z][a-zA-Z0-9_-]*$", value) is None:
-        raise ValueError('{} value contains illegal characters'.format(
-            prop.__name__))
+        if holder is not None and prop is not None:
+            raise qubes.exc.QubesPropertyValueError(holder, prop, value,
+                '{} value contains illegal characters'.format(prop.__name__))
+        else:
+            raise qubes.exc.QubesValueError(
+                'VM name contains illegal characters')
+
+
+def _setter_name(self, prop, value):
+    ''' Helper for setting the domain name '''
+    validate_name(self, prop, value)
+
     if self.is_running():
         raise qubes.exc.QubesVMNotHaltedError(
             self, 'Cannot change name of running VM')
@@ -101,7 +116,7 @@ def _setter_name(self, prop, value):
         pass
 
     if value in self.app.domains:
-        raise qubes.exc.QubesValueError(
+        raise qubes.exc.QubesPropertyValueError(self, prop, value,
             'VM named {} alread exists'.format(value))
 
     return value

From 123feced36596f858ce5936a5050a386692b756b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?=
 <marmarek@invisiblethingslab.com>
Date: Sat, 11 Mar 2017 19:21:59 +0100
Subject: [PATCH 09/31] vm/qubesvm: forbid '/' in kernel property

It would give VM access to some files outside of
/var/lib/qubes/vm-kernels.
---
 qubes/vm/qubesvm.py | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/qubes/vm/qubesvm.py b/qubes/vm/qubesvm.py
index 8a5ae3f9..f8a0aba3 100644
--- a/qubes/vm/qubesvm.py
+++ b/qubes/vm/qubesvm.py
@@ -128,6 +128,9 @@ def _setter_kernel(self, prop, value):
     if value is None:
         return value
     value = str(value)
+    if '/' in value:
+        raise qubes.exc.QubesPropertyValueError(self, prop, value,
+            'Kernel name cannot contain \'/\'')
     dirname = os.path.join(
         qubes.config.system_path['qubes_base_dir'],
         qubes.config.system_path['qubes_kernels_base_dir'],

From 2d2672ec583c749047a30d3e024304bcfdae1d4a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?=
 <marmarek@invisiblethingslab.com>
Date: Sun, 12 Mar 2017 01:50:09 +0100
Subject: [PATCH 10/31] vm/qubesvm: convert firewall_conf into dumb, read-only
 property

Don't allow anything else than firewall.xml.
---
 qubes/tests/vm/qubesvm.py | 14 --------------
 qubes/vm/mix/net.py       |  6 ++++--
 2 files changed, 4 insertions(+), 16 deletions(-)

diff --git a/qubes/tests/vm/qubesvm.py b/qubes/tests/vm/qubesvm.py
index 2fe79ff7..a8d9f626 100644
--- a/qubes/tests/vm/qubesvm.py
+++ b/qubes/tests/vm/qubesvm.py
@@ -340,20 +340,6 @@ class TC_90_QubesVM(QubesVMTestsMixin,qubes.tests.QubesTestCase):
         vm = self.get_vm()
         self._test_generic_bool_property(vm, 'include_in_backups', True)
 
-    def test_240_firewall_conf(self):
-        vm = self.get_vm()
-        self.assertPropertyDefaultValue(vm, 'firewall_conf', 'firewall.xml')
-        self.assertPropertyValue(vm, 'firewall_conf', 'other.xml',
-            'other.xml', 'other.xml')
-        del vm.firewall_conf
-        self.assertPropertyDefaultValue(vm, 'firewall_conf',
-            'firewall.xml')
-
-    @unittest.expectedFailure
-    def test_241_firewall_conf_invalid(self):
-        vm = self.get_vm()
-        self.assertPropertyInvalidValue(vm, 'firewall_conf', None)
-
     @qubes.tests.skipUnlessDom0
     def test_250_kernel(self):
         kernels = os.listdir(os.path.join(
diff --git a/qubes/vm/mix/net.py b/qubes/vm/mix/net.py
index dc7c9e95..dde78302 100644
--- a/qubes/vm/mix/net.py
+++ b/qubes/vm/mix/net.py
@@ -91,8 +91,10 @@ class NetVMMixin(qubes.events.Emitter):
         doc='''If this domain can act as network provider (formerly known as
             NetVM or ProxyVM)''')
 
-    firewall_conf = qubes.property('firewall_conf', type=str,
-        default='firewall.xml')
+
+    @property
+    def firewall_conf(self):
+        return 'firewall.xml'
 
     #
     # used in networked appvms or proxyvms (netvm is not None)

From 0f8fab088e66802b53bdfa61448086e83c3901cb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?=
 <marmarek@invisiblethingslab.com>
Date: Sun, 12 Mar 2017 01:52:24 +0100
Subject: [PATCH 11/31] vm/qubesvm: remove pool_name property

It isn't used anywhere - in core3 each storage volume have pool assigned
- which may be different for each volume.
---
 qubes/vm/qubesvm.py | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/qubes/vm/qubesvm.py b/qubes/vm/qubesvm.py
index f8a0aba3..a0a0d424 100644
--- a/qubes/vm/qubesvm.py
+++ b/qubes/vm/qubesvm.py
@@ -435,10 +435,6 @@ class QubesVM(qubes.vm.mix.net.NetVMMixin, qubes.vm.BaseVM):
         ls_width=2,
         doc='FIXME')
 
-    pool_name = qubes.property('pool_name',
-        default='default',
-        doc='storage pool for this qube devices')
-
     # CORE2: swallowed uses_default_kernel
     kernel = qubes.property('kernel', type=str,
         setter=_setter_kernel,

From da51e6f032f06a13e2c91ed87d0081478aff0ba0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?=
 <marmarek@invisiblethingslab.com>
Date: Sun, 12 Mar 2017 01:53:46 +0100
Subject: [PATCH 12/31] vm/qubesvm: add validator for default_user property

Don't allow characters potentially interfering with qrexec. To be on the
safe side, allow only alphanumeric characters + very few selected
punctuations.
---
 qubes/vm/qubesvm.py | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/qubes/vm/qubesvm.py b/qubes/vm/qubesvm.py
index a0a0d424..888d3fae 100644
--- a/qubes/vm/qubesvm.py
+++ b/qubes/vm/qubesvm.py
@@ -30,6 +30,7 @@ import os
 import os.path
 import re
 import shutil
+import string
 import subprocess
 import sys
 import time
@@ -165,6 +166,16 @@ def _setter_positive_int(self, prop, value):
     return value
 
 
+def _setter_default_user(self, prop, value):
+    ''' Helper for setting default user '''
+    value = str(value)
+    # specifically forbid: ':', ' ', ''', '"'
+    allowed_chars = string.ascii_letters + string.digits + '_-+,.'
+    if not all(c in allowed_chars for c in value):
+        raise qubes.exc.QubesPropertyValueError(self, prop, value,
+            'Username can contain only those characters: ' + allowed_chars)
+    return value
+
 class QubesVM(qubes.vm.mix.net.NetVMMixin, qubes.vm.BaseVM):
     '''Base functionality of Qubes VM shared between all VMs.
 
@@ -461,6 +472,7 @@ class QubesVM(qubes.vm.mix.net.NetVMMixin, qubes.vm.BaseVM):
     default_user = qubes.property('default_user', type=str,
         default=(lambda self: self.template.default_user
             if hasattr(self, 'template') else 'user'),
+        setter=_setter_default_user,
         ls_width=12,
         doc='FIXME')
 

From 868dbeac3eae72150e1856c403fe6e2ababbacc6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?=
 <marmarek@invisiblethingslab.com>
Date: Sun, 12 Mar 2017 01:25:22 +0100
Subject: [PATCH 13/31] mgmt: implement mgmt.vm.property.Set

Sanitization of input value is tricky here, and also very important at
the same time. If property define value type (and it's something more
specific than 'str'), use that. Otherwise allow only printable ASCII
characters, and let appropriate event and setter handle value.
At this point I've reviewed all QubesVM properties in this category and
added appropriate setters where needed.

QubesOS/qubes-issues#2622
---
 qubes/mgmt.py | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/qubes/mgmt.py b/qubes/mgmt.py
index 76cf04b4..513a5940 100644
--- a/qubes/mgmt.py
+++ b/qubes/mgmt.py
@@ -188,6 +188,53 @@ class QubesMgmt(object):
                 property_type,
                 str(value) if value is not None else '')
 
+    @asyncio.coroutine
+    def vm_property_set(self, untrusted_payload):
+        assert self.arg in self.dest.property_list()
+
+        property_def = self.dest.property_get_def(self.arg)
+        # do not treat type='str' as sufficient validation
+        if isinstance(property_def, qubes.vm.VMProperty):
+            try:
+                untrusted_vmname = untrusted_payload.decode('ascii')
+            except UnicodeDecodeError:
+                raise qubes.exc.QubesValueError
+            qubes.vm.qubesvm.validate_name(self.dest, property_def,
+                untrusted_vmname)
+            try:
+                newvalue = self.app.domains[untrusted_vmname]
+            except KeyError:
+                raise qubes.exc.QubesPropertyValueError(
+                    self.dest, property_def, untrusted_vmname)
+        elif property_def.type is not None and property_def.type is not str:
+            if property_def.type is bool:
+                try:
+                    untrusted_value = untrusted_payload.decode('ascii')
+                except UnicodeDecodeError:
+                    raise qubes.exc.QubesValueError
+                newvalue = qubes.property.bool(None, None, untrusted_value)
+            else:
+                try:
+                    newvalue = property_def.type(untrusted_payload)
+                except ValueError:
+                    raise qubes.exc.QubesValueError
+        else:
+            # other types (including explicitly defined 'str') coerce to str
+            # with limited characters allowed
+            try:
+                untrusted_value = untrusted_payload.decode('ascii')
+            except UnicodeDecodeError:
+                raise qubes.exc.QubesValueError
+            allowed_set = string.printable
+            if not all(x in allowed_set for x in untrusted_value):
+                raise qubes.exc.QubesValueError(
+                    'Invalid characters in property value')
+            newvalue = untrusted_value
+
+        self.fire_event_for_permission(newvalue=newvalue)
+
+        setattr(self.dest, self.arg, newvalue)
+
     @asyncio.coroutine
     def vm_property_help(self, untrusted_payload):
         assert self.arg in self.dest.property_list()

From 33f3fedca1a94da28fc7a3fb189fd4186b0c08d2 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?=
 <marmarek@invisiblethingslab.com>
Date: Mon, 13 Mar 2017 15:08:51 +0100
Subject: [PATCH 14/31] mgmt: save qubes.xml after config-modifying calls

In theory any call could modify config (through events), but lets keep
writes to qubes.xml low. In any case, qubes.xml will be eventually
written (either at next config-modifying call, or daemon exit).
---
 qubes/mgmt.py | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/qubes/mgmt.py b/qubes/mgmt.py
index 513a5940..8629be81 100644
--- a/qubes/mgmt.py
+++ b/qubes/mgmt.py
@@ -234,6 +234,7 @@ class QubesMgmt(object):
         self.fire_event_for_permission(newvalue=newvalue)
 
         setattr(self.dest, self.arg, newvalue)
+        self.app.save()
 
     @asyncio.coroutine
     def vm_property_help(self, untrusted_payload):
@@ -259,6 +260,7 @@ class QubesMgmt(object):
         self.fire_event_for_permission()
 
         delattr(self.dest, self.arg)
+        self.app.save()
 
     @asyncio.coroutine
     def vm_volume_list(self, untrusted_payload):
@@ -310,6 +312,7 @@ class QubesMgmt(object):
         self.fire_event_for_permission(revision=revision)
 
         self.dest.storage.get_pool(volume).revert(revision)
+        self.app.save()
 
     @asyncio.coroutine
     def vm_volume_resize(self, untrusted_payload):
@@ -324,6 +327,7 @@ class QubesMgmt(object):
         self.fire_event_for_permission(size=size)
 
         self.dest.storage.resize(self.arg, size)
+        self.app.save()
 
     @asyncio.coroutine
     def pool_list(self, untrusted_payload):
@@ -398,6 +402,7 @@ class QubesMgmt(object):
         pool_config = untrusted_pool_config
 
         self.app.add_pool(name=pool_name, driver=self.arg, **pool_config)
+        self.app.save()
 
     @asyncio.coroutine
     def pool_remove(self, untrusted_payload):
@@ -408,4 +413,5 @@ class QubesMgmt(object):
 
         self.fire_event_for_permission()
 
-        self.app.remove_pool(self.arg)
\ No newline at end of file
+        self.app.remove_pool(self.arg)
+        self.app.save()

From 010d40dc1e729caf6910ae03d434c58fb969f66e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?=
 <marmarek@invisiblethingslab.com>
Date: Mon, 13 Mar 2017 15:13:20 +0100
Subject: [PATCH 15/31] mgmt: add label-related calls

QubesOS/qubes-issues#2622
---
 qubes/__init__.py |  4 +++
 qubes/app.py      |  1 +
 qubes/config.py   |  3 ++
 qubes/mgmt.py     | 84 +++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 92 insertions(+)

diff --git a/qubes/__init__.py b/qubes/__init__.py
index cd0c34bf..0f623305 100644
--- a/qubes/__init__.py
+++ b/qubes/__init__.py
@@ -104,6 +104,10 @@ class Label(object):
             self.color,
             self.name)
 
+    def __eq__(self, other):
+        if isinstance(other, Label):
+            return self.name == other.name
+        return NotImplemented
 
     @builtins.property
     def icon_path(self):
diff --git a/qubes/app.py b/qubes/app.py
index a9e23104..35e109fe 100644
--- a/qubes/app.py
+++ b/qubes/app.py
@@ -859,6 +859,7 @@ class Qubes(qubes.PropertyHolder):
             7: qubes.Label(7, '0x75507b', 'purple'),
             8: qubes.Label(8, '0x000000', 'black'),
         }
+        assert max(self.labels.keys()) == qubes.config.max_default_label
 
         # check if the default LVM Thin pool qubes_dom0/pool00 exists
         if os.path.exists('/dev/mapper/qubes_dom0-pool00-tpool'):
diff --git a/qubes/config.py b/qubes/config.py
index 2596e239..4d65e873 100644
--- a/qubes/config.py
+++ b/qubes/config.py
@@ -109,3 +109,6 @@ defaults = {
 max_qid = 254
 max_netid = 254
 max_dispid = 10000
+#: built-in standard labels, if creating new one, allocate them above this
+# number, at least until label index is removed from API
+max_default_label = 8
diff --git a/qubes/mgmt.py b/qubes/mgmt.py
index 8629be81..8423f794 100644
--- a/qubes/mgmt.py
+++ b/qubes/mgmt.py
@@ -415,3 +415,87 @@ class QubesMgmt(object):
 
         self.app.remove_pool(self.arg)
         self.app.save()
+
+    @asyncio.coroutine
+    def label_list(self, untrusted_payload):
+        assert self.dest.name == 'dom0'
+        assert not self.arg
+        assert not untrusted_payload
+        del untrusted_payload
+
+        labels = self.fire_event_for_filter(self.app.labels.values())
+
+        return ''.join('{}\n'.format(label.name) for label in labels)
+
+    @asyncio.coroutine
+    def label_get(self, untrusted_payload):
+        assert self.dest.name == 'dom0'
+        assert not untrusted_payload
+        del untrusted_payload
+
+        try:
+            label = self.app.get_label(self.arg)
+        except KeyError:
+            raise qubes.exc.QubesValueError
+
+        self.fire_event_for_permission(label=label)
+
+        return label.color
+
+    @asyncio.coroutine
+    def label_create(self, untrusted_payload):
+        assert self.dest.name == 'dom0'
+
+        # don't confuse label name with label index
+        assert not self.arg.isdigit()
+        allowed_chars = string.ascii_letters + string.digits + '-_.'
+        assert all(c in allowed_chars for c in self.arg)
+        try:
+            self.app.get_label(self.arg)
+        except KeyError:
+            # ok, no such label yet
+            pass
+        else:
+            raise qubes.exc.QubesValueError('label already exists')
+
+        untrusted_payload = untrusted_payload.decode('ascii').strip()
+        assert len(untrusted_payload) == 8
+        assert untrusted_payload.startswith('0x')
+        # besides prefix, only hex digits are allowed
+        assert all(x in string.hexdigits for x in untrusted_payload[2:])
+
+        # TODO: try to avoid creating label too similar to existing one?
+        color = untrusted_payload
+
+        self.fire_event_for_permission(color=color)
+
+        # allocate new index, but make sure it's outside of default labels set
+        new_index = max(
+            qubes.config.max_default_label, *self.app.labels.keys()) + 1
+
+        label = qubes.Label(new_index, color, self.arg)
+        self.app.labels[new_index] = label
+        self.app.save()
+
+    @asyncio.coroutine
+    def label_remove(self, untrusted_payload):
+        assert self.dest.name == 'dom0'
+        assert not untrusted_payload
+        del untrusted_payload
+
+        try:
+            label = self.app.get_label(self.arg)
+        except KeyError:
+            raise qubes.exc.QubesValueError
+        # don't allow removing default labels
+        assert label.index > qubes.config.max_default_label
+
+        # FIXME: this should be in app.add_label()
+        for vm in self.app.domains:
+            if vm.label == label:
+                raise qubes.exc.QubesException('label still in use')
+
+        self.fire_event_for_permission(label=label)
+
+        del self.app.labels[label.index]
+        self.app.save()

From 2c4303efc4f48a98af5d957e72b89d954565b8b8 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?=
 <marmarek@invisiblethingslab.com>
Date: Wed, 15 Mar 2017 18:23:09 +0100
Subject: [PATCH 16/31] Prefer qubes.exc.QubesValueError over ValueError

This provide clearer information for UI.
---
 qubes/__init__.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qubes/__init__.py b/qubes/__init__.py
index 0f623305..f110a4d9 100644
--- a/qubes/__init__.py
+++ b/qubes/__init__.py
@@ -372,7 +372,7 @@ class property(object): # pylint: disable=redefined-builtin,invalid-name
                 return False
             if lcvalue in ('1', 'yes', 'true', 'on'):
                 return True
-            raise ValueError(
+            raise qubes.exc.QubesValueError(
                 'Invalid literal for boolean property: {!r}'.format(value))
 
         return bool(value)

From 32f6bc2cd99ba3265ae969a65bce6298d325e4cb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?=
 <marmarek@invisiblethingslab.com>
Date: Wed, 15 Mar 2017 18:35:15 +0100
Subject: [PATCH 17/31] qubes/app: fix notifying about default_netvm change

Notify every VM that is affected, not only those providing network
itself.
---
 qubes/app.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qubes/app.py b/qubes/app.py
index 35e109fe..8eeeddba 100644
--- a/qubes/app.py
+++ b/qubes/app.py
@@ -1087,7 +1087,7 @@ class Qubes(qubes.PropertyHolder):
             oldvalue=None):
         # pylint: disable=unused-argument
         for vm in self.domains:
-            if vm.provides_network and vm.property_is_default('netvm'):
+            if vm.property_is_default('netvm'):
                 # fire property-del:netvm as it is responsible for resetting
                 # netvm to it's default value
                 vm.fire_event('property-del:netvm',

From 35d11678935b49ede7e2a5e8268583bcc0723b17 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?=
 <marmarek@invisiblethingslab.com>
Date: Wed, 15 Mar 2017 18:36:34 +0100
Subject: [PATCH 18/31] qubes/vm/net: fix name of argument for property-del
 event handler

Since enforcing keyword arguments for event handlers, it's important now.
---
 qubes/vm/mix/net.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qubes/vm/mix/net.py b/qubes/vm/mix/net.py
index dde78302..8597e30f 100644
--- a/qubes/vm/mix/net.py
+++ b/qubes/vm/mix/net.py
@@ -332,7 +332,7 @@ class NetVMMixin(qubes.events.Emitter):
 
 
     @qubes.events.handler('property-del:netvm')
-    def on_property_del_netvm(self, event, prop, oldvalue=None):
+    def on_property_del_netvm(self, event, name, oldvalue=None):
         ''' Sets the the NetVM to default NetVM '''
         # pylint: disable=unused-argument
         # we are changing to default netvm

From 944bb2636961fbf7e9b5c7f63e344f975922feb3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?=
 <marmarek@invisiblethingslab.com>
Date: Wed, 15 Mar 2017 18:38:39 +0100
Subject: [PATCH 19/31] tests/mgmt: VM property related functions

---
 qubes/tests/__init__.py  |   1 +
 qubes/tests/mgmt.py      | 296 +++++++++++++++++++++++++++++++++++++++
 rpm_spec/core-dom0.spec  |   1 +
 test-packages/libvirt.py |   8 ++
 4 files changed, 306 insertions(+)
 create mode 100644 qubes/tests/mgmt.py

diff --git a/qubes/tests/__init__.py b/qubes/tests/__init__.py
index 2d63df7f..ba3a00dd 100644
--- a/qubes/tests/__init__.py
+++ b/qubes/tests/__init__.py
@@ -894,6 +894,7 @@ def load_tests(loader, tests, pattern): # pylint: disable=unused-argument
             'qubes.tests.vm.adminvm',
             'qubes.tests.app',
             'qubes.tests.tarwriter',
+            'qubes.tests.mgmt',
             'qubes.tests.tools.qvm_device',
             'qubes.tests.tools.qvm_firewall',
             'qubes.tests.tools.qvm_ls',
diff --git a/qubes/tests/mgmt.py b/qubes/tests/mgmt.py
new file mode 100644
index 00000000..4168e54c
--- /dev/null
+++ b/qubes/tests/mgmt.py
@@ -0,0 +1,296 @@
+# -*- encoding: utf8 -*-
+#
+# The Qubes OS Project, http://www.qubes-os.org
+#
+# Copyright (C) 2017 Marek Marczykowski-Górecki
+#                               <marmarek@invisiblethingslab.com>
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License along
+# with this program; if not, see <http://www.gnu.org/licenses/>.
+
+''' Tests for management calls endpoints '''
+
+import asyncio
+import libvirt
+import unittest.mock
+
+import qubes
+import qubes.tests
+import qubes.mgmt
+
+
+class MgmtTestCase(qubes.tests.QubesTestCase):
+    def setUp(self):
+        super().setUp()
+        app = qubes.Qubes('/tmp/qubes-test.xml', load=False)
+        app.vmm = unittest.mock.Mock(spec=qubes.app.VMMConnection)
+        app.load_initial_values()
+        app.default_kernel = '1.0'
+        app.default_netvm = None
+        app.add_new_vm('TemplateVM', label='black', name='test-template')
+        app.default_template = 'test-template'
+        app.save = unittest.mock.Mock()
+        self.vm = app.add_new_vm('AppVM', label='red', name='test-vm1',
+            template='test-template')
+        self.app = app
+        libvirt_attrs = {
+            'libvirt_conn.lookupByUUID.return_value.isActive.return_value':
+                False,
+            'libvirt_conn.lookupByUUID.return_value.state.return_value':
+                [libvirt.VIR_DOMAIN_SHUTOFF],
+        }
+        app.vmm.configure_mock(**libvirt_attrs)
+
+        self.emitter = qubes.tests.TestEmitter()
+        self.app.domains[0].fire_event = self.emitter.fire_event
+        self.app.domains[0].fire_event_pre = self.emitter.fire_event_pre
+
+    def call_mgmt_func(self, method, dest, arg=b'', payload=b''):
+        mgmt_obj = qubes.mgmt.QubesMgmt(self.app, b'dom0', method, dest, arg)
+
+        loop = asyncio.get_event_loop()
+        response = loop.run_until_complete(
+            mgmt_obj.execute(untrusted_payload=payload))
+        self.assertEventFired(self.emitter,
+            'mgmt-permission:' + method.decode('ascii'))
+        return response
+
+
+class TC_00_VMs(MgmtTestCase):
+    def test_000_vm_list(self):
+        value = self.call_mgmt_func(b'mgmt.vm.List', b'dom0')
+        self.assertEqual(value,
+            'dom0 class=AdminVM state=Running\n'
+            'test-template class=TemplateVM state=Halted\n'
+            'test-vm1 class=AppVM state=Halted\n')
+
+    def test_001_vm_list_single(self):
+        value = self.call_mgmt_func(b'mgmt.vm.List', b'test-vm1')
+        self.assertEqual(value,
+            'test-vm1 class=AppVM state=Halted\n')
+
+    def test_002_vm_list_unexpected_arg(self):
+        with self.assertRaises(AssertionError):
+            self.call_mgmt_func(b'mgmt.vm.List', b'dom0', b'test-vm1', b'')
+
+    def test_003_vm_list_unexpected_payload(self):
+        with self.assertRaises(AssertionError):
+            self.call_mgmt_func(b'mgmt.vm.List', b'dom0', b'', b'test-vm1')
+
+    def test_010_vm_property_list(self):
+        # this test is kind of stupid, but at least check if appropriate
+        # mgmt-permission event is fired
+        value = self.call_mgmt_func(b'mgmt.vm.property.List', b'test-vm1')
+        properties = self.app.domains['test-vm1'].property_list()
+        self.assertEqual(value,
+            ''.join('{}\n'.format(prop.__name__) for prop in properties))
+
+    def test_020_vm_property_get_str(self):
+        value = self.call_mgmt_func(b'mgmt.vm.property.Get', b'test-vm1',
+            b'name')
+        self.assertEqual(value, 'default=False type=str test-vm1')
+
+    def test_021_vm_property_get_int(self):
+        value = self.call_mgmt_func(b'mgmt.vm.property.Get', b'test-vm1',
+            b'vcpus')
+        self.assertEqual(value, 'default=True type=int 42')
+
+    def test_022_vm_property_get_bool(self):
+        value = self.call_mgmt_func(b'mgmt.vm.property.Get', b'test-vm1',
+            b'provides_network')
+        self.assertEqual(value, 'default=True type=bool False')
+
+    def test_023_vm_property_get_label(self):
+        value = self.call_mgmt_func(b'mgmt.vm.property.Get', b'test-vm1',
+            b'label')
+        self.assertEqual(value, 'default=False type=label red')
+
+    def test_024_vm_property_get_vm(self):
+        value = self.call_mgmt_func(b'mgmt.vm.property.Get', b'test-vm1',
+            b'template')
+        self.assertEqual(value, 'default=False type=vm test-template')
+
+    def test_025_vm_property_get_vm_none(self):
+        value = self.call_mgmt_func(b'mgmt.vm.property.Get', b'test-vm1',
+            b'netvm')
+        self.assertEqual(value, 'default=True type=vm ')
+
+    def test_030_vm_property_set_vm(self):
+        netvm = self.app.add_new_vm('AppVM', label='red', name='test-net',
+            template='test-template', provides_network=True)
+
+        with unittest.mock.patch('qubes.vm.VMProperty.__set__') as mock:
+            value = self.call_mgmt_func(b'mgmt.vm.property.Set', b'test-vm1',
+                b'netvm', b'test-net')
+            self.assertIsNone(value)
+            mock.assert_called_once_with(self.vm, netvm)
+        self.app.save.assert_called_once_with()
+
+    def test_031_vm_property_set_vm_invalid1(self):
+        with unittest.mock.patch('qubes.vm.VMProperty.__set__') as mock:
+            with self.assertRaises(qubes.exc.QubesValueError):
+                self.call_mgmt_func(b'mgmt.vm.property.Set', b'test-vm1',
+                    b'netvm', b'no-such-vm')
+            self.assertFalse(mock.called)
+        self.assertFalse(self.app.save.called)
+
+    def test_032_vm_property_set_vm_invalid2(self):
+        with unittest.mock.patch('qubes.vm.VMProperty.__set__') as mock:
+            with self.assertRaises(qubes.exc.QubesValueError):
+                self.call_mgmt_func(b'mgmt.vm.property.Set', b'test-vm1',
+                    b'netvm', b'forbidden-chars/../!')
+            self.assertFalse(mock.called)
+        self.assertFalse(self.app.save.called)
+
+    def test_033_vm_property_set_vm_invalid3(self):
+        with unittest.mock.patch('qubes.vm.VMProperty.__set__') as mock:
+            with self.assertRaises(qubes.exc.QubesValueError):
+                self.call_mgmt_func(b'mgmt.vm.property.Set', b'test-vm1',
+                    b'netvm', b'\x80\x90\xa0')
+            self.assertFalse(mock.called)
+        self.assertFalse(self.app.save.called)
+
+    def test_034_vm_propert_set_bool_true(self):
+        with unittest.mock.patch('qubes.property.__set__') as mock:
+            value = self.call_mgmt_func(b'mgmt.vm.property.Set', b'test-vm1',
+                b'autostart', b'True')
+            self.assertIsNone(value)
+            mock.assert_called_once_with(self.vm, True)
+        self.app.save.assert_called_once_with()
+
+    def test_035_vm_propert_set_bool_false(self):
+        with unittest.mock.patch('qubes.property.__set__') as mock:
+            value = self.call_mgmt_func(b'mgmt.vm.property.Set', b'test-vm1',
+                b'autostart', b'False')
+            self.assertIsNone(value)
+            mock.assert_called_once_with(self.vm, False)
+        self.app.save.assert_called_once_with()
+
+    def test_036_vm_propert_set_bool_invalid1(self):
+        with unittest.mock.patch('qubes.property.__set__') as mock:
+            with self.assertRaises(qubes.exc.QubesValueError):
+                self.call_mgmt_func(b'mgmt.vm.property.Set', b'test-vm1',
+                    b'autostart', b'some string')
+            self.assertFalse(mock.called)
+        self.assertFalse(self.app.save.called)
+
+    def test_037_vm_propert_set_bool_invalid2(self):
+        with unittest.mock.patch('qubes.property.__set__') as mock:
+            with self.assertRaises(qubes.exc.QubesValueError):
+                self.call_mgmt_func(b'mgmt.vm.property.Set', b'test-vm1',
+                    b'autostart', b'\x80\x90@#$%^&*(')
+            self.assertFalse(mock.called)
+        self.assertFalse(self.app.save.called)
+
+    def test_038_vm_propert_set_str(self):
+        with unittest.mock.patch('qubes.property.__set__') as mock:
+            value = self.call_mgmt_func(b'mgmt.vm.property.Set', b'test-vm1',
+                b'kernel', b'1.0')
+            self.assertIsNone(value)
+            mock.assert_called_once_with(self.vm, '1.0')
+        self.app.save.assert_called_once_with()
+
+    def test_039_vm_propert_set_str_invalid1(self):
+        with unittest.mock.patch('qubes.property.__set__') as mock:
+            with self.assertRaises(qubes.exc.QubesValueError):
+                self.call_mgmt_func(b'mgmt.vm.property.Set', b'test-vm1',
+                    b'kernel', b'some, non-ASCII: \x80\xd2')
+            self.assertFalse(mock.called)
+        self.assertFalse(self.app.save.called)
+
+    def test_040_vm_propert_set_int(self):
+        with unittest.mock.patch('qubes.property.__set__') as mock:
+            value = self.call_mgmt_func(b'mgmt.vm.property.Set', b'test-vm1',
+                b'maxmem', b'1024000')
+            self.assertIsNone(value)
+            mock.assert_called_once_with(self.vm, 1024000)
+        self.app.save.assert_called_once_with()
+
+    def test_041_vm_propert_set_int_invalid1(self):
+        with unittest.mock.patch('qubes.property.__set__') as mock:
+            with self.assertRaises(qubes.exc.QubesValueError):
+                self.call_mgmt_func(b'mgmt.vm.property.Set', b'test-vm1',
+                    b'maxmem', b'fourty two')
+            self.assertFalse(mock.called)
+        self.assertFalse(self.app.save.called)
+
+    def test_042_vm_propert_set_label(self):
+        with unittest.mock.patch('qubes.property.__set__') as mock:
+            value = self.call_mgmt_func(b'mgmt.vm.property.Set', b'test-vm1',
+                b'label', b'green')
+            self.assertIsNone(value)
+            mock.assert_called_once_with(self.vm, 'green')
+        self.app.save.assert_called_once_with()
+
+    def test_043_vm_propert_set_label_invalid1(self):
+        with unittest.mock.patch('qubes.property.__set__') as mock:
+            with self.assertRaises(qubes.exc.QubesValueError):
+                self.call_mgmt_func(b'mgmt.vm.property.Set', b'test-vm1',
+                    b'maxmem', b'some, non-ASCII: \x80\xd2')
+            self.assertFalse(mock.called)
+        self.assertFalse(self.app.save.called)
+
+    @unittest.skip('label existence not checked before actual setter yet')
+    def test_044_vm_propert_set_label_invalid2(self):
+        with unittest.mock.patch('qubes.property.__set__') as mock:
+            with self.assertRaises(qubes.exc.QubesValueError):
+                self.call_mgmt_func(b'mgmt.vm.property.Set', b'test-vm1',
+                    b'maxmem', b'non-existing-color')
+            self.assertFalse(mock.called)
+        self.assertFalse(self.app.save.called)
+
+    def test_050_vm_property_help(self):
+        value = self.call_mgmt_func(b'mgmt.vm.property.Help', b'test-vm1',
+            b'label')
+        self.assertEqual(value,
+            'Colourful label assigned to VM. This is where the colour of the '
+            'padlock is set.')
+        self.assertFalse(self.app.save.called)
+
+    def test_051_vm_property_help_unexpected_payload(self):
+        with self.assertRaises(AssertionError):
+            self.call_mgmt_func(b'mgmt.vm.property.Help', b'test-vm1',
+                b'label', b'asdasd')
+
+        self.assertFalse(self.app.save.called)
+
+    def test_052_vm_property_help_invalid_property(self):
+        with self.assertRaises(AssertionError):
+            self.call_mgmt_func(b'mgmt.vm.property.Help', b'test-vm1',
+                b'no-such-property')
+
+        self.assertFalse(self.app.save.called)
+
+    def test_060_vm_property_reset(self):
+        with unittest.mock.patch('qubes.property.__delete__') as mock:
+            value = self.call_mgmt_func(b'mgmt.vm.property.Reset', b'test-vm1',
+                b'default_user')
+            mock.assert_called_with(self.vm)
+        self.assertIsNone(value)
+        self.app.save.assert_called_once_with()
+
+    def test_061_vm_property_reset_unexpected_payload(self):
+        with unittest.mock.patch('qubes.property.__delete__') as mock:
+            with self.assertRaises(AssertionError):
+                self.call_mgmt_func(b'mgmt.vm.property.Help', b'test-vm1',
+                    b'label', b'asdasd')
+            self.assertFalse(mock.called)
+        self.assertFalse(self.app.save.called)
+
+    def test_062_vm_property_reset_invalid_property(self):
+        with unittest.mock.patch('qubes.property.__delete__') as mock:
+            with self.assertRaises(AssertionError):
+                self.call_mgmt_func(b'mgmt.vm.property.Help', b'test-vm1',
+                    b'no-such-property')
+            self.assertFalse(mock.called)
+        self.assertFalse(self.app.save.called)
diff --git a/rpm_spec/core-dom0.spec b/rpm_spec/core-dom0.spec
index 1cf8ef79..1330d38b 100644
--- a/rpm_spec/core-dom0.spec
+++ b/rpm_spec/core-dom0.spec
@@ -314,6 +314,7 @@ fi
 %{python3_sitelib}/qubes/tests/events.py
 %{python3_sitelib}/qubes/tests/firewall.py
 %{python3_sitelib}/qubes/tests/init.py
+%{python3_sitelib}/qubes/tests/mgmt.py
 %{python3_sitelib}/qubes/tests/storage.py
 %{python3_sitelib}/qubes/tests/storage_file.py
 %{python3_sitelib}/qubes/tests/storage_lvm.py
diff --git a/test-packages/libvirt.py b/test-packages/libvirt.py
index 4c850656..c227e986 100644
--- a/test-packages/libvirt.py
+++ b/test-packages/libvirt.py
@@ -13,3 +13,11 @@ class libvirtError(Exception):
 
 def openReadOnly(*args, **kwargs):
     raise libvirtError('mock module, always raises')
+
+VIR_DOMAIN_BLOCKED = 2
+VIR_DOMAIN_RUNNING = 1
+VIR_DOMAIN_PAUSED = 3
+VIR_DOMAIN_SHUTDOWN = 4
+VIR_DOMAIN_SHUTOFF = 5
+VIR_DOMAIN_CRASHED = 6
+VIR_DOMAIN_PMSUSPENDED = 7

From beaa0e93480b3a2551a7ca31f95f05b39d57e14d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?=
 <marmarek@invisiblethingslab.com>
Date: Thu, 16 Mar 2017 14:19:25 +0100
Subject: [PATCH 20/31] tests/mgmt: check if argument/payload is rejected when
 should be

Instead of creating such tests for each method separately, use unittest
subTest functionality to handle all of them at once.
---
 qubes/tests/mgmt.py | 347 +++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 324 insertions(+), 23 deletions(-)

diff --git a/qubes/tests/mgmt.py b/qubes/tests/mgmt.py
index 4168e54c..8c44b9e3 100644
--- a/qubes/tests/mgmt.py
+++ b/qubes/tests/mgmt.py
@@ -79,14 +79,6 @@ class TC_00_VMs(MgmtTestCase):
         self.assertEqual(value,
             'test-vm1 class=AppVM state=Halted\n')
 
-    def test_002_vm_list_unexpected_arg(self):
-        with self.assertRaises(AssertionError):
-            self.call_mgmt_func(b'mgmt.vm.List', b'dom0', b'test-vm1', b'')
-
-    def test_003_vm_list_unexpected_payload(self):
-        with self.assertRaises(AssertionError):
-            self.call_mgmt_func(b'mgmt.vm.List', b'dom0', b'', b'test-vm1')
-
     def test_010_vm_property_list(self):
         # this test is kind of stupid, but at least check if appropriate
         # mgmt-permission event is fired
@@ -257,13 +249,6 @@ class TC_00_VMs(MgmtTestCase):
             'padlock is set.')
         self.assertFalse(self.app.save.called)
 
-    def test_051_vm_property_help_unexpected_payload(self):
-        with self.assertRaises(AssertionError):
-            self.call_mgmt_func(b'mgmt.vm.property.Help', b'test-vm1',
-                b'label', b'asdasd')
-
-        self.assertFalse(self.app.save.called)
-
     def test_052_vm_property_help_invalid_property(self):
         with self.assertRaises(AssertionError):
             self.call_mgmt_func(b'mgmt.vm.property.Help', b'test-vm1',
@@ -279,14 +264,6 @@ class TC_00_VMs(MgmtTestCase):
         self.assertIsNone(value)
         self.app.save.assert_called_once_with()
 
-    def test_061_vm_property_reset_unexpected_payload(self):
-        with unittest.mock.patch('qubes.property.__delete__') as mock:
-            with self.assertRaises(AssertionError):
-                self.call_mgmt_func(b'mgmt.vm.property.Help', b'test-vm1',
-                    b'label', b'asdasd')
-            self.assertFalse(mock.called)
-        self.assertFalse(self.app.save.called)
-
     def test_062_vm_property_reset_invalid_property(self):
         with unittest.mock.patch('qubes.property.__delete__') as mock:
             with self.assertRaises(AssertionError):
@@ -294,3 +271,327 @@ class TC_00_VMs(MgmtTestCase):
                     b'no-such-property')
             self.assertFalse(mock.called)
         self.assertFalse(self.app.save.called)
+
+    def test_990_vm_unexpected_payload(self):
+        methods_with_no_payload = [
+            b'mgmt.vm.List',
+            b'mgmt.vm.Remove',
+            b'mgmt.vm.property.List',
+            b'mgmt.vm.property.Get',
+            b'mgmt.vm.property.Help',
+            b'mgmt.vm.property.HelpRst',
+            b'mgmt.vm.property.Reset',
+            b'mgmt.vm.feature.List',
+            b'mgmt.vm.feature.Get',
+            b'mgmt.vm.feature.CheckWithTemplate',
+            b'mgmt.vm.feature.Remove',
+            b'mgmt.vm.tag.List',
+            b'mgmt.vm.tag.Get',
+            b'mgmt.vm.tag.Remove',
+            b'mgmt.vm.tag.Set',
+            b'mgmt.vm.firewall.Get',
+            b'mgmt.vm.firewall.RemoveRule',
+            b'mgmt.vm.firewall.Flush',
+            b'mgmt.vm.device.pci.Attach',
+            b'mgmt.vm.device.pci.Detach',
+            b'mgmt.vm.device.pci.List',
+            b'mgmt.vm.device.pci.Available',
+            b'mgmt.vm.microphone.Attach',
+            b'mgmt.vm.microphone.Detach',
+            b'mgmt.vm.microphone.Status',
+            b'mgmt.vm.volume.ListSnapshots',
+            b'mgmt.vm.volume.List',
+            b'mgmt.vm.volume.Info',
+            b'mgmt.vm.Start',
+            b'mgmt.vm.Shutdown',
+            b'mgmt.vm.Pause',
+            b'mgmt.vm.Unpause',
+            b'mgmt.vm.Kill',
+        ]
+        # make sure also no methods on actual VM gets called
+        vm_mock = unittest.mock.MagicMock()
+        vm_mock.name = self.vm.name
+        vm_mock.qid = self.vm.qid
+        vm_mock.__lt__ = (lambda x, y: x.qid < y.qid)
+        self.app.domains._dict[self.vm.qid] = vm_mock
+        for method in methods_with_no_payload:
+            # should reject payload regardless of having argument or not
+            with self.subTest(method.decode('ascii')):
+                with self.assertRaises(AssertionError):
+                    self.call_mgmt_func(method, b'test-vm1', b'',
+                        b'unexpected-payload')
+                self.assertFalse(vm_mock.called)
+                self.assertFalse(self.app.save.called)
+
+            with self.subTest(method.decode('ascii') + '+arg'):
+                with self.assertRaises(AssertionError):
+                    self.call_mgmt_func(method, b'test-vm1', b'some-arg',
+                        b'unexpected-payload')
+                self.assertFalse(vm_mock.called)
+                self.assertFalse(self.app.save.called)
+
+    def test_991_vm_unexpected_argument(self):
+        methods_with_no_argument = [
+            b'mgmt.vm.List',
+            b'mgmt.vm.Clone',
+            b'mgmt.vm.Remove',
+            b'mgmt.vm.property.List',
+            b'mgmt.vm.feature.List',
+            b'mgmt.vm.tag.List',
+            b'mgmt.vm.firewall.List',
+            b'mgmt.vm.firewall.Flush',
+            b'mgmt.vm.device.pci.List',
+            b'mgmt.vm.device.pci.Available',
+            b'mgmt.vm.microphone.Attach',
+            b'mgmt.vm.microphone.Detach',
+            b'mgmt.vm.microphone.Status',
+            b'mgmt.vm.volume.List',
+            b'mgmt.vm.Start',
+            b'mgmt.vm.Shutdown',
+            b'mgmt.vm.Pause',
+            b'mgmt.vm.Unpause',
+            b'mgmt.vm.Kill',
+        ]
+        # make sure also no methods on actual VM gets called
+        vm_mock = unittest.mock.MagicMock()
+        vm_mock.name = self.vm.name
+        vm_mock.qid = self.vm.qid
+        vm_mock.__lt__ = (lambda x, y: x.qid < y.qid)
+        self.app.domains._dict[self.vm.qid] = vm_mock
+        for method in methods_with_no_argument:
+            # should reject argument regardless of having payload or not
+            with self.subTest(method.decode('ascii')):
+                with self.assertRaises(AssertionError):
+                    self.call_mgmt_func(method, b'test-vm1', b'some-arg',
+                        b'')
+                self.assertFalse(vm_mock.called)
+                self.assertFalse(self.app.save.called)
+
+            with self.subTest(method.decode('ascii') + '+payload'):
+                with self.assertRaises(AssertionError):
+                    self.call_mgmt_func(method, b'test-vm1', b'unexpected-arg',
+                        b'some-payload')
+                self.assertFalse(vm_mock.called)
+                self.assertFalse(self.app.save.called)
+
+    def test_992_dom0_unexpected_payload(self):
+        methods_with_no_payload = [
+            b'mgmt.vmclass.List',
+            b'mgmt.vm.List',
+            b'mgmt.label.List',
+            b'mgmt.label.Get',
+            b'mgmt.label.Remove',
+            b'mgmt.property.List',
+            b'mgmt.property.Get',
+            b'mgmt.property.Help',
+            b'mgmt.property.HelpRst',
+            b'mgmt.property.Reset',
+            b'mgmt.pool.List',
+            b'mgmt.pool.ListDrivers',
+            b'mgmt.pool.Info',
+            b'mgmt.pool.Remove',
+            b'mgmt.backup.Execute',
+        ]
+        # make sure also no methods on actual VM gets called
+        vm_mock = unittest.mock.MagicMock()
+        vm_mock.name = self.vm.name
+        vm_mock.qid = self.vm.qid
+        vm_mock.__lt__ = (lambda x, y: x.qid < y.qid)
+        self.app.domains._dict[self.vm.qid] = vm_mock
+        for method in methods_with_no_payload:
+            # should reject payload regardless of having argument or not
+            with self.subTest(method.decode('ascii')):
+                with self.assertRaises(AssertionError):
+                    self.call_mgmt_func(method, b'dom0', b'',
+                        b'unexpected-payload')
+                self.assertFalse(vm_mock.called)
+                self.assertFalse(self.app.save.called)
+
+            with self.subTest(method.decode('ascii') + '+arg'):
+                with self.assertRaises(AssertionError):
+                    self.call_mgmt_func(method, b'dom0', b'some-arg',
+                        b'unexpected-payload')
+                self.assertFalse(vm_mock.called)
+                self.assertFalse(self.app.save.called)
+
+    def test_993_dom0_unexpected_argument(self):
+        methods_with_no_argument = [
+            b'mgmt.vmclass.List',
+            b'mgmt.vm.List',
+            b'mgmt.label.List',
+            b'mgmt.property.List',
+            b'mgmt.pool.List',
+            b'mgmt.pool.ListDrivers',
+        ]
+        # make sure also no methods on actual VM gets called
+        vm_mock = unittest.mock.MagicMock()
+        vm_mock.name = self.vm.name
+        vm_mock.qid = self.vm.qid
+        vm_mock.__lt__ = (lambda x, y: x.qid < y.qid)
+        self.app.domains._dict[self.vm.qid] = vm_mock
+        for method in methods_with_no_argument:
+            # should reject argument regardless of having payload or not
+            with self.subTest(method.decode('ascii')):
+                with self.assertRaises(AssertionError):
+                    self.call_mgmt_func(method, b'dom0', b'some-arg',
+                        b'')
+                self.assertFalse(vm_mock.called)
+                self.assertFalse(self.app.save.called)
+
+            with self.subTest(method.decode('ascii') + '+payload'):
+                with self.assertRaises(AssertionError):
+                    self.call_mgmt_func(method, b'dom0', b'unexpected-arg',
+                        b'some-payload')
+                self.assertFalse(vm_mock.called)
+                self.assertFalse(self.app.save.called)
+
+    def test_994_dom0_only_calls(self):
+        # TODO set some better arguments, to make sure the call was rejected
+        # because of invalid destination, not invalid arguments
+        methods_for_dom0_only = [
+            b'mgmt.vmclass.List',
+            b'mgmt.vm.Create.AppVM',
+            b'mgmt.vm.CreateInPool.AppVM',
+            b'mgmt.vm.CreateTemplate',
+            b'mgmt.label.List',
+            b'mgmt.label.Create',
+            b'mgmt.label.Get',
+            b'mgmt.label.Remove',
+            b'mgmt.property.List',
+            b'mgmt.property.Get',
+            b'mgmt.property.Set',
+            b'mgmt.property.Help',
+            b'mgmt.property.HelpRst',
+            b'mgmt.property.Reset',
+            b'mgmt.pool.List',
+            b'mgmt.pool.ListDrivers',
+            b'mgmt.pool.Info',
+            b'mgmt.pool.Add',
+            b'mgmt.pool.Remove',
+            b'mgmt.pool.volume.List',
+            b'mgmt.pool.volume.Info',
+            b'mgmt.pool.volume.ListSnapshots',
+            b'mgmt.pool.volume.Snapshot',
+            b'mgmt.pool.volume.Revert',
+            b'mgmt.pool.volume.Resize',
+            b'mgmt.backup.Execute',
+            b'mgmt.backup.Info',
+            b'mgmt.backup.Restore',
+        ]
+        # make sure also no methods on actual VM gets called
+        vm_mock = unittest.mock.MagicMock()
+        vm_mock.name = self.vm.name
+        vm_mock.qid = self.vm.qid
+        vm_mock.__lt__ = (lambda x, y: x.qid < y.qid)
+        self.app.domains._dict[self.vm.qid] = vm_mock
+        for method in methods_for_dom0_only:
+            # should reject call regardless of having payload or not
+            with self.subTest(method.decode('ascii')):
+                with self.assertRaises(AssertionError):
+                    self.call_mgmt_func(method, b'test-vm1', b'',
+                        b'')
+                self.assertFalse(vm_mock.called)
+                self.assertFalse(self.app.save.called)
+
+            with self.subTest(method.decode('ascii') + '+arg'):
+                with self.assertRaises(AssertionError):
+                    self.call_mgmt_func(method, b'test-vm1', b'some-arg',
+                        b'')
+                self.assertFalse(vm_mock.called)
+                self.assertFalse(self.app.save.called)
+
+            with self.subTest(method.decode('ascii') + '+payload'):
+                with self.assertRaises(AssertionError):
+                    self.call_mgmt_func(method, b'test-vm1', b'',
+                        b'payload')
+                self.assertFalse(vm_mock.called)
+                self.assertFalse(self.app.save.called)
+
+            with self.subTest(method.decode('ascii') + '+arg+payload'):
+                with self.assertRaises(AssertionError):
+                    self.call_mgmt_func(method, b'test-vm1', b'some-arg',
+                        b'some-payload')
+                self.assertFalse(vm_mock.called)
+                self.assertFalse(self.app.save.called)
+
+    @unittest.skip('undecided')
+    def test_995_vm_only_calls(self):
+        # XXX is it really a good idea to prevent those calls this early?
+        # TODO set some better arguments, to make sure the call was rejected
+        # because of invalid destination, not invalid arguments
+        methods_for_vm_only = [
+            b'mgmt.vm.Clone',
+            b'mgmt.vm.Remove',
+            b'mgmt.vm.property.List',
+            b'mgmt.vm.property.Get',
+            b'mgmt.vm.property.Set',
+            b'mgmt.vm.property.Help',
+            b'mgmt.vm.property.HelpRst',
+            b'mgmt.vm.property.Reset',
+            b'mgmt.vm.feature.List',
+            b'mgmt.vm.feature.Get',
+            b'mgmt.vm.feature.Set',
+            b'mgmt.vm.feature.CheckWithTemplate',
+            b'mgmt.vm.feature.Remove',
+            b'mgmt.vm.tag.List',
+            b'mgmt.vm.tag.Get',
+            b'mgmt.vm.tag.Remove',
+            b'mgmt.vm.tag.Set',
+            b'mgmt.vm.firewall.Get',
+            b'mgmt.vm.firewall.RemoveRule',
+            b'mgmt.vm.firewall.InsertRule',
+            b'mgmt.vm.firewall.Flush',
+            b'mgmt.vm.device.pci.Attach',
+            b'mgmt.vm.device.pci.Detach',
+            b'mgmt.vm.device.pci.List',
+            b'mgmt.vm.device.pci.Available',
+            b'mgmt.vm.microphone.Attach',
+            b'mgmt.vm.microphone.Detach',
+            b'mgmt.vm.microphone.Status',
+            b'mgmt.vm.volume.ListSnapshots',
+            b'mgmt.vm.volume.List',
+            b'mgmt.vm.volume.Info',
+            b'mgmt.vm.volume.Revert',
+            b'mgmt.vm.volume.Resize',
+            b'mgmt.vm.Start',
+            b'mgmt.vm.Shutdown',
+            b'mgmt.vm.Pause',
+            b'mgmt.vm.Unpause',
+            b'mgmt.vm.Kill',
+        ]
+        # make sure also no methods on actual VM gets called
+        vm_mock = unittest.mock.MagicMock()
+        vm_mock.name = self.vm.name
+        vm_mock.qid = self.vm.qid
+        vm_mock.__lt__ = (lambda x, y: x.qid < y.qid)
+        self.app.domains._dict[self.vm.qid] = vm_mock
+        for method in methods_for_vm_only:
+            # should reject payload regardless of having argument or not
+            # should reject call regardless of having payload or not
+            with self.subTest(method.decode('ascii')):
+                with self.assertRaises(AssertionError):
+                    self.call_mgmt_func(method, b'dom0', b'',
+                        b'')
+                self.assertFalse(vm_mock.called)
+                self.assertFalse(self.app.save.called)
+
+            with self.subTest(method.decode('ascii') + '+arg'):
+                with self.assertRaises(AssertionError):
+                    self.call_mgmt_func(method, b'dom0', b'some-arg',
+                        b'')
+                self.assertFalse(vm_mock.called)
+                self.assertFalse(self.app.save.called)
+
+            with self.subTest(method.decode('ascii') + '+payload'):
+                with self.assertRaises(AssertionError):
+                    self.call_mgmt_func(method, b'dom0', b'',
+                        b'payload')
+                self.assertFalse(vm_mock.called)
+                self.assertFalse(self.app.save.called)
+
+            with self.subTest(method.decode('ascii') + '+arg+payload'):
+                with self.assertRaises(AssertionError):
+                    self.call_mgmt_func(method, b'dom0', b'some-arg',
+                        b'some-payload')
+                self.assertFalse(vm_mock.called)
+                self.assertFalse(self.app.save.called)

From f1c8f3220d7b8ab090cf66b2ed10aeaac0397a5f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?=
 <marmarek@invisiblethingslab.com>
Date: Thu, 16 Mar 2017 15:30:49 +0100
Subject: [PATCH 21/31] tests: deal with events containing dict in kwargs

If kwargs contains dict as one of values, it isn't hashable and can't be
used as value in frozenset/tuple. Convert such values into
frozenset(dict.items()). Only one (more) level is supported, but it
should be enough.

Solution from http://stackoverflow.com/a/13264725
---
 qubes/tests/__init__.py | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/qubes/tests/__init__.py b/qubes/tests/__init__.py
index ba3a00dd..7ee7f5c9 100644
--- a/qubes/tests/__init__.py
+++ b/qubes/tests/__init__.py
@@ -143,12 +143,22 @@ class TestEmitter(qubes.events.Emitter):
 
     def fire_event(self, event, **kwargs):
         effects = super(TestEmitter, self).fire_event(event, **kwargs)
-        self.fired_events[(event, tuple(kwargs.items()))] += 1
+        ev_kwargs = frozenset(
+            (key,
+                frozenset(value.items()) if isinstance(value, dict) else value)
+            for key, value in kwargs.items()
+        )
+        self.fired_events[(event, ev_kwargs)] += 1
         return effects
 
     def fire_event_pre(self, event, **kwargs):
         effects = super(TestEmitter, self).fire_event_pre(event, **kwargs)
-        self.fired_events[(event, tuple(kwargs.items()))] += 1
+        ev_kwargs = frozenset(
+            (key,
+                frozenset(value.items()) if isinstance(value, dict) else value)
+            for key, value in kwargs.items()
+        )
+        self.fired_events[(event, ev_kwargs)] += 1
         return effects
 
 def expectedFailureIfTemplate(templates):

From 2a223811a56831e9e96548d1973d2618db29604e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?=
 <marmarek@invisiblethingslab.com>
Date: Thu, 16 Mar 2017 19:54:22 +0100
Subject: [PATCH 22/31] tests: add missing import

---
 qubes/tests/__init__.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/qubes/tests/__init__.py b/qubes/tests/__init__.py
index 7ee7f5c9..7b0b2684 100644
--- a/qubes/tests/__init__.py
+++ b/qubes/tests/__init__.py
@@ -45,6 +45,7 @@ import unittest
 from distutils import spawn
 
 import lxml.etree
+import pkg_resources
 
 import qubes.backup
 import qubes.config

From 8371ffcd0f943d49157b353628179d95ca8a41fa Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?=
 <marmarek@invisiblethingslab.com>
Date: Thu, 16 Mar 2017 19:58:09 +0100
Subject: [PATCH 23/31] tests: storage and labels

QubesOS/qubes-issues#2622
---
 qubes/tests/mgmt.py | 522 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 522 insertions(+)

diff --git a/qubes/tests/mgmt.py b/qubes/tests/mgmt.py
index 8c44b9e3..f154d257 100644
--- a/qubes/tests/mgmt.py
+++ b/qubes/tests/mgmt.py
@@ -28,6 +28,11 @@ import qubes
 import qubes.tests
 import qubes.mgmt
 
+# properties defined in API
+volume_properties = [
+    'pool', 'vid', 'size', 'usage', 'rw', 'internal', 'source',
+    'save_on_stop', 'snap_on_start']
+
 
 class MgmtTestCase(qubes.tests.QubesTestCase):
     def setUp(self):
@@ -272,6 +277,523 @@ class TC_00_VMs(MgmtTestCase):
             self.assertFalse(mock.called)
         self.assertFalse(self.app.save.called)
 
+    def test_070_vm_volume_list(self):
+        self.vm.volumes = unittest.mock.Mock()
+        volumes_conf = {
+            'keys.return_value': ['root', 'private', 'volatile', 'kernel']
+        }
+        self.vm.volumes.configure_mock(**volumes_conf)
+        value = self.call_mgmt_func(b'mgmt.vm.volume.List', b'test-vm1')
+        self.assertEqual(value, 'root\nprivate\nvolatile\nkernel\n')
+        # check if _only_ keys were accessed
+        self.assertEqual(self.vm.volumes.mock_calls,
+            [unittest.mock.call.keys()])
+
+    def test_080_vm_volume_info(self):
+        self.vm.volumes = unittest.mock.MagicMock()
+        volumes_conf = {
+            'keys.return_value': ['root', 'private', 'volatile', 'kernel']
+        }
+        for prop in volume_properties:
+            volumes_conf[
+                '__getitem__.return_value.{}'.format(prop)] = prop +'-value'
+        self.vm.volumes.configure_mock(**volumes_conf)
+        value = self.call_mgmt_func(b'mgmt.vm.volume.Info', b'test-vm1',
+            b'private')
+        self.assertEqual(value,
+            ''.join('{p}={p}-value\n'.format(p=p) for p in volume_properties))
+        self.assertEqual(self.vm.volumes.mock_calls,
+            [unittest.mock.call.keys(),
+             unittest.mock.call.__getattr__('__getitem__')('private')])
+
+    def test_080_vm_volume_info_invalid_volume(self):
+        self.vm.volumes = unittest.mock.MagicMock()
+        volumes_conf = {
+            'keys.return_value': ['root', 'private', 'volatile', 'kernel']
+        }
+        self.vm.volumes.configure_mock(**volumes_conf)
+        with self.assertRaises(AssertionError):
+            self.call_mgmt_func(b'mgmt.vm.volume.Info', b'test-vm1',
+                b'no-such-volume')
+        self.assertEqual(self.vm.volumes.mock_calls,
+            [unittest.mock.call.keys()])
+
+    def test_090_vm_volume_listsnapshots(self):
+        self.vm.volumes = unittest.mock.MagicMock()
+        volumes_conf = {
+            'keys.return_value': ['root', 'private', 'volatile', 'kernel'],
+            '__getitem__.return_value.revisions': ['rev1', 'rev2'],
+        }
+        self.vm.volumes.configure_mock(**volumes_conf)
+        value = self.call_mgmt_func(b'mgmt.vm.volume.ListSnapshots',
+            b'test-vm1', b'private')
+        self.assertEqual(value,
+            'rev1\nrev2\n')
+        self.assertEqual(self.vm.volumes.mock_calls,
+            [unittest.mock.call.keys(),
+            unittest.mock.call.__getattr__('__getitem__')('private')])
+
+    def test_090_vm_volume_listsnapshots_invalid_volume(self):
+        self.vm.volumes = unittest.mock.MagicMock()
+        volumes_conf = {
+            'keys.return_value': ['root', 'private', 'volatile', 'kernel']
+        }
+        self.vm.volumes.configure_mock(**volumes_conf)
+        with self.assertRaises(AssertionError):
+            self.call_mgmt_func(b'mgmt.vm.volume.ListSnapshots', b'test-vm1',
+                b'no-such-volume')
+        self.assertEqual(self.vm.volumes.mock_calls,
+            [unittest.mock.call.keys()])
+
+    @unittest.skip('method not implemented yet')
+    def test_100_vm_volume_snapshot(self):
+        pass
+
+    @unittest.skip('method not implemented yet')
+    def test_100_vm_volume_snapshot_invlid_volume(self):
+        self.vm.volumes = unittest.mock.MagicMock()
+        volumes_conf = {
+            'keys.return_value': ['root', 'private', 'volatile', 'kernel'],
+            '__getitem__.return_value.revisions': ['rev1', 'rev2'],
+        }
+        self.vm.volumes.configure_mock(**volumes_conf)
+        with self.assertRaises(AssertionError):
+            self.call_mgmt_func(b'mgmt.vm.volume.Snapshots',
+                b'test-vm1', b'no-such-volume')
+        self.assertEqual(self.vm.volumes.mock_calls,
+            [unittest.mock.call.keys()])
+
+    @unittest.skip('method not implemented yet')
+    def test_100_vm_volume_snapshot_invalid_revision(self):
+        self.vm.volumes = unittest.mock.MagicMock()
+        volumes_conf = {
+            'keys.return_value': ['root', 'private', 'volatile', 'kernel']
+        }
+        self.vm.volumes.configure_mock(**volumes_conf)
+        with self.assertRaises(AssertionError):
+            self.call_mgmt_func(b'mgmt.vm.volume.Snapshots',
+                b'test-vm1', b'private', b'no-such-rev')
+        self.assertEqual(self.vm.volumes.mock_calls,
+            [unittest.mock.call.keys(),
+            unittest.mock.call.__getattr__('__getitem__')('private')])
+
+    def test_110_vm_volume_revert(self):
+        self.vm.volumes = unittest.mock.MagicMock()
+        volumes_conf = {
+            'keys.return_value': ['root', 'private', 'volatile', 'kernel'],
+            '__getitem__.return_value.revisions': ['rev1', 'rev2'],
+        }
+        self.vm.volumes.configure_mock(**volumes_conf)
+        self.vm.storage = unittest.mock.Mock()
+        value = self.call_mgmt_func(b'mgmt.vm.volume.Revert',
+            b'test-vm1', b'private', b'rev1')
+        self.assertIsNone(value)
+        self.assertEqual(self.vm.volumes.mock_calls,
+            [unittest.mock.call.keys(),
+                unittest.mock.call.__getattr__('__getitem__')('private')])
+        self.assertEqual(self.vm.storage.mock_calls,
+            [unittest.mock.call.get_pool(self.vm.volumes['private']),
+             unittest.mock.call.get_pool().revert('rev1')])
+
+    def test_110_vm_volume_revert_invalid_rev(self):
+        self.vm.volumes = unittest.mock.MagicMock()
+        volumes_conf = {
+            'keys.return_value': ['root', 'private', 'volatile', 'kernel'],
+            '__getitem__.return_value.revisions': ['rev1', 'rev2'],
+        }
+        self.vm.volumes.configure_mock(**volumes_conf)
+        self.vm.storage = unittest.mock.Mock()
+        with self.assertRaises(AssertionError):
+            self.call_mgmt_func(b'mgmt.vm.volume.Revert',
+                b'test-vm1', b'private', b'no-such-rev')
+        self.assertEqual(self.vm.volumes.mock_calls,
+            [unittest.mock.call.keys(),
+                unittest.mock.call.__getattr__('__getitem__')('private')])
+        self.assertFalse(self.vm.storage.called)
+
+    def test_120_vm_volume_resize(self):
+        self.vm.volumes = unittest.mock.MagicMock()
+        volumes_conf = {
+            'keys.return_value': ['root', 'private', 'volatile', 'kernel'],
+        }
+        self.vm.volumes.configure_mock(**volumes_conf)
+        self.vm.storage = unittest.mock.Mock()
+        value = self.call_mgmt_func(b'mgmt.vm.volume.Resize',
+            b'test-vm1', b'private', b'1024000000')
+        self.assertIsNone(value)
+        self.assertEqual(self.vm.volumes.mock_calls,
+            [unittest.mock.call.keys()])
+        self.assertEqual(self.vm.storage.mock_calls,
+            [unittest.mock.call.resize('private', 1024000000)])
+
+    def test_120_vm_volume_resize_invalid_size1(self):
+        self.vm.volumes = unittest.mock.MagicMock()
+        volumes_conf = {
+            'keys.return_value': ['root', 'private', 'volatile', 'kernel'],
+        }
+        self.vm.volumes.configure_mock(**volumes_conf)
+        self.vm.storage = unittest.mock.Mock()
+        with self.assertRaises(AssertionError):
+            self.call_mgmt_func(b'mgmt.vm.volume.Resize',
+                b'test-vm1', b'private', b'no-int-size')
+        self.assertEqual(self.vm.volumes.mock_calls,
+            [unittest.mock.call.keys()])
+        self.assertFalse(self.vm.storage.called)
+
+    def test_120_vm_volume_resize_invalid_size2(self):
+        self.vm.volumes = unittest.mock.MagicMock()
+        volumes_conf = {
+            'keys.return_value': ['root', 'private', 'volatile', 'kernel'],
+        }
+        self.vm.volumes.configure_mock(**volumes_conf)
+        self.vm.storage = unittest.mock.Mock()
+        with self.assertRaises(AssertionError):
+            self.call_mgmt_func(b'mgmt.vm.volume.Resize',
+                b'test-vm1', b'private', b'-1')
+        self.assertEqual(self.vm.volumes.mock_calls,
+            [unittest.mock.call.keys()])
+        self.assertFalse(self.vm.storage.called)
+
+    def test_130_pool_list(self):
+        self.app.pools = ['file', 'lvm']
+        value = self.call_mgmt_func(b'mgmt.pool.List', b'dom0')
+        self.assertEqual(value, 'file\nlvm\n')
+        self.assertFalse(self.app.save.called)
+
+    @unittest.mock.patch('qubes.storage.pool_drivers')
+    @unittest.mock.patch('qubes.storage.driver_parameters')
+    def test_140_pool_listdrivers(self, mock_parameters, mock_drivers):
+        self.app.pools = ['file', 'lvm']
+
+        mock_drivers.return_value = ['driver1', 'driver2']
+        mock_parameters.side_effect = \
+            lambda driver: {
+                'driver1': ['param1', 'param2'],
+                'driver2': ['param3', 'param4']
+            }[driver]
+
+        value = self.call_mgmt_func(b'mgmt.pool.ListDrivers', b'dom0')
+        self.assertEqual(value,
+            'driver1 param1 param2\ndriver2 param3 param4\n')
+        self.assertEqual(mock_drivers.mock_calls, [unittest.mock.call()])
+        self.assertEqual(mock_parameters.mock_calls,
+            [unittest.mock.call('driver1'), unittest.mock.call('driver2')])
+        self.assertFalse(self.app.save.called)
+
+    def test_150_pool_info(self):
+        self.app.pools = {
+            'pool1': unittest.mock.Mock(config={
+                'param1': 'value1', 'param2': 'value2'})
+        }
+        value = self.call_mgmt_func(b'mgmt.pool.Info', b'dom0', b'pool1')
+
+        self.assertEqual(value, 'param1=value1\nparam2=value2\n')
+        self.assertFalse(self.app.save.called)
+
+    @unittest.mock.patch('qubes.storage.pool_drivers')
+    @unittest.mock.patch('qubes.storage.driver_parameters')
+    def test_160_pool_add(self, mock_parameters, mock_drivers):
+        self.app.pools = {
+            'file': unittest.mock.Mock(),
+            'lvm': unittest.mock.Mock()
+        }
+
+        mock_drivers.return_value = ['driver1', 'driver2']
+        mock_parameters.side_effect = \
+            lambda driver: {
+                'driver1': ['param1', 'param2'],
+                'driver2': ['param3', 'param4']
+            }[driver]
+
+        self.app.add_pool = unittest.mock.Mock()
+
+        value = self.call_mgmt_func(b'mgmt.pool.Add', b'dom0', b'driver1',
+            b'name=test-pool\nparam1=some-value\n')
+        self.assertIsNone(value)
+        self.assertEqual(mock_drivers.mock_calls, [unittest.mock.call()])
+        self.assertEqual(mock_parameters.mock_calls,
+            [unittest.mock.call('driver1')])
+        self.assertEqual(self.app.add_pool.mock_calls,
+            [unittest.mock.call(name='test-pool', driver='driver1',
+                param1='some-value')])
+        self.assertTrue(self.app.save.called)
+
+    @unittest.mock.patch('qubes.storage.pool_drivers')
+    @unittest.mock.patch('qubes.storage.driver_parameters')
+    def test_160_pool_add_invalid_driver(self, mock_parameters, mock_drivers):
+        self.app.pools = {
+            'file': unittest.mock.Mock(),
+            'lvm': unittest.mock.Mock()
+        }
+
+        mock_drivers.return_value = ['driver1', 'driver2']
+        mock_parameters.side_effect = \
+            lambda driver: {
+                'driver1': ['param1', 'param2'],
+                'driver2': ['param3', 'param4']
+            }[driver]
+
+        self.app.add_pool = unittest.mock.Mock()
+
+        with self.assertRaises(AssertionError):
+            self.call_mgmt_func(b'mgmt.pool.Add', b'dom0',
+                b'no-such-driver', b'name=test-pool\nparam1=some-value\n')
+        self.assertEqual(mock_drivers.mock_calls, [unittest.mock.call()])
+        self.assertEqual(mock_parameters.mock_calls, [])
+        self.assertEqual(self.app.add_pool.mock_calls, [])
+        self.assertFalse(self.app.save.called)
+
+
+    @unittest.mock.patch('qubes.storage.pool_drivers')
+    @unittest.mock.patch('qubes.storage.driver_parameters')
+    def test_160_pool_add_invalid_param(self, mock_parameters, mock_drivers):
+        self.app.pools = {
+            'file': unittest.mock.Mock(),
+            'lvm': unittest.mock.Mock()
+        }
+
+        mock_drivers.return_value = ['driver1', 'driver2']
+        mock_parameters.side_effect = \
+            lambda driver: {
+                'driver1': ['param1', 'param2'],
+                'driver2': ['param3', 'param4']
+            }[driver]
+
+        self.app.add_pool = unittest.mock.Mock()
+
+        with self.assertRaises(AssertionError):
+            self.call_mgmt_func(b'mgmt.pool.Add', b'dom0',
+                b'driver1', b'name=test-pool\nparam3=some-value\n')
+        self.assertEqual(mock_drivers.mock_calls, [unittest.mock.call()])
+        self.assertEqual(mock_parameters.mock_calls,
+            [unittest.mock.call('driver1')])
+        self.assertEqual(self.app.add_pool.mock_calls, [])
+        self.assertFalse(self.app.save.called)
+
+    @unittest.mock.patch('qubes.storage.pool_drivers')
+    @unittest.mock.patch('qubes.storage.driver_parameters')
+    def test_160_pool_add_missing_name(self, mock_parameters, mock_drivers):
+        self.app.pools = {
+            'file': unittest.mock.Mock(),
+            'lvm': unittest.mock.Mock()
+        }
+
+        mock_drivers.return_value = ['driver1', 'driver2']
+        mock_parameters.side_effect = \
+            lambda driver: {
+                'driver1': ['param1', 'param2'],
+                'driver2': ['param3', 'param4']
+            }[driver]
+
+        self.app.add_pool = unittest.mock.Mock()
+
+        with self.assertRaises(AssertionError):
+            self.call_mgmt_func(b'mgmt.pool.Add', b'dom0',
+                b'driver1', b'param1=value\nparam2=some-value\n')
+        self.assertEqual(mock_drivers.mock_calls, [unittest.mock.call()])
+        self.assertEqual(mock_parameters.mock_calls, [])
+        self.assertEqual(self.app.add_pool.mock_calls, [])
+        self.assertFalse(self.app.save.called)
+
+    @unittest.mock.patch('qubes.storage.pool_drivers')
+    @unittest.mock.patch('qubes.storage.driver_parameters')
+    def test_160_pool_add_existing_pool(self, mock_parameters, mock_drivers):
+        self.app.pools = {
+            'file': unittest.mock.Mock(),
+            'lvm': unittest.mock.Mock()
+        }
+
+        mock_drivers.return_value = ['driver1', 'driver2']
+        mock_parameters.side_effect = \
+            lambda driver: {
+                'driver1': ['param1', 'param2'],
+                'driver2': ['param3', 'param4']
+            }[driver]
+
+        self.app.add_pool = unittest.mock.Mock()
+
+        with self.assertRaises(AssertionError):
+            self.call_mgmt_func(b'mgmt.pool.Add', b'dom0',
+                b'driver1', b'name=file\nparam1=value\nparam2=some-value\n')
+        self.assertEqual(mock_drivers.mock_calls, [unittest.mock.call()])
+        self.assertEqual(mock_parameters.mock_calls, [])
+        self.assertEqual(self.app.add_pool.mock_calls, [])
+        self.assertFalse(self.app.save.called)
+
+    @unittest.mock.patch('qubes.storage.pool_drivers')
+    @unittest.mock.patch('qubes.storage.driver_parameters')
+    def test_160_pool_add_invalid_config_format(self, mock_parameters,
+            mock_drivers):
+        self.app.pools = {
+            'file': unittest.mock.Mock(),
+            'lvm': unittest.mock.Mock()
+        }
+
+        mock_drivers.return_value = ['driver1', 'driver2']
+        mock_parameters.side_effect = \
+            lambda driver: {
+                'driver1': ['param1', 'param2'],
+                'driver2': ['param3', 'param4']
+            }[driver]
+
+        self.app.add_pool = unittest.mock.Mock()
+
+        with self.assertRaises(AssertionError):
+            self.call_mgmt_func(b'mgmt.pool.Add', b'dom0',
+                b'driver1', b'name=test-pool\nparam 1=value\n_param2\n')
+        self.assertEqual(mock_drivers.mock_calls, [unittest.mock.call()])
+        self.assertEqual(mock_parameters.mock_calls, [])
+        self.assertEqual(self.app.add_pool.mock_calls, [])
+        self.assertFalse(self.app.save.called)
+
+    def test_170_pool_remove(self):
+        self.app.pools = {
+            'file': unittest.mock.Mock(),
+            'lvm': unittest.mock.Mock(),
+            'test-pool': unittest.mock.Mock(),
+        }
+        self.app.remove_pool = unittest.mock.Mock()
+        value = self.call_mgmt_func(b'mgmt.pool.Remove', b'dom0', b'test-pool')
+        self.assertIsNone(value)
+        self.assertEqual(self.app.remove_pool.mock_calls,
+            [unittest.mock.call('test-pool')])
+        self.assertTrue(self.app.save.called)
+
+    def test_170_pool_remove_invalid_pool(self):
+        self.app.pools = {
+            'file': unittest.mock.Mock(),
+            'lvm': unittest.mock.Mock(),
+            'test-pool': unittest.mock.Mock(),
+        }
+        self.app.remove_pool = unittest.mock.Mock()
+        with self.assertRaises(AssertionError):
+            self.call_mgmt_func(b'mgmt.pool.Remove', b'dom0',
+                b'no-such-pool')
+        self.assertEqual(self.app.remove_pool.mock_calls, [])
+        self.assertFalse(self.app.save.called)
+
+    def test_180_label_list(self):
+        value = self.call_mgmt_func(b'mgmt.label.List', b'dom0')
+        self.assertEqual(value,
+            ''.join('{}\n'.format(l.name) for l in self.app.labels.values()))
+        self.assertFalse(self.app.save.called)
+
+    def test_190_label_get(self):
+        self.app.get_label = unittest.mock.Mock()
+        self.app.get_label.configure_mock(**{'return_value.color': '0xff0000'})
+        value = self.call_mgmt_func(b'mgmt.label.Get', b'dom0', b'red')
+        self.assertEqual(value, '0xff0000')
+        self.assertEqual(self.app.get_label.mock_calls,
+            [unittest.mock.call('red')])
+        self.assertFalse(self.app.save.called)
+
+    def test_200_label_create(self):
+        self.app.get_label = unittest.mock.Mock()
+        self.app.get_label.side_effect=KeyError
+        self.app.labels = unittest.mock.MagicMock()
+        labels_config = {
+            'keys.return_value': range(1, 9),
+        }
+        self.app.labels.configure_mock(**labels_config)
+        value = self.call_mgmt_func(b'mgmt.label.Create', b'dom0', b'cyan',
+            b'0x00ffff')
+        self.assertIsNone(value)
+        self.assertEqual(self.app.get_label.mock_calls,
+            [unittest.mock.call('cyan')])
+        self.assertEqual(self.app.labels.mock_calls,
+            [unittest.mock.call.keys(),
+            unittest.mock.call.__getattr__('__setitem__')(9,
+                qubes.Label(9, '0x00ffff', 'cyan'))])
+        self.assertTrue(self.app.save.called)
+
+    def test_200_label_create_invalid_color(self):
+        self.app.get_label = unittest.mock.Mock()
+        self.app.get_label.side_effect=KeyError
+        self.app.labels = unittest.mock.MagicMock()
+        labels_config = {
+            'keys.return_value': range(1, 9),
+        }
+        self.app.labels.configure_mock(**labels_config)
+        with self.assertRaises(AssertionError):
+            self.call_mgmt_func(b'mgmt.label.Create', b'dom0', b'cyan',
+                b'abcd')
+        self.assertEqual(self.app.get_label.mock_calls,
+            [unittest.mock.call('cyan')])
+        self.assertEqual(self.app.labels.mock_calls, [])
+        self.assertFalse(self.app.save.called)
+
+    def test_200_label_create_invalid_name(self):
+        self.app.get_label = unittest.mock.Mock()
+        self.app.get_label.side_effect=KeyError
+        self.app.labels = unittest.mock.MagicMock()
+        labels_config = {
+            'keys.return_value': range(1, 9),
+        }
+        self.app.labels.configure_mock(**labels_config)
+        with self.assertRaises(AssertionError):
+            self.call_mgmt_func(b'mgmt.label.Create', b'dom0', b'01',
+                b'0xff0000')
+        with self.assertRaises(AssertionError):
+            self.call_mgmt_func(b'mgmt.label.Create', b'dom0', b'../xxx',
+                b'0xff0000')
+        with self.assertRaises(AssertionError):
+            self.call_mgmt_func(b'mgmt.label.Create', b'dom0',
+                b'strange-name!@#$',
+                b'0xff0000')
+
+        self.assertEqual(self.app.get_label.mock_calls, [])
+        self.assertEqual(self.app.labels.mock_calls, [])
+        self.assertFalse(self.app.save.called)
+
+    def test_200_label_create_already_exists(self):
+        self.app.get_label = unittest.mock.Mock(wraps=self.app.get_label)
+        with self.assertRaises(qubes.exc.QubesValueError):
+            self.call_mgmt_func(b'mgmt.label.Create', b'dom0', b'red',
+                b'abcd')
+        self.assertEqual(self.app.get_label.mock_calls,
+            [unittest.mock.call('red')])
+        self.assertFalse(self.app.save.called)
+
+    def test_210_label_remove(self):
+        label = qubes.Label(9, '0x00ffff', 'cyan')
+        self.app.labels[9] = label
+        self.app.get_label = unittest.mock.Mock(wraps=self.app.get_label,
+            **{'return_value.index': 9})
+        self.app.labels = unittest.mock.MagicMock(wraps=self.app.labels)
+        value = self.call_mgmt_func(b'mgmt.label.Remove', b'dom0', b'cyan')
+        self.assertIsNone(value)
+        self.assertEqual(self.app.get_label.mock_calls,
+            [unittest.mock.call('cyan')])
+        self.assertEqual(self.app.labels.mock_calls,
+            [unittest.mock.call.__delitem__(9)])
+        self.assertTrue(self.app.save.called)
+
+    def test_210_label_remove_invalid_label(self):
+        with self.assertRaises(qubes.exc.QubesValueError):
+            self.call_mgmt_func(b'mgmt.label.Remove', b'dom0',
+                b'no-such-label')
+        self.assertFalse(self.app.save.called)
+
+    def test_210_label_remove_default_label(self):
+        self.app.labels = unittest.mock.MagicMock(wraps=self.app.labels)
+        self.app.get_label = unittest.mock.Mock(wraps=self.app.get_label,
+            **{'return_value.index': 6})
+        with self.assertRaises(AssertionError):
+            self.call_mgmt_func(b'mgmt.label.Remove', b'dom0',
+                b'blue')
+        self.assertEqual(self.app.labels.mock_calls, [])
+        self.assertFalse(self.app.save.called)
+
+    def test_210_label_remove_in_use(self):
+        self.app.labels = unittest.mock.MagicMock(wraps=self.app.labels)
+        self.app.get_label = unittest.mock.Mock(wraps=self.app.get_label,
+            **{'return_value.index': 1})
+        with self.assertRaises(AssertionError):
+            self.call_mgmt_func(b'mgmt.label.Remove', b'dom0',
+                b'red')
+        self.assertEqual(self.app.labels.mock_calls, [])
+        self.assertFalse(self.app.save.called)
+
     def test_990_vm_unexpected_payload(self):
         methods_with_no_payload = [
             b'mgmt.vm.List',

From d21f54887df88857063157acccd7d1ee9b8c8b72 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?=
 <marmarek@invisiblethingslab.com>
Date: Thu, 16 Mar 2017 20:21:11 +0100
Subject: [PATCH 24/31] mgmt: add decorator for method calls not accepting
 payload

Fixes QubesOS/qubes-issues#2687
---
 qubes/mgmt.py | 85 ++++++++++++++++++++++++---------------------------
 1 file changed, 40 insertions(+), 45 deletions(-)

diff --git a/qubes/mgmt.py b/qubes/mgmt.py
index 8423f794..d76954b5 100644
--- a/qubes/mgmt.py
+++ b/qubes/mgmt.py
@@ -26,6 +26,8 @@ import asyncio
 import reprlib
 import string
 
+import functools
+
 import qubes.vm.qubesvm
 import qubes.storage
 
@@ -63,6 +65,14 @@ def not_in_api(func):
     func.not_in_api = True
     return func
 
+def no_payload(func):
+    @functools.wraps(func)
+    def wrapper(self, untrusted_payload):
+        if untrusted_payload != b'':
+            raise ProtocolError('unexpected payload')
+        return func(self)
+    return wrapper
+
 class QubesMgmt(object):
     def __init__(self, app, src, method, dest, arg):
         self.app = app
@@ -131,10 +141,9 @@ class QubesMgmt(object):
     #
 
     @asyncio.coroutine
-    def vm_list(self, untrusted_payload):
+    @no_payload
+    def vm_list(self):
         assert not self.arg
-        assert not untrusted_payload
-        del untrusted_payload
 
         if self.dest.name == 'dom0':
             domains = self.fire_event_for_filter(self.app.domains)
@@ -148,20 +157,18 @@ class QubesMgmt(object):
             for vm in sorted(domains))
 
     @asyncio.coroutine
-    def vm_property_list(self, untrusted_payload):
+    @no_payload
+    def vm_property_list(self):
         assert not self.arg
-        assert not untrusted_payload
-        del untrusted_payload
 
         properties = self.fire_event_for_filter(self.dest.property_list())
 
         return ''.join('{}\n'.format(prop.__name__) for prop in properties)
 
     @asyncio.coroutine
-    def vm_property_get(self, untrusted_payload):
+    @no_payload
+    def vm_property_get(self):
         assert self.arg in self.dest.property_list()
-        assert not untrusted_payload
-        del untrusted_payload
 
         self.fire_event_for_permission()
 
@@ -237,10 +244,9 @@ class QubesMgmt(object):
         self.app.save()
 
     @asyncio.coroutine
-    def vm_property_help(self, untrusted_payload):
+    @no_payload
+    def vm_property_help(self):
         assert self.arg in self.dest.property_list()
-        assert not untrusted_payload
-        del untrusted_payload
 
         self.fire_event_for_permission()
 
@@ -252,10 +258,9 @@ class QubesMgmt(object):
         return qubes.utils.format_doc(doc)
 
     @asyncio.coroutine
-    def vm_property_reset(self, untrusted_payload):
+    @no_payload
+    def vm_property_reset(self):
         assert self.arg in self.dest.property_list()
-        assert not untrusted_payload
-        del untrusted_payload
 
         self.fire_event_for_permission()
 
@@ -263,19 +268,17 @@ class QubesMgmt(object):
         self.app.save()
 
     @asyncio.coroutine
-    def vm_volume_list(self, untrusted_payload):
+    @no_payload
+    def vm_volume_list(self):
         assert not self.arg
-        assert not untrusted_payload
-        del untrusted_payload
 
         volume_names = self.fire_event_for_filter(self.dest.volumes.keys())
         return ''.join('{}\n'.format(name) for name in volume_names)
 
     @asyncio.coroutine
-    def vm_volume_info(self, untrusted_payload):
+    @no_payload
+    def vm_volume_info(self):
         assert self.arg in self.dest.volumes.keys()
-        assert not untrusted_payload
-        del untrusted_payload
 
         self.fire_event_for_permission()
 
@@ -288,10 +291,9 @@ class QubesMgmt(object):
             volume_properties)
 
     @asyncio.coroutine
-    def vm_volume_listsnapshots(self, untrusted_payload):
+    @no_payload
+    def vm_volume_listsnapshots(self):
         assert self.arg in self.dest.volumes.keys()
-        assert not untrusted_payload
-        del untrusted_payload
 
         self.fire_event_for_permission()
 
@@ -330,22 +332,20 @@ class QubesMgmt(object):
         self.app.save()
 
     @asyncio.coroutine
-    def pool_list(self, untrusted_payload):
+    @no_payload
+    def pool_list(self):
         assert not self.arg
         assert self.dest.name == 'dom0'
-        assert not untrusted_payload
-        del untrusted_payload
 
         pools = self.fire_event_for_filter(self.app.pools)
 
         return ''.join('{}\n'.format(pool) for pool in pools)
 
     @asyncio.coroutine
-    def pool_listdrivers(self, untrusted_payload):
+    @no_payload
+    def pool_listdrivers(self):
         assert self.dest.name == 'dom0'
         assert not self.arg
-        assert not untrusted_payload
-        del untrusted_payload
 
         drivers = self.fire_event_for_filter(qubes.storage.pool_drivers())
 
@@ -355,11 +355,10 @@ class QubesMgmt(object):
             for driver in drivers)
 
     @asyncio.coroutine
-    def pool_info(self, untrusted_payload):
+    @no_payload
+    def pool_info(self):
         assert self.dest.name == 'dom0'
         assert self.arg in self.app.pools.keys()
-        assert not untrusted_payload
-        del untrusted_payload
 
         pool = self.app.pools[self.arg]
 
@@ -405,11 +404,10 @@ class QubesMgmt(object):
         self.app.save()
 
     @asyncio.coroutine
-    def pool_remove(self, untrusted_payload):
+    @no_payload
+    def pool_remove(self):
         assert self.dest.name == 'dom0'
         assert self.arg in self.app.pools.keys()
-        assert not untrusted_payload
-        del untrusted_payload
 
         self.fire_event_for_permission()
 
@@ -417,21 +415,19 @@ class QubesMgmt(object):
         self.app.save()
 
     @asyncio.coroutine
-    def label_list(self, untrusted_payload):
+    @no_payload
+    def label_list(self):
         assert self.dest.name == 'dom0'
         assert not self.arg
-        assert not untrusted_payload
-        del untrusted_payload
 
         labels = self.fire_event_for_filter(self.app.labels.values())
 
         return ''.join('{}\n'.format(label.name) for label in labels)
 
     @asyncio.coroutine
-    def label_get(self, untrusted_payload):
+    @no_payload
+    def label_get(self):
         assert self.dest.name == 'dom0'
-        assert not untrusted_payload
-        del untrusted_payload
 
         try:
             label = self.app.get_label(self.arg)
@@ -478,10 +474,9 @@ class QubesMgmt(object):
         self.app.save()
 
     @asyncio.coroutine
-    def label_remove(self, untrusted_payload):
+    @no_payload
+    def label_remove(self):
         assert self.dest.name == 'dom0'
-        assert not untrusted_payload
-        del untrusted_payload
 
         try:
             label = self.app.get_label(self.arg)

From f93674de1acca91741fb833d33a6b3a1bcd65d2e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?=
 <marmarek@invisiblethingslab.com>
Date: Thu, 16 Mar 2017 20:29:13 +0100
Subject: [PATCH 25/31] mgmt: drop ProtocolRepr

Since we've added type= argument to property.Get format, it isn't
useful anymore.

QubesOS/qubes-issues#2622
---
 qubes/mgmt.py | 28 ++--------------------------
 1 file changed, 2 insertions(+), 26 deletions(-)

diff --git a/qubes/mgmt.py b/qubes/mgmt.py
index d76954b5..7c983e43 100644
--- a/qubes/mgmt.py
+++ b/qubes/mgmt.py
@@ -23,7 +23,6 @@ Qubes OS Management API
 '''
 
 import asyncio
-import reprlib
 import string
 
 import functools
@@ -32,26 +31,6 @@ import qubes.vm.qubesvm
 import qubes.storage
 
 
-class ProtocolRepr(reprlib.Repr):
-    def repr1(self, x, level):
-        if isinstance(x, qubes.vm.qubesvm.QubesVM):
-            x = x.name
-        return super().repr1(x, level)
-
-    # pylint: disable=invalid-name
-
-    def repr_str(self, x, level):
-        '''Warning: this is incompatible with python 3 wrt to b'' '''
-        return "'{}'".format(''.join(
-                chr(c)
-                if 0x20 < c < 0x7f and c not in (ord("'"), ord('\\'))
-                else '\\x{:02x}'.format(c)
-            for c in x.encode()))
-
-    def repr_Label(self, x, level):
-        return self.repr1(x.name, level)
-
-
 class ProtocolError(AssertionError):
     '''Raised when something is wrong with data received'''
     pass
@@ -65,6 +44,7 @@ def not_in_api(func):
     func.not_in_api = True
     return func
 
+
 def no_payload(func):
     @functools.wraps(func)
     def wrapper(self, untrusted_payload):
@@ -73,6 +53,7 @@ def no_payload(func):
         return func(self)
     return wrapper
 
+
 class QubesMgmt(object):
     def __init__(self, app, src, method, dest, arg):
         self.app = app
@@ -81,8 +62,6 @@ class QubesMgmt(object):
         self.dest = self.app.domains[dest.decode('ascii')]
         self.arg = arg.decode('ascii')
 
-        self.prepr = ProtocolRepr()
-
         self.method = method.decode('ascii')
 
         untrusted_func_name = self.method
@@ -132,9 +111,6 @@ class QubesMgmt(object):
             iterable = filter(selector, iterable)
         return iterable
 
-    @not_in_api
-    def repr(self, *args, **kwargs):
-        return self.prepr.repr(*args, **kwargs)
 
     #
     # ACTUAL RPC CALLS

From b225cefdf84b98e070269967cd82721a47c6620e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?=
 <marmarek@invisiblethingslab.com>
Date: Thu, 16 Mar 2017 20:35:56 +0100
Subject: [PATCH 26/31] tests: plug coverage into run-tests

It makes easier to check what functions lack unit tests.
---
 ci/coveragerc       | 3 +++
 ci/requirements.txt | 1 +
 run-tests           | 2 +-
 3 files changed, 5 insertions(+), 1 deletion(-)
 create mode 100644 ci/coveragerc

diff --git a/ci/coveragerc b/ci/coveragerc
new file mode 100644
index 00000000..2d47c77e
--- /dev/null
+++ b/ci/coveragerc
@@ -0,0 +1,3 @@
+[run]
+source = qubes
+omit = qubes/tests/*
diff --git a/ci/requirements.txt b/ci/requirements.txt
index a702822e..81325bdf 100644
--- a/ci/requirements.txt
+++ b/ci/requirements.txt
@@ -1,5 +1,6 @@
 # WARNING: those requirements are used only for travis-ci.org
 # they SHOULD NOT be used under normal conditions; use system package manager
+coverage
 docutils
 jinja2
 lxml
diff --git a/run-tests b/run-tests
index 85ed63ee..17471333 100755
--- a/run-tests
+++ b/run-tests
@@ -7,4 +7,4 @@ PYTHONPATH="${TESTPYTHONPATH}:${PYTHONPATH}"
 export PYTHONPATH
 
 "${PYTHON}" setup.py egg_info --egg-base "${TESTPYTHONPATH}"
-"${PYTHON}" -m qubes.tests.run "$@"
+"${PYTHON}" -m coverage run --rcfile=ci/coveragerc -m qubes.tests.run "$@"

From 6924ec3b85b735bbb86bc2513144d3439dfbeb8b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?=
 <marmarek@invisiblethingslab.com>
Date: Thu, 16 Mar 2017 20:36:41 +0100
Subject: [PATCH 27/31] tests: use python3 by default

QubesOS/qubes-issues#2074
---
 run-tests | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/run-tests b/run-tests
index 17471333..1d0706f6 100755
--- a/run-tests
+++ b/run-tests
@@ -1,6 +1,6 @@
 #!/bin/sh
 
-: "${PYTHON:=python}"
+: "${PYTHON:=python3}"
 : "${TESTPYTHONPATH:=test-packages}"
 
 PYTHONPATH="${TESTPYTHONPATH}:${PYTHONPATH}"

From 2c115bb64890eb9e57ffbc0af16a32e141939d5b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?=
 <marmarek@invisiblethingslab.com>
Date: Fri, 17 Mar 2017 12:49:28 +0100
Subject: [PATCH 28/31] mgmt: allow functions to return None if no data is to
 be returned

Don't force `return ''` everywhere.

QubesOS/qubes-issues#2622
---
 qubes/tools/qubesd.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/qubes/tools/qubesd.py b/qubes/tools/qubesd.py
index b72cd9ba..f91f6e5e 100644
--- a/qubes/tools/qubesd.py
+++ b/qubes/tools/qubesd.py
@@ -117,7 +117,8 @@ class QubesDaemonProtocol(asyncio.Protocol):
 
     def send_response(self, content):
         self.send_header(0x30)
-        self.transport.write(content.encode('utf-8'))
+        if content is not None:
+            self.transport.write(content.encode('utf-8'))
 
     def send_event(self, subject, event, **kwargs):
         self.send_header(0x31)

From ad456a338710fc291a69b5c6c17efa76b44ac8ac Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?=
 <marmarek@invisiblethingslab.com>
Date: Tue, 28 Mar 2017 15:25:10 +0200
Subject: [PATCH 29/31] mgmt: minor adjustments

per @woju review

QubesOS/qubes-issues#2622
---
 qubes/mgmt.py | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/qubes/mgmt.py b/qubes/mgmt.py
index 7c983e43..d825c871 100644
--- a/qubes/mgmt.py
+++ b/qubes/mgmt.py
@@ -271,10 +271,11 @@ class QubesMgmt(object):
     def vm_volume_listsnapshots(self):
         assert self.arg in self.dest.volumes.keys()
 
-        self.fire_event_for_permission()
-
         volume = self.dest.volumes[self.arg]
-        return ''.join('{}\n'.format(revision) for revision in volume.revisions)
+        revisions = [revision for revision in volume.revisions]
+        revisions = self.fire_event_for_filter(revisions)
+
+        return ''.join('{}\n'.format(revision) for revision in revisions)
 
     @asyncio.coroutine
     def vm_volume_revert(self, untrusted_payload):
@@ -369,13 +370,11 @@ class QubesMgmt(object):
 
         driver_parameters = qubes.storage.driver_parameters(self.arg)
         assert all(key in driver_parameters for key in untrusted_pool_config)
-
-        # option names validated, validation of option values is delegated to
-        #  extension (through events mechanism)
-        self.fire_event_for_permission(name=pool_name,
-            untrusted_pool_config=untrusted_pool_config)
         pool_config = untrusted_pool_config
 
+        self.fire_event_for_permission(name=pool_name,
+            pool_config=pool_config)
+
         self.app.add_pool(name=pool_name, driver=self.arg, **pool_config)
         self.app.save()
 
@@ -436,7 +435,7 @@ class QubesMgmt(object):
         # besides prefix, only hex digits are allowed
         assert all(x in string.hexdigits for x in untrusted_payload[2:])
 
-        # TODO: try to avoid creating label too similar to existing one?
+        # SEE: #2732
         color = untrusted_payload
 
         self.fire_event_for_permission(color=color)

From ce3bedbf2c9c8194cb4305ae4f5ccb607e3f9615 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?=
 <marmarek@invisiblethingslab.com>
Date: Tue, 28 Mar 2017 20:42:10 +0200
Subject: [PATCH 30/31] vm: move validate_name to qubes/vm

This will be needed by VMProperty class in the next commit.

QubesOS/qubes-issues#2622
---
 qubes/vm/__init__.py | 26 ++++++++++++++++++++++++++
 qubes/vm/qubesvm.py  | 28 +---------------------------
 2 files changed, 27 insertions(+), 27 deletions(-)

diff --git a/qubes/vm/__init__.py b/qubes/vm/__init__.py
index 50879a06..ac9ffd08 100644
--- a/qubes/vm/__init__.py
+++ b/qubes/vm/__init__.py
@@ -27,6 +27,7 @@
 
 import datetime
 import os
+import re
 import subprocess
 import sys
 import xml.parsers.expat
@@ -40,6 +41,31 @@ import qubes.log
 import qubes.tools.qvm_ls
 
 
+def validate_name(holder, prop, value):
+    ''' Check if value is syntactically correct VM name '''
+    if not isinstance(value, str):
+        raise TypeError('VM name must be string, {!r} found'.format(
+            type(value).__name__))
+    if len(value) > 31:
+        if holder is not None and prop is not None:
+            raise qubes.exc.QubesPropertyValueError(holder, prop, value,
+                '{} value must be shorter than 32 characters'.format(
+                    prop.__name__))
+        else:
+            raise qubes.exc.QubesValueError(
+                'VM name must be shorter than 32 characters')
+
+    # this regexp does not contain '+'; if it had it, we should specifically
+    # disallow 'lost+found' #1440
+    if re.match(r"^[a-zA-Z][a-zA-Z0-9_-]*$", value) is None:
+        if holder is not None and prop is not None:
+            raise qubes.exc.QubesPropertyValueError(holder, prop, value,
+                '{} value contains illegal characters'.format(prop.__name__))
+        else:
+            raise qubes.exc.QubesValueError(
+                'VM name contains illegal characters')
+
+
 class Features(dict):
     '''Manager of the features.
 
diff --git a/qubes/vm/qubesvm.py b/qubes/vm/qubesvm.py
index 888d3fae..eb53f9a6 100644
--- a/qubes/vm/qubesvm.py
+++ b/qubes/vm/qubesvm.py
@@ -28,7 +28,6 @@ import base64
 import datetime
 import os
 import os.path
-import re
 import shutil
 import string
 import subprocess
@@ -76,34 +75,9 @@ def _setter_qid(self, prop, value):
     return value
 
 
-def validate_name(holder, prop, value):
-    ''' Check if value is syntactically correct VM name '''
-    if not isinstance(value, str):
-        raise TypeError('VM name must be string, {!r} found'.format(
-            type(value).__name__))
-    if len(value) > 31:
-        if holder is not None and prop is not None:
-            raise qubes.exc.QubesPropertyValueError(holder, prop, value,
-                '{} value must be shorter than 32 characters'.format(
-                    prop.__name__))
-        else:
-            raise qubes.exc.QubesValueError(
-                'VM name must be shorter than 32 characters')
-
-    # this regexp does not contain '+'; if it had it, we should specifically
-    # disallow 'lost+found' #1440
-    if re.match(r"^[a-zA-Z][a-zA-Z0-9_-]*$", value) is None:
-        if holder is not None and prop is not None:
-            raise qubes.exc.QubesPropertyValueError(holder, prop, value,
-                '{} value contains illegal characters'.format(prop.__name__))
-        else:
-            raise qubes.exc.QubesValueError(
-                'VM name contains illegal characters')
-
-
 def _setter_name(self, prop, value):
     ''' Helper for setting the domain name '''
-    validate_name(self, prop, value)
+    qubes.vm.validate_name(self, prop, value)
 
     if self.is_running():
         raise qubes.exc.QubesVMNotHaltedError(

From 3388054eae008891c961bd9b01964a02af778329 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?=
 <marmarek@invisiblethingslab.com>
Date: Tue, 28 Mar 2017 20:43:53 +0200
Subject: [PATCH 31/31] mgmt: move property value sanitization to property
 definition

This also means we don't check if a VM with given name (in case of
VMProperty) exists in the system, at this stage. But this is ok, lets
not duplicate work of property setter.

QubesOS/qubes-issues#2622
---
 qubes/__init__.py    | 36 ++++++++++++++++++++++++++++++++++++
 qubes/mgmt.py        | 38 +-------------------------------------
 qubes/tests/mgmt.py  | 14 +++-----------
 qubes/vm/__init__.py |  8 ++++++++
 4 files changed, 48 insertions(+), 48 deletions(-)

diff --git a/qubes/__init__.py b/qubes/__init__.py
index f110a4d9..1a5859df 100644
--- a/qubes/__init__.py
+++ b/qubes/__init__.py
@@ -31,6 +31,7 @@ import builtins
 import collections
 import os
 import os.path
+import string
 
 import lxml.etree
 import qubes.config
@@ -321,6 +322,41 @@ class property(object): # pylint: disable=redefined-builtin,invalid-name
                 'property {!r} is write-once and already set'.format(
                     self.__name__))
 
+    def sanitize(self, *, untrusted_newvalue):
+        '''Coarse sanitization of value to be set, before sending it to a
+        setter. Can raise QubesValueError if the value is invalid.
+
+        :param untrusted_newvalue: value to be validated
+        :return sanitized value
+        :raises qubes.exc.QubesValueError
+        '''
+        # do not treat type='str' as sufficient validation
+        if self.type is not None and self.type is not str:
+            # assume specific type will preform enough validation
+            if self.type is bool:
+                try:
+                    untrusted_newvalue = untrusted_newvalue.decode('ascii')
+                except UnicodeDecodeError:
+                    raise qubes.exc.QubesValueError
+                return self.bool(None, None, untrusted_newvalue)
+            else:
+                try:
+                    return self.type(untrusted_newvalue)
+                except ValueError:
+                    raise qubes.exc.QubesValueError
+        else:
+            # 'str' or not specified type
+            try:
+                untrusted_newvalue = untrusted_newvalue.decode('ascii',
+                    errors='strict')
+            except UnicodeDecodeError:
+                raise qubes.exc.QubesValueError
+            allowed_set = string.printable
+            if not all(x in allowed_set for x in untrusted_newvalue):
+                raise qubes.exc.QubesValueError(
+                    'Invalid characters in property value')
+            return untrusted_newvalue
+
 
     #
     # exceptions
diff --git a/qubes/mgmt.py b/qubes/mgmt.py
index d825c871..c9d5cff7 100644
--- a/qubes/mgmt.py
+++ b/qubes/mgmt.py
@@ -176,43 +176,7 @@ class QubesMgmt(object):
         assert self.arg in self.dest.property_list()
 
         property_def = self.dest.property_get_def(self.arg)
-        # do not treat type='str' as sufficient validation
-        if isinstance(property_def, qubes.vm.VMProperty):
-            try:
-                untrusted_vmname = untrusted_payload.decode('ascii')
-            except UnicodeDecodeError:
-                raise qubes.exc.QubesValueError
-            qubes.vm.qubesvm.validate_name(self.dest, property_def,
-                untrusted_vmname)
-            try:
-                newvalue = self.app.domains[untrusted_vmname]
-            except KeyError:
-                raise qubes.exc.QubesPropertyValueError(
-                    self.dest, property_def, untrusted_vmname)
-        elif property_def.type is not None and property_def.type is not str:
-            if property_def.type is bool:
-                try:
-                    untrusted_value = untrusted_payload.decode('ascii')
-                except UnicodeDecodeError:
-                    raise qubes.exc.QubesValueError
-                newvalue = qubes.property.bool(None, None, untrusted_value)
-            else:
-                try:
-                    newvalue = property_def.type(untrusted_payload)
-                except ValueError:
-                    raise qubes.exc.QubesValueError
-        else:
-            # other types (including explicitly defined 'str') coerce to str
-            # with limited characters allowed
-            try:
-                untrusted_value = untrusted_payload.decode('ascii')
-            except UnicodeDecodeError:
-                raise qubes.exc.QubesValueError
-            allowed_set = string.printable
-            if not all(x in allowed_set for x in untrusted_value):
-                raise qubes.exc.QubesValueError(
-                    'Invalid characters in property value')
-            newvalue = untrusted_value
+        newvalue = property_def.sanitize(untrusted_newvalue=untrusted_payload)
 
         self.fire_event_for_permission(newvalue=newvalue)
 
diff --git a/qubes/tests/mgmt.py b/qubes/tests/mgmt.py
index f154d257..d55f907a 100644
--- a/qubes/tests/mgmt.py
+++ b/qubes/tests/mgmt.py
@@ -130,18 +130,10 @@ class TC_00_VMs(MgmtTestCase):
             value = self.call_mgmt_func(b'mgmt.vm.property.Set', b'test-vm1',
                 b'netvm', b'test-net')
             self.assertIsNone(value)
-            mock.assert_called_once_with(self.vm, netvm)
+            mock.assert_called_once_with(self.vm, 'test-net')
         self.app.save.assert_called_once_with()
 
-    def test_031_vm_property_set_vm_invalid1(self):
-        with unittest.mock.patch('qubes.vm.VMProperty.__set__') as mock:
-            with self.assertRaises(qubes.exc.QubesValueError):
-                self.call_mgmt_func(b'mgmt.vm.property.Set', b'test-vm1',
-                    b'netvm', b'no-such-vm')
-            self.assertFalse(mock.called)
-        self.assertFalse(self.app.save.called)
-
-    def test_032_vm_property_set_vm_invalid2(self):
+    def test_032_vm_property_set_vm_invalid1(self):
         with unittest.mock.patch('qubes.vm.VMProperty.__set__') as mock:
             with self.assertRaises(qubes.exc.QubesValueError):
                 self.call_mgmt_func(b'mgmt.vm.property.Set', b'test-vm1',
@@ -149,7 +141,7 @@ class TC_00_VMs(MgmtTestCase):
             self.assertFalse(mock.called)
         self.assertFalse(self.app.save.called)
 
-    def test_033_vm_property_set_vm_invalid3(self):
+    def test_033_vm_property_set_vm_invalid2(self):
         with unittest.mock.patch('qubes.vm.VMProperty.__set__') as mock:
             with self.assertRaises(qubes.exc.QubesValueError):
                 self.call_mgmt_func(b'mgmt.vm.property.Set', b'test-vm1',
diff --git a/qubes/vm/__init__.py b/qubes/vm/__init__.py
index ac9ffd08..524d5000 100644
--- a/qubes/vm/__init__.py
+++ b/qubes/vm/__init__.py
@@ -358,3 +358,11 @@ class VMProperty(qubes.property):
                     self.vmclass.__name__))
 
         super(VMProperty, self).__set__(instance, vm)
+
+    def sanitize(self, *, untrusted_newvalue):
+        try:
+            untrusted_vmname = untrusted_newvalue.decode('ascii')
+        except UnicodeDecodeError:
+            raise qubes.exc.QubesValueError
+        validate_name(None, self, untrusted_vmname)
+        return untrusted_vmname