uitests: Finish addstorage.py coverage

We need to wire up some craziness to make path permission
searching fail, but this is a critical area to get correct
so it is worth it

Signed-off-by: Cole Robinson <crobinso@redhat.com>

tests/uitests/data/capabilities/dac-caps-template.xml

@@ -0,0 +1,89 @@
+<capabilities>
+
+  <host>
+    <cpu>
+      <arch>i686</arch>
+      <features>
+        <pae/>
+        <nonpae/>
+      </features>
+      <pages unit='KiB' size='4'/>
+      <pages unit='KiB' size='8'/>
+      <pages unit='KiB' size='2048'/>
+      <pages unit='KiB' size='1048576'/>
+    </cpu>
+    <power_management/>
+    <iommu support='yes'/>
+    <topology>
+      <cells num='2'>
+        <cell id='0'>
+          <memory unit='KiB'>2097152</memory>
+          <pages unit='KiB' size='4'>524288</pages>
+          <pages unit='KiB' size='2048'>0</pages>
+          <pages unit='KiB' size='1048576'>0</pages>
+          <cpus num='8'>
+            <cpu id='0' socket_id='0' die_id='0' core_id='0' siblings='0'/>
+            <cpu id='1' socket_id='0' die_id='0' core_id='1' siblings='1'/>
+            <cpu id='2' socket_id='0' die_id='0' core_id='2' siblings='2'/>
+            <cpu id='3' socket_id='0' die_id='0' core_id='3' siblings='3'/>
+            <cpu id='4' socket_id='0' die_id='0' core_id='4' siblings='4'/>
+            <cpu id='5' socket_id='0' die_id='0' core_id='5' siblings='5'/>
+            <cpu id='6' socket_id='0' die_id='0' core_id='6' siblings='6'/>
+            <cpu id='7' socket_id='0' die_id='0' core_id='7' siblings='7'/>
+          </cpus>
+        </cell>
+        <cell id='1'>
+          <memory unit='KiB'>4194304</memory>
+          <pages unit='KiB' size='8'>524288</pages>
+          <pages unit='KiB' size='2048'>0</pages>
+          <pages unit='KiB' size='1048576'>0</pages>
+          <cpus num='8'>
+            <cpu id='8' socket_id='1' die_id='0' core_id='0' siblings='8'/>
+            <cpu id='9' socket_id='1' die_id='0' core_id='1' siblings='9'/>
+            <cpu id='10' socket_id='1' die_id='0' core_id='2' siblings='10'/>
+            <cpu id='11' socket_id='1' die_id='0' core_id='3' siblings='11'/>
+            <cpu id='12' socket_id='1' die_id='0' core_id='4' siblings='12'/>
+            <cpu id='13' socket_id='1' die_id='0' core_id='5' siblings='13'/>
+            <cpu id='14' socket_id='1' die_id='0' core_id='6' siblings='14'/>
+            <cpu id='15' socket_id='1' die_id='0' core_id='7' siblings='15'/>
+          </cpus>
+        </cell>
+      </cells>
+    </topology>
+    <secmodel>
+      <model>dac</model>
+      <doi>0</doi>
+      <baselabel type='kvm'>+%(UID)s:+%(GID)s</baselabel>
+      <baselabel type='qemu'>+%(UID)s:+%(GID)s</baselabel>
+    </secmodel>
+  </host>
+
+  <guest>
+    <os_type>hvm</os_type>
+    <arch name='i686'>
+      <wordsize>32</wordsize>
+      <emulator>/usr/bin/test-hv</emulator>
+      <domain type='test'/>
+    </arch>
+    <features>
+      <pae/>
+      <nonpae/>
+    </features>
+  </guest>
+
+  <guest>
+    <os_type>xen</os_type>
+    <arch name='i686'>
+      <wordsize>32</wordsize>
+      <emulator>/usr/bin/test-hv</emulator>
+      <domain type='test'/>
+    </arch>
+    <features>
+      <pae/>
+      <nonpae/>
+    </features>
+  </guest>
+
+</capabilities>
+
+

tests/uitests/test_addhardware.py

@@ -2,11 +2,44 @@
 # See the COPYING file in the top-level directory.
 
 import os
+import tempfile
 
 import tests
 from tests.uitests import utils as uiutils
 
 
+def _search_permissions_decorator(fn):
+    """
+    Decorator to set up necessary bits to test disk permission search
+    """
+    def wrapper(self, *args, **kwargs):
+        # Generate capabilities XML from a template, with out
+        # UID/GID inserted as the intended emulator permissions
+        capsfile = (os.path.dirname(__file__) +
+                "/data/capabilities/dac-caps-template.xml")
+        capsdata = open(capsfile).read() % {
+                "UID": os.getuid(), "GID": os.getgid()}
+        tmpcaps = tempfile.NamedTemporaryFile(
+                prefix="virt-manager-uitests-caps")
+        tmpcapspath = tmpcaps.name
+        open(tmpcapspath, "w").write(capsdata)
+
+        # We mock a qemu URI to trigger the permissions check
+        uri = (tests.utils.URIs.test_full +
+                ",fakeuri=qemu:///system,caps=%s" % tmpcapspath)
+
+        # Create a temporary directory that we can manipulate perms
+        tmpobj = tempfile.TemporaryDirectory(
+                prefix="virtinst-test-search")
+        tmpdir = tmpobj.name
+        try:
+            os.chmod(tmpdir, 0o000)
+            fn(self, uri, tmpdir, *args, **kwargs)
+        finally:
+            os.chmod(tmpdir, 0o777)
+    return wrapper
+
+
 class AddHardware(uiutils.UITestCase):
     """
     UI tests for virt-manager's VM addhardware window
@@ -122,12 +155,16 @@ class AddHardware(uiutils.UITestCase):
         # Disk with some tweaks
         addhw = self._open_addhw_window(details)
         tab = self._select_hw(addhw, "Storage", "storage-tab")
-        tab.find("GiB", "spin button").text = "1.5"
         tab.find("Bus type:", "combo box").click()
         tab.find("VirtIO", "menu item").click()
         tab.find("Advanced options", "toggle button").click_expander()
         tab.find("Cache mode:", "combo box").click()
         tab.find("none", "menu item").click()
+        # Size too big
+        tab.find("GiB", "spin button").text = "2000"
+        finish.click()
+        self._click_alert_button("not enough free space", "Close")
+        tab.find("GiB", "spin button").text = "1.5"
         finish.click()
         uiutils.check_in_loop(lambda: details.active)
 
@@ -135,6 +172,8 @@ class AddHardware(uiutils.UITestCase):
         addhw = self._open_addhw_window(details)
         tab = self._select_hw(addhw, "Storage", "storage-tab")
         tab.find_fuzzy("Select or create", "radio").click()
+        finish.click()
+        self._click_alert_button("storage path must be specified", "OK")
         tab.find("storage-browse", "push button").click()
         browse = self.app.root.find("vmm-storage-browser")
 
@@ -179,6 +218,8 @@ class AddHardware(uiutils.UITestCase):
         browse.find("Choose Volume", "push button").click()
         self.assertTrue("/diskvol1" in tab.find("storage-entry").text)
         finish.click()
+        self._click_alert_button("already in use by", "No")
+        finish.click()
         self._click_alert_button("already in use by", "Yes")
         uiutils.check_in_loop(lambda: details.active)
 
@@ -204,6 +245,106 @@ class AddHardware(uiutils.UITestCase):
         finish.click()
         uiutils.check_in_loop(lambda: details.active)
 
+    @_search_permissions_decorator
+    def testAddDiskSearchPermsCheckbox(self, uri, tmpdir):
+        """
+        Test search permissions 'no' and checkbox case
+        """
+        self.app.uri = uri
+        details = self._open_details_window()
+
+        # Say 'No' but path should still work due to test driver
+        addhw = self._open_addhw_window(details)
+        finish = addhw.find("Finish", "push button")
+        tab = self._select_hw(addhw, "Storage", "storage-tab")
+        tab.find_fuzzy("Select or create", "radio").click()
+        path = tmpdir + "/foo1.img"
+        tab.find("storage-entry").text = path
+        finish.click()
+        self._click_alert_button("emulator may not have", "No")
+        uiutils.check_in_loop(lambda: details.active)
+
+        # Say 'don't ask again'
+        addhw = self._open_addhw_window(details)
+        tab = self._select_hw(addhw, "Storage", "storage-tab")
+        tab.find_fuzzy("Select or create", "radio").click()
+        path = tmpdir + "/foo2.img"
+        tab.find("storage-entry").text = path
+        finish.click()
+        alert = self.app.root.find_fuzzy("vmm dialog", "alert")
+        alert.find_fuzzy("Don't ask", "check box").click()
+        self._click_alert_button("emulator may not have", "No")
+        uiutils.check_in_loop(lambda: details.active)
+
+        # Confirm it doesn't ask about path again
+        addhw = self._open_addhw_window(details)
+        tab = self._select_hw(addhw, "Storage", "storage-tab")
+        tab.find_fuzzy("Select or create", "radio").click()
+        path = tmpdir + "/foo3.img"
+        tab.find("storage-entry").text = path
+        finish.click()
+        uiutils.check_in_loop(lambda: details.active)
+
+    @_search_permissions_decorator
+    def testAddDiskSearchPermsSuccess(self, uri, tmpdir):
+        """
+        Select 'Yes' for search perms fixing
+        """
+        self.app.uri = uri
+        details = self._open_details_window()
+
+        # Say 'Yes'
+        addhw = self._open_addhw_window(details)
+        finish = addhw.find("Finish", "push button")
+        tab = self._select_hw(addhw, "Storage", "storage-tab")
+        tab.find_fuzzy("Select or create", "radio").click()
+        path = tmpdir + "/foo1.img"
+        tab.find("storage-entry").text = path
+        finish.click()
+        self._click_alert_button("emulator may not have", "Yes")
+        uiutils.check_in_loop(lambda: details.active)
+
+        # Confirm it doesn't ask about path again
+        addhw = self._open_addhw_window(details)
+        tab = self._select_hw(addhw, "Storage", "storage-tab")
+        tab.find_fuzzy("Select or create", "radio").click()
+        path = tmpdir + "/foo3.img"
+        tab.find("storage-entry").text = path
+        finish.click()
+        uiutils.check_in_loop(lambda: details.active)
+
+    @_search_permissions_decorator
+    def testAddDiskSearchPermsFail(self, uri, tmpdir):
+        """
+        Force perms fixing to fail
+        """
+        self.app.uri = uri
+        self.app.open(break_setfacl=True)
+        details = self._open_details_window()
+
+        # Say 'Yes' and it should fail, then blacklist the paths
+        addhw = self._open_addhw_window(details)
+        finish = addhw.find("Finish", "push button")
+        tab = self._select_hw(addhw, "Storage", "storage-tab")
+        tab.find_fuzzy("Select or create", "radio").click()
+        path = tmpdir + "/foo1.img"
+        tab.find("storage-entry").text = path
+        finish.click()
+        self._click_alert_button("emulator may not have", "Yes")
+        alert = self.app.root.find("vmm dialog", "alert")
+        alert.find_fuzzy("Errors were encountered", "label")
+        alert.find_fuzzy("Don't ask", "check box").click()
+        alert.find_fuzzy("OK", "push button").click()
+        uiutils.check_in_loop(lambda: details.active)
+
+        # Confirm it doesn't ask about path again
+        addhw = self._open_addhw_window(details)
+        tab = self._select_hw(addhw, "Storage", "storage-tab")
+        tab.find_fuzzy("Select or create", "radio").click()
+        path = tmpdir + "/foo2.img"
+        tab.find("storage-entry").text = path
+        finish.click()
+        uiutils.check_in_loop(lambda: details.active)
 
     def testAddNetworks(self):
         """

tests/uitests/utils.py

@@ -455,7 +455,8 @@ class VMMDogtailApp(object):
         return bool(self._proc and self._proc.poll() is None)
 
     def open(self, extra_opts=None, check_already_running=True, use_uri=True,
-            window_name=None, xmleditor_enabled=False, keyfile=None):
+            window_name=None, xmleditor_enabled=False, keyfile=None,
+            break_setfacl=False):
         extra_opts = extra_opts or []
 
         if tests.utils.TESTCONFIG.debug:
@@ -476,6 +477,8 @@ class VMMDogtailApp(object):
         testoptions = []
         if xmleditor_enabled:
             testoptions.append("xmleditor-enabled")
+        if break_setfacl:
+            testoptions.append("break-setfacl")
         if keyfile:
             import atexit
             import tempfile

virtManager/addhardware.py

@@ -1428,8 +1428,6 @@ class vmmAddHardware(vmmGObjectUI):
                 return False
 
     def _validate_device(self, dev):
-        dev.validate()
-
         if dev.DEVICE_TYPE == "disk":
             if self.addstorage.validate_device(dev) is False:
                 return False
@@ -1440,6 +1438,8 @@ class vmmAddHardware(vmmGObjectUI):
         if dev.DEVICE_TYPE == "hostdev":
             self._validate_hostdev_collision(dev)
 
+        dev.validate()
+
     def _build_xmleditor_device(self, srcdev):
         xml = self._xmleditor.get_xml()
         log.debug("Using XML from xmleditor:\n%s", xml)

virtManager/device/addstorage.py

@@ -49,7 +49,7 @@ class vmmAddStorage(vmmGObjectUI):
                     pool.refresh()
                 avail = int(pool.get_available())
                 return float(avail / 1024.0 / 1024.0 / 1024.0)
-        except Exception:
+        except Exception:  # pragma: no cover
             log.exception("Error determining host disk space")
         return -1
 
@@ -108,9 +108,8 @@ class vmmAddStorage(vmmGObjectUI):
                    "following directories:")
         details = ""
         for p, error in errors.items():
-            if p not in broken_paths:
+            if p in broken_paths:
-                continue
+                details += "%s : %s\n" % (p, error)
-            details += "%s : %s\n" % (p, error)
         details += "\nIt is very likely the VM will fail to start up."
 
         log.debug("Permission errors:\n%s", details)
@@ -136,7 +135,7 @@ class vmmAddStorage(vmmGObjectUI):
         storage_area = self.widget("storage-box")
 
         storage_area.set_sensitive(can_storage)
-        if not can_storage:
+        if not can_storage:  # pragma: no cover
             storage_tooltip = _("Connection does not support storage"
                                 " management.")
             use_storage.set_sensitive(True)
@@ -145,7 +144,7 @@ class vmmAddStorage(vmmGObjectUI):
     def get_default_path(self, name, collideguest=None):
         pool = self.conn.get_default_pool()
         if not pool:
-            return
+            return  # pragma: no cover
 
         fmt = self.conn.get_default_storage_format()
         suffix = virtinst.StorageVolume.get_file_extension_for_format(fmt)
@@ -189,7 +188,7 @@ class vmmAddStorage(vmmGObjectUI):
                 disk.get_vol_install().format = fmt
             else:
                 log.debug("path=%s can not use default prefs format=%s, "
-                    "not setting it", disk.path, fmt)
+                        "not setting it", disk.path, fmt)  # pragma: no cover
 
         return disk
 
@@ -199,13 +198,6 @@ class vmmAddStorage(vmmGObjectUI):
 
         disk.validate()
 
-        isfatal, errmsg = disk.is_size_conflict()
-        if not isfatal and errmsg:
-            # Fatal errors are reported when setting 'size'
-            res = self.err.ok_cancel(_("Not Enough Free Space"), errmsg)
-            if not res:
-                return False
-
         # Disk collision
         names = disk.is_conflict_disk()
         if names:

virtManager/virtmanager.py

@@ -211,6 +211,7 @@ class CLITestOptionsClass:
         self.no_events = _get("no-events")
         self.xmleditor_enabled = _get("xmleditor-enabled")
         self.gsettings_keyfile = _get_value("gsettings-keyfile")
+        self.break_setfacl = _get("break-setfacl")
 
         if opts:
             print("Unknown --test-options keys: %s" % opts)
@@ -251,6 +252,14 @@ def main():
     if options.test_no_events:
         CLITestOptions.no_events = True
 
+    if CLITestOptions.break_setfacl:
+        import virtinst.diskbackend
+        def fake_search(*args, **kwargs):
+            raise RuntimeError("Fake search fix fail from test suite")
+        virtinst.diskbackend.SETFACL = "getfacl"
+        # pylint: disable=protected-access
+        virtinst.diskbackend._fix_perms_chmod = fake_search
+
     # With F27 gnome+wayland we need to set these before GTK import
     os.environ["GSETTINGS_SCHEMA_DIR"] = BuildConfig.gsettings_dir
     if CLITestOptions.first_run: