domain: Split cdrom media change into hotplug and define pieces.

This allows us to check for hotplug errors, but still define the change
to take place after the next reboot.

src/virtManager/details.py

@@ -864,6 +864,33 @@ class vmmDetails(gobject.GObject):
     def config_boot_options_changed(self, src):
         self.window.get_widget("config-apply").set_sensitive(True)
 
+    # CDROM Eject/Connect
+    def toggle_cdrom(self, src):
+        info = self.get_hw_selection(HW_LIST_COL_DEVICE)
+        if not info:
+            return
+
+        dev_id_info = info[1]
+        curpath = info[3]
+
+        if curpath:
+            # Disconnect cdrom
+            self.change_cdrom_media(dev_id_info, None, _type=None)
+
+        else:
+            def change_cdrom_wrapper(src, dev_id_info, newpath,
+                                     _type=None):
+                return self.change_cdrom_media(dev_id_info, newpath, _type)
+
+            # Launch 'Choose CD' dialog
+            if self.choose_cd is None:
+                self.choose_cd = vmmChooseCD(self.config,
+                                             dev_id_info,
+                                             self.vm.get_connection())
+                self.choose_cd.connect("cdrom-chosen", change_cdrom_wrapper)
+            else:
+                self.choose_cd.dev_id_info = dev_id_info
+            self.choose_cd.show()
 
     ##################################################
     # Details/Hardware config changes (apply button) #
@@ -990,35 +1017,30 @@ class vmmDetails(gobject.GObject):
                 return False
 
     # CDROM
-    def toggle_cdrom(self, src):
-        info = self.get_hw_selection(HW_LIST_COL_DEVICE)
-        if not info:
-            return
+    def change_cdrom_media(self, dev_id_info, newpath, _type=None):
+        hotplug_err = False
 
-        dev_id_info = info[1]
-
-        if src.get_label() == gtk.STOCK_DISCONNECT:
-            # Disconnect cdrom
-            self.vm.change_cdrom_media(dev_id_info, None)
-
-        else:
-            # connect a new cdrom
-            if self.choose_cd is None:
-                self.choose_cd = vmmChooseCD(self.config,
-                                             dev_id_info,
-                                             self.vm.get_connection())
-                self.choose_cd.connect("cdrom-chosen", self.change_cdrom_media)
-            else:
-                self.choose_cd.dev_id_info = dev_id_info
-            self.choose_cd.show()
-
-    def change_cdrom_media(self, ignore, dev_id_info, newpath, _type=None):
+        # Hotplug change
         try:
-            self.vm.change_cdrom_media(dev_id_info, newpath, _type)
+            if self.vm.is_active():
+                self.vm.hotplug_cdrom_media(dev_id_info, newpath, _type)
+        except Exception, e:
+            logging.debug("CDROM eject/connect failed: %s" % str(e))
+            hotplug_err = True
+
+        # Persistent config change
+        try:
+            self.vm.define_cdrom_media(dev_id_info, newpath, _type)
         except Exception, e:
             self.err.show_err(_("Error changing CDROM media: %s" % str(e)),
                               "".join(traceback.format_exc()))
+            return
 
+        if hotplug_err:
+            self.err.show_info(_("These changes will take effect after the "
+                                 "next guest reboot."))
+
+    # Device removal
     def remove_device(self, dev_type, dev_id_info):
         logging.debug("Removing device: %s %s" % (dev_type, dev_id_info))
 

src/virtManager/domain.py

@@ -349,6 +349,7 @@ class vmmDomain(gobject.GObject):
                 raise e
 
 
+    # Generic device Add/Remove
     def add_device(self, devxml):
         """
         Redefine guest with appended device XML 'devxml'
@@ -383,69 +384,81 @@ class vmmDomain(gobject.GObject):
 
         self.redefine(_remove_xml_device, dev_type, dev_id_info)
 
-    def change_cdrom_media(self, dev_id_info, newpath, _type=None):
-        def cdrom_xml_connect(doc, ctx):
-            disk_fragment = self._get_device_xml_nodes(ctx, "disk",
-                                                       dev_id_info)[0]
-            driver_fragment = None
+    # Media change
 
-            for child in disk_fragment.children or []:
-                if child.name == "driver":
-                    driver_fragment = child
+    # Helper for connecting a new source path to an existing disk
+    def _media_xml_connect(self, doc, ctx, dev_id_info, newpath, _type):
+        disk_fragment = self._get_device_xml_nodes(ctx, "disk",
+                                                   dev_id_info)[0]
+        driver_fragment = None
 
-            disk_fragment.setProp("type", _type)
-            elem = disk_fragment.newChild(None, "source", None)
+        for child in disk_fragment.children or []:
+            if child.name == "driver":
+                driver_fragment = child
 
-            if _type == "file":
-                elem.setProp("file", newpath)
-                driver_name = _type
+        disk_fragment.setProp("type", _type)
+        elem = disk_fragment.newChild(None, "source", None)
+
+        if _type == "file":
+            elem.setProp("file", newpath)
+            driver_name = _type
+        else:
+            elem.setProp("dev", newpath)
+            driver_name = "phy"
+
+        if driver_fragment:
+            orig_name = driver_fragment.prop("name")
+
+            # For Xen, the driver name is dependent on the storage type
+            # (file or phys).
+            if orig_name and orig_name in [ "file", "phy" ]:
+                driver_fragment.setProp("name", driver_name)
+
+        return doc.serialize(), disk_fragment.serialize()
+
+    # Helper for disconnecting a path from an existing disk
+    def _media_xml_disconnect(self, doc, ctx, dev_id_info, newpath, _type):
+        disk_fragment = self._get_device_xml_nodes(ctx, "disk",
+                                                   dev_id_info)[0]
+        sourcenode = None
+
+        for child in disk_fragment.children:
+            if child.name == "source":
+                sourcenode = child
+                break
             else:
-                elem.setProp("dev", newpath)
-                driver_name = "phy"
-
-            if driver_fragment:
-                orig_name = driver_fragment.prop("name")
-
-                # For Xen, the driver name is dependent on the storage type
-                # (file or phys).
-                if orig_name and orig_name in [ "file", "phy" ]:
-                    driver_fragment.setProp("name", driver_name)
-
-            return doc.serialize(), disk_fragment.serialize()
-
-        def cdrom_xml_disconnect(doc, ctx):
-            disk_fragment = self._get_device_xml_nodes(ctx, "disk",
-                                                       dev_id_info)[0]
-
-            sourcenode = None
-
-            for child in disk_fragment.children:
-                if child.name == "source":
-                    sourcenode = child
-                    break
-                else:
-                    continue
+                continue
 
+        if sourcenode:
             sourcenode.unlinkNode()
             sourcenode.freeNode()
-            return doc.serialize(), disk_fragment.serialize()
 
+        return doc.serialize(), disk_fragment.serialize()
+
+    def define_cdrom_media(self, dev_id_info, newpath, _type=None):
         if not newpath:
-            func = cdrom_xml_disconnect
+            func = self._media_xml_disconnect
         else:
-            func = cdrom_xml_connect
+            func = self._media_xml_connect
 
-        ignore, diskxml = util.xml_parse_wrapper(self.get_xml(), func)
-
-        def change_cdrom_helper(origxml, newdev, dev_id_info):
-            vmxml, ignore = util.xml_parse_wrapper(origxml, func)
+        def change_cdrom_helper(origxml):
+            vmxml, ignore = util.xml_parse_wrapper(origxml, func, dev_id_info,
+                                                   newpath, _type)
             return vmxml
-        self.redefine(change_cdrom_helper, diskxml, dev_id_info)
+        self.redefine(change_cdrom_helper)
 
-        # If vm is shutoff, remove device, and redefine with media
-        if self.is_active():
-            self.attach_device(diskxml)
+    def hotplug_cdrom_media(self, dev_id_info, newpath, _type=None):
+        if not newpath:
+            func = self._media_xml_disconnect
+        else:
+            func = self._media_xml_connect
 
+        ignore, diskxml = util.xml_parse_wrapper(self.get_xml(), func,
+                                                 dev_id_info, newpath, _type)
+
+        self.attach_device(diskxml)
+
+    # VCPU changing
     def define_vcpus(self, vcpus, cpuset=None):
         vcpus = int(vcpus)