prefs: Add a setting to enable/disable libguestfs inspection

Rather than key it on the library being available. Makes it much
easier to test both modes of behavior.

Fix up a few inspection bugs while I'm in the area, and convert
it to be more singleton like.

7 files changed, 273 insertions, 130 deletions

diff --git a/data/org.virt-manager.virt-manager.gschema.xml b/data/org.virt-manager.virt-manager.gschema.xml
index d88450f5..f6db721b 100644
--- a/data/org.virt-manager.virt-manager.gschema.xml
+++ b/data/org.virt-manager.virt-manager.gschema.xml
@@ -47,6 +47,12 @@
       <description>Show system tray icon while app is running</description>
     </key>
 
+    <key name="enable-libguestfs-vm-inspection" type="b">
+      <default>true</default>
+      <summary>Enable libguestfs VM inspection</summary>
+      <description>Enable libguestfs VM inspection for things like OS icons, installed applications, etc. This only works if python libguestfs bindings are installed.</description>
+    </key>
+
     <key name="manager-window-height" type="i">
       <default>0</default>
       <summary>Default manager window height</summary>
diff --git a/ui/preferences.ui b/ui/preferences.ui
index 81802a28..01b54602 100644
--- a/ui/preferences.ui
+++ b/ui/preferences.ui
@@ -1,5 +1,5 @@
 <?xml version="1.0" encoding="UTF-8"?>
-<!-- Generated with glade 3.20.2 -->
+<!-- Generated with glade 3.20.3 -->
 <interface>
   <requires lib="gtk+" version="3.14"/>
   <object class="GtkAdjustment" id="adjustment1">
@@ -49,6 +49,7 @@
                             <property name="visible">True</property>
                             <property name="can_focus">True</property>
                             <property name="receives_default">False</property>
+                            <property name="halign">start</property>
                             <property name="use_underline">True</property>
                             <property name="draw_indicator">True</property>
                             <signal name="toggled" handler="on_prefs_system_tray_toggled" swapped="no"/>
@@ -58,6 +59,68 @@
                             <property name="top_attach">0</property>
                           </packing>
                         </child>
+                        <child>
+                          <object class="GtkGrid">
+                            <property name="visible">True</property>
+                            <property name="can_focus">False</property>
+                            <property name="row_spacing">3</property>
+                            <child>
+                              <object class="GtkCheckButton" id="prefs-libguestfs">
+                                <property name="label" translatable="yes">Enable libgues_tfs VM introspection</property>
+                                <property name="visible">True</property>
+                                <property name="can_focus">True</property>
+                                <property name="receives_default">False</property>
+                                <property name="halign">start</property>
+                                <property name="use_underline">True</property>
+                                <property name="draw_indicator">True</property>
+                                <signal name="toggled" handler="on_prefs_libguestfs_toggled" swapped="no"/>
+                              </object>
+                              <packing>
+                                <property name="left_attach">0</property>
+                                <property name="top_attach">0</property>
+                              </packing>
+                            </child>
+                            <child>
+                              <object class="GtkBox" id="prefs-libguestfs-warn-box">
+                                <property name="can_focus">False</property>
+                                <property name="spacing">3</property>
+                                <child>
+                                  <object class="GtkImage">
+                                    <property name="visible">True</property>
+                                    <property name="can_focus">False</property>
+                                    <property name="stock">gtk-dialog-warning</property>
+                                  </object>
+                                  <packing>
+                                    <property name="expand">False</property>
+                                    <property name="fill">True</property>
+                                    <property name="position">0</property>
+                                  </packing>
+                                </child>
+                                <child>
+                                  <object class="GtkLabel" id="prefs-libguestfs-warn-label">
+                                    <property name="visible">True</property>
+                                    <property name="can_focus">False</property>
+                                    <property name="label">&lt;small&gt;You must restart the application for this change to take effect&lt;/small&gt;</property>
+                                    <property name="use_markup">True</property>
+                                  </object>
+                                  <packing>
+                                    <property name="expand">False</property>
+                                    <property name="fill">True</property>
+                                    <property name="position">1</property>
+                                  </packing>
+                                </child>
+                              </object>
+                              <packing>
+                                <property name="left_attach">0</property>
+                                <property name="top_attach">1</property>
+                              </packing>
+                            </child>
+                          </object>
+                          <packing>
+                            <property name="left_attach">0</property>
+                            <property name="top_attach">1</property>
+                          </packing>
+                        </child>
                       </object>
                     </child>
                   </object>
diff --git a/virtManager/config.py b/virtManager/config.py
index de66f80c..c7d3f082 100644
--- a/virtManager/config.py
+++ b/virtManager/config.py
@@ -25,6 +25,8 @@ from gi.repository import GLib
 from gi.repository import Gtk
 
 from virtinst import CPU
+
+from .inspection import vmmInspection
 from .keyring import vmmKeyring, vmmSecret
 
 
@@ -215,19 +217,6 @@ class vmmConfig(object):
 
         self._objects = []
 
-        self.support_inspection = self.check_inspection()
-
-        self._spice_error = None
-
-
-    def check_inspection(self):
-        try:
-            # Check we can open the Python guestfs module.
-            from guestfs import GuestFS  # pylint: disable=import-error
-            g = GuestFS(close_on_exit=False)
-            return bool(getattr(g, "add_libvirt_dom", None))
-        except Exception:
-            return False
 
     # General app wide helpers (gsettings agnostic)
 
@@ -242,6 +231,11 @@ class vmmConfig(object):
         ret = ["vnc", "spice"]
         return ret
 
+    def inspection_supported(self):
+        if not vmmInspection.libguestfs_installed():
+            return False
+        return self.get_libguestfs_inspect_vms()
+
     def remove_notifier(self, h):
         self.conf.notify_remove(h)
 
@@ -379,7 +373,6 @@ class vmmConfig(object):
     def get_confirm_delstorage(self):
         return self.conf.get("/confirm/delete-storage")
 
-
     def set_confirm_forcepoweroff(self, val):
         self.conf.set("/confirm/forcepoweroff", val)
     def set_confirm_poweroff(self, val):
@@ -404,6 +397,14 @@ class vmmConfig(object):
     def set_view_system_tray(self, val):
         self.conf.set("/system-tray", val)
 
+    # Libguestfs VM inspection
+    def on_libguestfs_inspect_vms_changed(self, cb):
+        return self.conf.notify_add("/enable-libguestfs-vm-inspection", cb)
+    def get_libguestfs_inspect_vms(self):
+        return self.conf.get("/enable-libguestfs-vm-inspection")
+    def set_libguestfs_inspect_vms(self, val):
+        self.conf.set("/enable-libguestfs-vm-inspection", val)
+
 
     # Stats history and interval length
     def get_stats_history_length(self):
diff --git a/virtManager/details.py b/virtManager/details.py
index eeeea4b9..2c7a4525 100644
--- a/virtManager/details.py
+++ b/virtManager/details.py
@@ -2370,61 +2370,62 @@ class vmmDetails(vmmGObjectUI):
                 self.widget(name).set_value(int(IdMap_proper))
 
     def refresh_inspection_page(self):
-        inspection_supported = self.config.support_inspection
+        inspection_supported = self.config.inspection_supported()
         uiutil.set_grid_row_visible(self.widget("details-overview-error"),
                                        self.vm.inspection.error)
         if self.vm.inspection.error:
             msg = _("Error while inspecting the guest configuration")
             self.widget("details-overview-error").set_text(msg)
 
-        # Operating System (ie. inspection data)
         self.widget("details-inspection-os").set_visible(inspection_supported)
-        if inspection_supported:
-            hostname = self.vm.inspection.hostname
-            if not hostname:
-                hostname = _("unknown")
-            self.widget("inspection-hostname").set_text(hostname)
-            os_type = self.vm.inspection.os_type
-            if not os_type:
-                os_type = "unknown"
-            self.widget("inspection-type").set_text(_label_for_os_type(os_type))
-            product_name = self.vm.inspection.product_name
-            if not product_name:
-                product_name = _("unknown")
-            self.widget("inspection-product-name").set_text(product_name)
+        self.widget("details-inspection-apps").set_visible(inspection_supported)
+        if not inspection_supported:
+            return
+
+        # Operating System (ie. inspection data)
+        hostname = self.vm.inspection.hostname
+        if not hostname:
+            hostname = _("unknown")
+        self.widget("inspection-hostname").set_text(hostname)
+        os_type = self.vm.inspection.os_type
+        if not os_type:
+            os_type = "unknown"
+        self.widget("inspection-type").set_text(_label_for_os_type(os_type))
+        product_name = self.vm.inspection.product_name
+        if not product_name:
+            product_name = _("unknown")
+        self.widget("inspection-product-name").set_text(product_name)
 
         # Applications (also inspection data)
-        self.widget("details-inspection-apps").set_visible(inspection_supported)
-        if inspection_supported:
-            apps = self.vm.inspection.applications or []
-            apps_list = self.widget("inspection-apps")
-            apps_model = apps_list.get_model()
-            apps_model.clear()
-            for app in apps:
-                name = ""
-                if app["app_name"]:
-                    name = app["app_name"]
-                if app["app_display_name"]:
-                    name = app["app_display_name"]
-                version = ""
-                if app["app_epoch"] > 0:
-                    version += str(app["app_epoch"]) + ":"
-                if app["app_version"]:
-                    version += app["app_version"]
-                if app["app_release"]:
-                    version += "-" + app["app_release"]
-                summary = ""
-                if app["app_summary"]:
-                    summary = app["app_summary"]
-                elif app["app_description"]:
-                    summary = app["app_description"]
-                    pos = summary.find("\n")
-                    if pos > -1:
-                        summary = _("%(summary)s ...") % {
-                            "summary": summary[0:pos]
-                        }
-
-                apps_model.append([name, version, summary])
+        apps = self.vm.inspection.applications or []
+        apps_list = self.widget("inspection-apps")
+        apps_model = apps_list.get_model()
+        apps_model.clear()
+        for app in apps:
+            name = ""
+            if app["app_name"]:
+                name = app["app_name"]
+            if app["app_display_name"]:
+                name = app["app_display_name"]
+            version = ""
+            if app["app_epoch"] > 0:
+                version += str(app["app_epoch"]) + ":"
+            if app["app_version"]:
+                version += app["app_version"]
+            if app["app_release"]:
+                version += "-" + app["app_release"]
+            summary = ""
+            if app["app_summary"]:
+                summary = app["app_summary"]
+            elif app["app_description"]:
+                summary = app["app_description"]
+                pos = summary.find("\n")
+                if pos > -1:
+                    summary = _("%(summary)s ...") % {
+                        "summary": summary[0:pos]
+                    }
+
+            apps_model.append([name, version, summary])
 
     def refresh_stats_page(self):
         def _multi_color(text1, text2):
@@ -3070,7 +3071,7 @@ class vmmDetails(vmmGObjectUI):
 
         add_hw_list_option(_("Overview"), HW_LIST_TYPE_GENERAL, "computer")
         if not self.is_customize_dialog:
-            if self.config.support_inspection:
+            if self.config.inspection_supported():
                 add_hw_list_option(_("OS information"),
                     HW_LIST_TYPE_INSPECTION, "computer")
             add_hw_list_option(_("Performance"), HW_LIST_TYPE_STATS,
diff --git a/virtManager/engine.py b/virtManager/engine.py
index d330bac4..47d96ec7 100644
--- a/virtManager/engine.py
+++ b/virtManager/engine.py
@@ -34,14 +34,15 @@ from .baseclass import vmmGObject
 from .clone import vmmCloneVM
 from .connect import vmmConnect
 from .connection import vmmConnection
-from .manager import vmmManager
-from .migrate import vmmMigrateDialog
-from .details import vmmDetails
 from .create import vmmCreate
-from .host import vmmHost
+from .delete import vmmDeleteDialog
+from .details import vmmDetails
 from .error import vmmErrorDialog
+from .host import vmmHost
+from .inspection import vmmInspection
+from .manager import vmmManager
+from .migrate import vmmMigrateDialog
 from .systray import vmmSystray
-from .delete import vmmDeleteDialog
 
 DETAILS_PERF = 1
 DETAILS_CONFIG = 2
@@ -93,8 +94,7 @@ class vmmEngine(vmmGObject):
         self._tick_thread.daemon = True
         self._tick_queue = queue.PriorityQueue(100)
 
-        self.inspection = None
-        self._create_inspection_thread()
+        vmmInspection.get_instance(self)
 
         # Counter keeping track of how many manager and details windows
         # are open. When it is decremented to 0, close the app or
@@ -394,10 +394,6 @@ class vmmEngine(vmmGObject):
     def _cleanup(self):
         self.err = None
 
-        if self.inspection:
-            self.inspection.cleanup()
-            self.inspection = None
-
         if self.timer is not None:
             GLib.source_remove(self.timer)
 
@@ -462,19 +458,6 @@ class vmmEngine(vmmGObject):
         logging.debug("Exiting app normally.")
         self._application.quit()
 
-    def _create_inspection_thread(self):
-        logging.debug("libguestfs inspection support: %s",
-                      self.config.support_inspection)
-        if not self.config.support_inspection:
-            return
-
-        from .inspection import vmmInspection
-        self.inspection = vmmInspection()
-        self.inspection.start()
-        self.connect("conn-added", self.inspection.conn_added)
-        self.connect("conn-removed", self.inspection.conn_removed)
-        return
-
     def _find_error_parent_cb(self):
         """
         Search over the toplevel windows for any that are visible or have
@@ -858,12 +841,14 @@ class vmmEngine(vmmGObject):
             src.err.show_err(_("Error setting clone parameters: %s") % str(e))
 
     def _do_refresh_inspection(self, src_ignore, uri, connkey):
-        if not self.inspection:
+        inspection = vmmInspection.get_instance(self)
+        if not inspection:
             return
 
         conn = self._lookup_conn(uri)
         vm = conn.get_vm(connkey)
-        self.inspection.vm_refresh(vm)
+        inspection.vm_refresh(vm)
+
 
     ##########################################
     # Window launchers from virt-manager cli #
diff --git a/virtManager/inspection.py b/virtManager/inspection.py
index 63ffcf09..02997fa4 100644
--- a/virtManager/inspection.py
+++ b/virtManager/inspection.py
@@ -17,11 +17,10 @@
 # MA 02110-1301 USA.
 #
 
-from queue import Queue
-from threading import Thread
+import functools
 import logging
-
-from guestfs import GuestFS  # pylint: disable=import-error
+import queue
+import threading
 
 from .baseclass import vmmGObject
 from .domain import vmmInspectionData
@@ -30,22 +29,55 @@ from .domain import vmmInspectionData
 class vmmInspection(vmmGObject):
     # Can't find a way to make Thread release our reference
     _leak_check = False
+    _instance = None
+    _libguestfs_installed = None
+
+    @classmethod
+    def get_instance(cls, engine):
+        if not cls._instance:
+            if not cls.libguestfs_installed():
+                return None
+            cls._instance = cls(engine)
+        return cls._instance
+
+    @classmethod
+    def libguestfs_installed(cls):
+        if cls._libguestfs_installed is None:
+            try:
+                import guestfs as ignore  # pylint: disable=import-error
+                logging.debug("python guestfs is installed")
+                cls._libguestfs_installed = True
+            except ImportError:
+                logging.debug("python guestfs is not installed")
+                cls._libguestfs_installed = False
+            except Exception:
+                logging.debug("error importing guestfs",
+                        exc_info=True)
+                cls._libguestfs_installed = False
+        return cls._libguestfs_installed
 
-    def __init__(self):
+    def __init__(self, engine):
         vmmGObject.__init__(self)
 
-        self._thread = Thread(name="inspection thread", target=self._run)
-        self._thread.daemon = True
+        self._thread = None
         self._wait = 5 * 1000  # 5 seconds
 
-        self._q = Queue()
+        self._q = queue.Queue()
         self._conns = {}
         self._vmseen = {}
         self._cached_data = {}
 
+        val = self.config.get_libguestfs_inspect_vms()
+        logging.debug("libguestfs gsetting enabled=%s", str(val))
+        if not val:
+            return
+        engine.connect("conn-added", self._conn_added)
+        engine.connect("conn-removed", self._conn_removed)
+        self._start()
+
     def _cleanup(self):
-        self._thread = None
-        self._q = Queue()
+        self._stop()
+        self._q = queue.Queue()
         self._conns = {}
         self._vmseen = {}
         self._cached_data = {}
@@ -53,16 +85,16 @@ class vmmInspection(vmmGObject):
     # Called by the main thread whenever a connection is added or
     # removed.  We tell the inspection thread, so it can track
     # connections.
-    def conn_added(self, engine_ignore, conn):
+    def _conn_added(self, engine_ignore, conn):
         obj = ("conn_added", conn)
         self._q.put(obj)
 
-    def conn_removed(self, engine_ignore, uri):
+    def _conn_removed(self, engine_ignore, uri):
         obj = ("conn_removed", uri)
         self._q.put(obj)
 
     # Called by the main thread whenever a VM is added to vmlist.
-    def vm_added(self, conn, connkey):
+    def _vm_added(self, conn, connkey):
         if connkey.startswith("guestfs-"):
             logging.debug("ignore libvirt/guestfs temporary VM %s",
                           connkey)
@@ -75,58 +107,80 @@ class vmmInspection(vmmGObject):
         obj = ("vm_refresh", vm.conn.get_uri(), vm.get_name(), vm.get_uuid())
         self._q.put(obj)
 
-    def start(self):
-        # Wait a few seconds before we do anything.  This prevents
-        # inspection from being a burden for initial virt-manager
-        # interactivity (although it shouldn't affect interactivity at
-        # all).
+    def _start(self):
+        if self._thread:
+            return
+
         def cb():
-            self._thread.start()
+            if self._thread:
+                self._thread.start()
             return 0
 
-        logging.debug("waiting")
+        self._thread = threading.Thread(
+                name="inspection thread", target=self._run)
+        self._thread.daemon = True
+
+        # Wait a few seconds before we do anything.  This prevents
+        # inspection from being a burden for initial virt-manager
+        # interactivity (although it shouldn't affect interactivity at all)
+        logging.debug("waiting before startup wait=%s", self._wait)
         self.timeout_add(self._wait, cb)
 
+    def _stop(self):
+        if self._thread is None:
+            return
+
+        self._q.put(None)
+        self._thread = None
+
     def _run(self):
         # Process everything on the queue.  If the queue is empty when
         # called, block.
         while True:
             obj = self._q.get()
+            if obj is None:
+                logging.debug("libguestfs queue obj=None, exiting thread")
+                return
             self._process_queue_item(obj)
             self._q.task_done()
 
     def _process_queue_item(self, obj):
-        if obj[0] == "conn_added":
+        cmd = obj[0]
+        if cmd == "conn_added":
             conn = obj[1]
             uri = conn.get_uri()
-            if conn and not (conn.is_remote()) and not (uri in self._conns):
-                self._conns[uri] = conn
-                conn.connect("vm-added", self.vm_added)
-                for vm in conn.list_vms():
-                    self.vm_added(conn, vm.get_connkey())
-        elif obj[0] == "conn_removed":
+            if conn.is_remote() or uri in self._conns:
+                return
+
+            self._conns[uri] = conn
+            conn.connect("vm-added", self._vm_added)
+            for vm in conn.list_vms():
+                self._vm_added(conn, vm.get_connkey())
+
+        elif cmd == "conn_removed":
             uri = obj[1]
-            del self._conns[uri]
-        elif obj[0] == "vm_added" or obj[0] == "vm_refresh":
+            self._conns.pop(uri)
+
+        elif cmd == "vm_added" or cmd == "vm_refresh":
             uri = obj[1]
-            if not (uri in self._conns):
+            if uri not in self._conns:
                 # This connection disappeared in the meanwhile.
                 return
+
             conn = self._conns[uri]
-            if not conn.is_active():
-                return
-            connkey = obj[2]
-            vm = conn.get_vm(connkey)
+            vm = conn.get_vm(obj[2])
             if not vm:
                 # The VM was removed in the meanwhile.
                 return
-            if obj[0] == "vm_refresh":
+
+            if cmd == "vm_refresh":
                 vmuuid = obj[3]
                 # When refreshing the inspection data of a VM,
                 # all we need is to remove it from the "seen" cache,
                 # as the data itself will be replaced once the new
                 # results are available.
-                del self._vmseen[vmuuid]
+                self._vmseen.pop(vmuuid)
+
             self._process_vm(conn, vm)
 
     # Try processing a single VM, keeping into account whether it was
@@ -167,12 +221,21 @@ class vmmInspection(vmmGObject):
             logging.exception("%s: exception while processing", prettyvm)
 
     def _inspect_vm(self, conn, vm):
-        g = GuestFS(close_on_exit=False)
-        prettyvm = conn.get_uri() + ":" + vm.get_name()
+        if self._thread is None:
+            return
 
-        g.add_libvirt_dom(vm.get_backend(), readonly=1)
+        import guestfs  # pylint: disable=import-error
 
-        g.launch()
+        g = guestfs.GuestFS(close_on_exit=False)
+        prettyvm = conn.get_uri() + ":" + vm.get_name()
+        try:
+            g.add_libvirt_dom(vm.get_backend(), readonly=1)
+            g.launch()
+        except Exception as e:
+            logging.debug("%s: Error launching libguestfs appliance: %s",
+                    prettyvm, str(e))
+            return None
+        logging.debug("%s: inspection appliance connected", prettyvm)
 
         # Inspect the operating system.
         roots = g.inspect_os()
@@ -210,8 +273,8 @@ class vmmInspection(vmmGObject):
                     return 0
                 else:
                     return -1
-            mps.sort(compare)
 
+            mps.sort(key=functools.cmp_to_key(compare))
             for mp_dev in mps:
                 try:
                     g.mount_ro(mp_dev[1], mp_dev[0])
@@ -261,7 +324,7 @@ class vmmInspection(vmmGObject):
         data.product_name = str(product_name)
         data.product_variant = str(product_variant)
         data.icon = icon
-        data.applications = list(apps)
+        data.applications = list(apps or [])
         data.error = False
 
         return data
diff --git a/virtManager/preferences.py b/virtManager/preferences.py
index a4d57808..11ee6b37 100644
--- a/virtManager/preferences.py
+++ b/virtManager/preferences.py
@@ -25,6 +25,7 @@ from gi.repository import Gdk
 
 from . import uiutil
 from .baseclass import vmmGObjectUI
+from .inspection import vmmInspection
 
 
 class vmmPreferences(vmmGObjectUI):
@@ -45,7 +46,10 @@ class vmmPreferences(vmmGObjectUI):
 
         self._init_ui()
 
+        self._orig_libguestfs_val = None
+
         self.refresh_view_system_tray()
+        self.refresh_libguestfs()
         self.refresh_update_interval()
         self.refresh_console_accels()
         self.refresh_console_scaling()
@@ -73,6 +77,7 @@ class vmmPreferences(vmmGObjectUI):
             "on_prefs_close_clicked": self.close,
 
             "on_prefs_system_tray_toggled": self.change_view_system_tray,
+            "on_prefs_libguestfs_toggled": self.change_libguestfs,
             "on_prefs_stats_update_interval_changed": self.change_update_interval,
             "on_prefs_console_accels_toggled": self.change_console_accels,
             "on_prefs_console_scaling_changed": self.change_console_scaling,
@@ -181,6 +186,11 @@ class vmmPreferences(vmmGObjectUI):
         combo.set_model(model)
         uiutil.init_combo_text_column(combo, 1)
 
+        if not vmmInspection.libguestfs_installed():
+            self.widget("prefs-libguestfs").set_sensitive(False)
+            self.widget("prefs-libguestfs").set_tooltip_text(
+                    _("python libguestfs support is not installed"))
+
 
     #########################
     # Config Change Options #
@@ -190,6 +200,12 @@ class vmmPreferences(vmmGObjectUI):
         val = self.config.get_view_system_tray()
         self.widget("prefs-system-tray").set_active(bool(val))
 
+    def refresh_libguestfs(self):
+        val = self.config.get_libguestfs_inspect_vms()
+        if self._orig_libguestfs_val is None:
+            self._orig_libguestfs_val = val
+        self.widget("prefs-libguestfs").set_active(bool(val))
+
     def refresh_update_interval(self):
         self.widget("prefs-stats-update-interval").set_value(
             self.config.get_stats_update_interval())
@@ -341,6 +357,14 @@ class vmmPreferences(vmmGObjectUI):
 
     def change_view_system_tray(self, src):
         self.config.set_view_system_tray(src.get_active())
+    def change_libguestfs(self, src):
+        val = src.get_active()
+        self.config.set_libguestfs_inspect_vms(val)
+
+        vis = (val != self._orig_libguestfs_val and
+               self.widget("prefs-libguestfs").get_sensitive())
+        uiutil.set_grid_row_visible(
+                self.widget("prefs-libguestfs-warn-box"), vis)
 
     def change_update_interval(self, src):
         self.config.set_stats_update_interval(src.get_value_as_int())