guest: Encode defaults into the XML at install time

Previously we made all the default encoding non-permanent by XML objects
before generating the XML, making changes to the copies, and restoring
to the old state after the XML is returned to the user.

This allows us to call start_install multiple times with the same Guest
object and not alter it's original config... but that feature isn't really
useful anymore, and this behavior makes the 'customize before install'
dialog difficult to handle.

So drop most of it, and fix some of the minor fallout.

tests/xmlconfig.py

@@ -287,43 +287,43 @@ class TestXMLMisc(unittest.TestCase):
         """
         # Use connver=12005 so that non-rhel displays ac97
         conn = utils.open_rhelkvm(connver=12005)
+
         g = _make_guest(conn=conn)
-        do_install = False
-
-        # Call get_xml_config sets first round of defaults w/o os_variant set
-        g.get_install_xml(do_install)
-
         g.os_variant = "fedora11"
-        self._compare(g, "install-f11-norheldefaults", do_install)
+        self._compare(g, "install-f11-norheldefaults", False)
 
         try:
             CLIConfig.stable_defaults = True
+
+            g = _make_guest(conn=conn)
+            g.os_variant = "fedora11"
             origemu = g.emulator
             g.emulator = "/usr/libexec/qemu-kvm"
             self.assertTrue(g.conn.stable_defaults())
 
             setattr(g.conn, "_support_cache", {})
-            self._compare(g, "install-f11-rheldefaults", do_install)
+            self._compare(g, "install-f11-rheldefaults", False)
             g.emulator = origemu
             setattr(g.conn, "_support_cache", {})
         finally:
             CLIConfig.stable_defaults = False
 
-        # Verify main guest wasn't polluted
-        self._compare(g, "install-f11-norheldefaults", do_install)
-
     def test_no_vmvga_RHEL(self):
         # Test that vmvga is not used on RHEL
         conn = utils.open_rhelkvm()
-        g = _make_guest(conn=conn)
+        def _make():
-
+            g = _make_guest(conn=conn)
-        try:
             g.emulator = "/usr/libexec/qemu-kvm"
             g.add_default_video_device()
             g.os_variant = "ubuntu13.10"
+            return g
 
+        try:
+            g = _make()
             self._compare(g, "install-novmvga-rhel", True)
+
             CLIConfig.stable_defaults = True
+            g = _make()
             self._compare(g, "install-novmvga-rhel", True)
         finally:
             CLIConfig.stable_defaults = False

virtinst/cli.py

@@ -1626,7 +1626,7 @@ class ParserDisk(VirtCLIParser):
         if not inst.target:
             skip_targets = [d.target for d in self.guest.get_devices("disk")]
             inst.generate_target(skip_targets)
-            inst.cli_set_target = True
+            inst.cli_generated_target = True
 
         return inst
 

virtinst/guest.py

@@ -125,6 +125,7 @@ class Guest(XMLBuilder):
         self.__os_object = None
         self._random_uuid = None
         self._install_devices = []
+        self._defaults_are_set = False
 
         # The libvirt virDomain object we 'Create'
         self.domain = None
@@ -301,22 +302,12 @@ class Guest(XMLBuilder):
             self.add_device(dev)
             self._install_devices.append(dev)
 
-
-    ##############
-    # Public API #
-    ##############
-
     def _prepare_get_xml(self):
-        # We do a shallow copy of the device list here, and set the defaults.
+        # We do a shallow copy of the OS block here, so that we can
-        # This way, default changes aren't persistent, and we don't need
+        # set the install time properties but not permanently overwrite
-        # to worry about when to call set_defaults
+        # any config the user explicitly requested.
-        #
+        data = self.os
-        # XXX: this is hacky, we should find a way to use xmlbuilder.copy(),
-        # but need to make sure it's not a massive performance hit
-        data = (self._devices[:], self.features, self.os)
         try:
-            self._propstore["_devices"] = [dev.copy() for dev in self._devices]
-            self._propstore["features"] = self.features.copy()
             self._propstore["os"] = self.os.copy()
         except:
             self._finish_get_xml(data)
@@ -324,11 +315,9 @@ class Guest(XMLBuilder):
         return data
 
     def _finish_get_xml(self, data):
-        (self._propstore["_devices"],
+        self._propstore["os"] = data
-         self._propstore["features"],
-         self._propstore["os"]) = data
 
-    def get_install_xml(self, *args, **kwargs):
+    def _get_install_xml(self, *args, **kwargs):
         data = self._prepare_get_xml()
         try:
             return self._do_get_install_xml(*args, **kwargs)
@@ -355,14 +344,19 @@ class Guest(XMLBuilder):
         if osblob_install and not self.installer.has_install_phase():
             return None
 
-        self.installer.alter_bootconfig(self, osblob_install, self.os)
+        self.installer.alter_bootconfig(self, osblob_install)
         self._set_transient_device_defaults(install)
 
         action = install and "destroy" or "restart"
         self.on_reboot = action
         self.on_crash = action
 
-        self._set_defaults()
+        self._set_osxml_defaults()
+
+        # Only set defaults on any install devices
+        for dev in self._install_devices:
+            dev.set_defaults(self)
+        self._set_disk_defaults(disks=self._install_devices)
 
         self.bootloader = None
         if (not install and
@@ -373,6 +367,11 @@ class Guest(XMLBuilder):
 
         return self.get_xml_config()
 
+
+    ##############
+    # Public API #
+    ##############
+
     def get_continue_inst(self):
         """
         Return True if this guest requires a call to 'continue_install',
@@ -385,11 +384,6 @@ class Guest(XMLBuilder):
 
         return self._os_object.is_windows()
 
-
-    ##########################
-    # Actual install methods #
-    ##########################
-
     def start_install(self, meter=None,
                       dry=False, return_xml=False, noboot=False):
         """
@@ -400,6 +394,7 @@ class Guest(XMLBuilder):
             raise RuntimeError(_("Domain has already been started!"))
 
         is_initial = True
+        self.set_install_defaults()
 
         self._prepare_install(meter, dry)
         try:
@@ -461,8 +456,8 @@ class Guest(XMLBuilder):
         log_label = is_initial and "install" or "continue"
         disk_boot = not is_initial
 
-        start_xml = self.get_install_xml(install=True, disk_boot=disk_boot)
+        start_xml = self._get_install_xml(install=True, disk_boot=disk_boot)
-        final_xml = self.get_install_xml(install=False)
+        final_xml = self._get_install_xml(install=False)
 
         logging.debug("Generated %s XML: %s",
                       log_label,
@@ -554,6 +549,21 @@ class Guest(XMLBuilder):
     # Device defaults #
     ###################
 
+    def set_install_defaults(self):
+        """
+        Allow API users to set defaults ahead of time if they want it.
+        Used by vmmDomainVirtinst so the 'Customize before install' dialog
+        shows accurate values.
+
+        If the user doesn't explicitly call this, it will be called by
+        start_install()
+        """
+        if self._defaults_are_set:
+            return
+
+        self._set_defaults()
+        self._defaults_are_set = True
+
     def stable_defaults(self, *args, **kwargs):
         return self.conn.stable_defaults(self.emulator, *args, **kwargs)
 
@@ -673,7 +683,6 @@ class Guest(XMLBuilder):
                 dev.path = None
 
     def _set_defaults(self):
-        self._set_osxml_defaults()
         self._set_clock_defaults()
         self._set_emulator_defaults()
         self._set_cpu_defaults()
@@ -917,7 +926,11 @@ class Guest(XMLBuilder):
 
         return False
 
-    def _set_disk_defaults(self):
+    def _set_disk_defaults(self, disks=None):
+        alldisks = self.get_devices("disk")
+        if disks is None:
+            disks = alldisks
+
         def set_disk_bus(d):
             if d.is_floppy():
                 d.bus = "fdc"
@@ -948,15 +961,19 @@ class Guest(XMLBuilder):
             else:
                 d.bus = "ide"
 
+
+        # Generate disk targets
         used_targets = []
-        for disk in self.get_devices("disk"):
+        for disk in disks:
             if not disk.bus:
                 set_disk_bus(disk)
 
-            # Generate disk targets
+        for disk in alldisks:
-            if disk.target and not getattr(disk, "cli_set_target", False):
+            if disk.target and not getattr(disk,
+                    "cli_generated_target", False):
                 used_targets.append(disk.target)
-            else:
+            elif disk in disks:
+                disk.cli_generated_target = False
                 used_targets.append(disk.generate_target(used_targets))
 
     def _set_net_defaults(self):

virtinst/installer.py

@@ -112,7 +112,7 @@ class Installer(object):
         dev.validate()
         return dev
 
-    def alter_bootconfig(self, guest, isinstall, bootconfig):
+    def alter_bootconfig(self, guest, isinstall):
         """
         Generate the portion of the guest xml that determines boot devices
         and parameters. (typically the <os></os> block)
@@ -128,20 +128,20 @@ class Installer(object):
 
         bootorder = self._build_boot_order(isinstall, guest)
 
-        if not bootconfig.bootorder:
+        if not guest.os.bootorder:
             # Per device <boot order> is not compatible with os/boot.
             if not any(d.boot.order for d in guest.get_all_devices()):
-                bootconfig.bootorder = bootorder
+                guest.os.bootorder = bootorder
 
         if not isinstall:
             return
 
         if self._install_kernel:
-            bootconfig.kernel = self._install_kernel
+            guest.os.kernel = self._install_kernel
         if self._install_initrd:
-            bootconfig.initrd = self._install_initrd
+            guest.os.initrd = self._install_initrd
         if self.extraargs:
-            bootconfig.kernel_args = self.extraargs
+            guest.os.kernel_args = self.extraargs
 
 
     ##########################