cloner: Big API rework

* Centralize lots of disk building
* Open code virt-clone specific behavior at the source
* Drop a lot or properties
* Move most testing to test_cli.py
* Generally a ton of cleanup

virt-manager clone wizard has not been converted yet so is totally
broken after this commit

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

1 files changed, 358 insertions, 476 deletions

diff --git a/virtinst/cloner.py b/virtinst/cloner.py
index 4ce56683..3c23e8cb 100644
--- a/virtinst/cloner.py
+++ b/virtinst/cloner.py
@@ -14,7 +14,6 @@ import libvirt
 
 from . import generatename
 from . import progress
-from . import xmlutil
 from .guest import Guest
 from .devices import DeviceInterface
 from .devices import DeviceDisk
@@ -49,394 +48,416 @@ def _replace_vm(conn, name):
             })
 
 
+def _generate_clone_name(conn, basename):
+    """
+    If the orig name is "foo-clone", we don't want the clone to be
+    "foo-clone-clone", we want "foo-clone1"
+    """
+    match = re.search("-clone[1-9]*$", basename)
+    start_num = 1
+    force_num = False
+    if match:
+        num_match = re.search("[1-9]+$", match.group())
+        if num_match:
+            start_num = int(str(num_match.group())) + 1
+            force_num = True
+        basename = basename.replace(match.group(), "")
+
+    def cb(n):
+        return generatename.check_libvirt_collision(
+            conn.lookupByName, n)
+    basename = basename + "-clone"
+    return generatename.generate_name(basename, cb,
+            sep="", start_num=start_num, force_num=force_num)
+
+
+def _generate_clone_disk_path(conn, origname, newname, origpath):
+    """
+    Generate desired cloned disk path name, derived from the
+    original path, original VM name, and proposed new VM name
+    """
+    if origpath is None:
+        return None
+
+    path = origpath
+    suffix = ""
+
+    # Try to split the suffix off the existing disk name. Ex.
+    # foobar.img -> foobar-clone.img
+    #
+    # If the suffix is greater than 7 characters, assume it isn't
+    # a file extension and is part of the disk name, at which point
+    # just stick '-clone' on the end.
+    if "." in origpath and len(origpath.rsplit(".", 1)[1]) <= 7:
+        path, suffix = origpath.rsplit(".", 1)
+        suffix = "." + suffix
+
+    dirname = os.path.dirname(path)
+    basename = os.path.basename(path)
+
+    clonebase = basename + "-clone"
+    if origname and basename == origname:
+        clonebase = newname
+
+    clonebase = os.path.join(dirname, clonebase)
+    def cb(p):
+        return DeviceDisk.path_definitely_exists(conn, p)
+    return generatename.generate_name(clonebase, cb, suffix=suffix)
+
+
+def _lookup_vm(conn, name):
+    try:
+        return conn.lookupByName(name)
+    except libvirt.libvirtError:
+        e = ValueError(_("Domain '%s' was not found.") % str(name))
+        raise e from None
+
+
+def _build_clone_vol_install(orig_disk, clone_disk):
+    vol_install = DeviceDisk.build_vol_install(
+        orig_disk.conn, os.path.basename(clone_disk.path),
+        clone_disk.get_parent_pool(), .000001, False)
+    vol_install.input_vol = orig_disk.get_vol_object()
+
+    # Source and dest are managed. If they share the same pool,
+    # replace vol_install with a CloneVolume instance, otherwise
+    # simply set input_vol on the dest vol_install
+    if (vol_install.pool.name() ==
+        orig_disk.get_parent_pool().name()):
+        vol_install.sync_input_vol()
+    else:
+        # Cross pool cloning
+        # Sync only the format of the image.
+        vol_install.sync_input_vol(only_format=True)
+
+    return vol_install
+
+
+def _build_clone_disk(orig_disk, clonepath, allow_create, sparse):
+    conn = orig_disk.conn
+    device = DeviceDisk.DEVICE_DISK
+    if not clonepath:
+        device = DeviceDisk.DEVICE_CDROM
+
+    clone_disk = DeviceDisk(conn)
+    clone_disk.path = clonepath
+    clone_disk.device = device
+
+    if not allow_create:
+        clone_disk.validate()
+        return clone_disk
+
+    if clone_disk.get_vol_object():
+        # Special case: non remote cloning of a guest using
+        # managed block devices: fall back to local cloning if
+        # we have permissions to do so. This validation check
+        # caused a few bug reports in a short period of time,
+        # so must be a common case.
+        if (conn.is_remote() or
+            clone_disk.type != clone_disk.TYPE_BLOCK or
+            not orig_disk.path or
+            not os.access(orig_disk.path, os.R_OK) or
+            not clone_disk.path or
+            not os.access(clone_disk.path, os.W_OK)):
+            raise RuntimeError(
+                _("Clone onto existing storage volume is not "
+                  "currently supported: '%s'") % clone_disk.path)
+
+    if (orig_disk.get_vol_object() and
+        clone_disk.wants_storage_creation()):
+        vol_install = _build_clone_vol_install(orig_disk, clone_disk)
+        if not sparse:
+            vol_install.allocation = vol_install.capacity
+        clone_disk.set_vol_install(vol_install)
+    elif orig_disk.path:
+        clone_disk.set_local_disk_to_clone(orig_disk, sparse)
+
+    clone_disk.validate()
+    return clone_disk
+
+
+class _CloneDiskInfo:
+    """
+    Class that tracks some additional information about how we want
+    to default handle each disk of the source VM
+    """
+    def __init__(self, srcdisk):
+        self.disk = DeviceDisk(srcdisk.conn, parsexml=srcdisk.get_xml())
+        self._do_clone = self._do_we_clone_default()
+        self.clone_disk = None
+
+    def is_clone_requested(self):
+        return self._do_clone
+    def set_clone_requested(self, val):
+        self._do_clone = val
+
+    def _do_we_clone_default(self):
+        if not self.disk.path:
+            return False
+        if self.disk.read_only:
+            return False
+        if self.disk.shareable:
+            return False
+        return True
+
+    def check_clonable(self):
+        try:
+            # This forces DeviceDisk to resolve the storage backend
+            self.disk.path = self.disk.path
+            if self.disk.wants_storage_creation():
+                raise ValueError(
+                        _("Disk path '%s' does not exist.") % self.disk.path)
+        except Exception as e:
+            log.debug("Exception processing clone original path", exc_info=True)
+            err = _("Could not determine original disk information: %s" % str(e))
+            raise ValueError(err) from None
+
+    def set_clone_path(self, path, allow_create, sparse):
+        if allow_create:
+            self.check_clonable()
+
+        try:
+            self.clone_disk = _build_clone_disk(
+                    self.disk, path, allow_create, sparse)
+        except Exception as e:
+            log.debug("Error setting clone path.", exc_info=True)
+            raise ValueError(
+                _("Could not use path '%(path)s' for cloning: %(error)s") % {
+                    "path": path,
+                    "error": str(e),
+                })
+
+
 class Cloner(object):
+    @staticmethod
+    def generate_clone_name(conn, basename):
+        return _generate_clone_name(conn, basename)
 
-    # Reasons why we don't default to cloning.
-    CLONE_POLICY_NO_READONLY   = 1
-    CLONE_POLICY_NO_SHAREABLE  = 2
-    CLONE_POLICY_NO_EMPTYMEDIA = 3
+    @staticmethod
+    def generate_clone_disk_path(conn, origname, newname, origpath):
+        return _generate_clone_disk_path(conn, origname, newname, origpath)
 
-    def __init__(self, conn):
+    def __init__(self, conn, src_name=None, src_xml=None):
         self.conn = conn
 
-        # original guest name or uuid
-        self._original_guest = None
-        self.original_dom = None
-        self._original_disks = []
-        self._original_xml = None
-        self._guest = None
-
-        # clone guest
-        self._clone_name = None
-        self._clone_disks = []
-        self._clone_macs = []
-        self._clone_uuid = None
-        self._clone_sparse = True
-        self._clone_xml = None
-        self.clone_nvram = None
+        self._src_guest = None
+        self._new_guest = None
+        self._diskinfos = []
+        self._init_src(src_name, src_xml)
+
+        self._new_nvram_path = None
         self._nvram_disk = None
 
-        self._force_target = []
-        self._skip_target = []
-        self._preserve = True
-        self._clone_running = False
+        self._sparse = True
+        self._overwrite = True
         self._replace = False
         self._reflink = False
 
-        # Default clone policy for back compat: don't clone readonly,
-        # shareable, or empty disks
-        self._clone_policy = []
-        self.clone_policy = [self.CLONE_POLICY_NO_READONLY,
-                             self.CLONE_POLICY_NO_SHAREABLE,
-                             self.CLONE_POLICY_NO_EMPTYMEDIA]
 
-        # Generate a random UUID at the start
-        self.clone_uuid = Guest.generate_uuid(conn)
+    #################
+    # Init routines #
+    #################
 
+    def _init_src(self, src_name, src_xml):
+        """
+        Set up the source VM info we are cloning, from passed in VM name
+        or full XML
+        """
+        if not src_xml:
+            dom = _lookup_vm(self.conn, src_name)
+            status = dom.info()[0]
+            if status not in [libvirt.VIR_DOMAIN_SHUTOFF]:
+                raise RuntimeError(_("Domain to clone must be shutoff."))
+            flags = libvirt.VIR_DOMAIN_XML_SECURE
+            src_xml = dom.XMLDesc(flags)
 
-    ##############
-    # Properties #
-    ##############
+        log.debug("Original XML:\n%s", src_xml)
 
-    # Original guest name
-    def get_original_guest(self):
-        return self._original_guest
-    def set_original_guest(self, original_guest):
-        if self._lookup_vm(original_guest):
-            self._original_guest = original_guest
-    original_guest = property(get_original_guest, set_original_guest)
-
-    # XML of the original guest
-    def set_original_xml(self, val):
-        self._original_xml = val
-        self._original_guest = Guest(self.conn,
-                                     parsexml=self._original_xml).name
-    def get_original_xml(self):
-        return self._original_xml
-    original_xml = property(get_original_xml, set_original_xml)
-
-    # Name to use for the new guest clone
-    def get_clone_name(self):
-        return self._clone_name
-    def set_clone_name(self, name):
-        try:
-            Guest.validate_name(self.conn, name,
-                                check_collision=not self.replace,
-                                validate=False)
-        except ValueError as e:
-            raise ValueError(_("Invalid name for new guest: %s") % e)
+        self._src_guest = Guest(self.conn, parsexml=src_xml)
+        self._new_guest = Guest(self.conn, parsexml=src_xml)
+        self._init_new_guest()
 
-        self._clone_name = name
-    clone_name = property(get_clone_name, set_clone_name)
+        # Collect disk info for every disk to determine if we will
+        # default to cloning or not
+        for disk in self._src_guest.devices.disk:
+            self._diskinfos.append(_CloneDiskInfo(disk))
+        for diskinfo in [d for d in self._diskinfos if d.is_clone_requested()]:
+            disk = diskinfo.disk
+            log.debug("Wants cloning: size=%s path=%s",
+                    disk.get_size(), disk.path)
 
-    # UUID to use for the new guest clone
-    def set_clone_uuid(self, uuid):
-        self._clone_uuid = uuid
-    def get_clone_uuid(self):
-        return self._clone_uuid
-    clone_uuid = property(get_clone_uuid, set_clone_uuid)
-
-    # Paths to use for the new disk locations
-    def set_clone_paths(self, paths):
-        disklist = []
-        for path in xmlutil.listify(paths):
-            try:
-                device = DeviceDisk.DEVICE_DISK
-                if not path:
-                    device = DeviceDisk.DEVICE_CDROM
-
-                disk = DeviceDisk(self.conn)
-                disk.path = path
-                disk.device = device
-
-                if (not self.preserve_dest_disks and
-                    disk.wants_storage_creation()):
-                    vol_install = DeviceDisk.build_vol_install(
-                        self.conn, os.path.basename(disk.path),
-                        disk.get_parent_pool(), .000001, False)
-                    disk.set_vol_install(vol_install)
-                disk.validate()
-                disklist.append(disk)
-            except Exception as e:
-                log.debug("Error setting clone path.", exc_info=True)
-                raise ValueError(
-                    _("Could not use path '%(path)s' for cloning: %(error)s") % {
-                        "path": path,
-                        "error": str(e),
-                    })
+    def _init_new_guest(self):
+        """
+        Perform the series of unconditional new VM changes we always make
+        """
+        self._new_guest.id = None
+        self._new_guest.title = None
+        self._new_guest.uuid = None
+        self._new_guest.uuid = Guest.generate_uuid(self.conn)
 
-        self._clone_disks = disklist
-    def get_clone_paths(self):
-        return [d.path for d in self.clone_disks]
-    clone_paths = property(get_clone_paths, set_clone_paths)
+        for dev in self._new_guest.devices.graphics:
+            if dev.port and dev.port != -1:
+                log.warning(_("Setting the graphics device port to autoport, "
+                               "in order to avoid conflicting."))
+                dev.port = -1
 
-    # DeviceDisk instances for the new disk paths
-    @property
-    def clone_disks(self):
-        return self._clone_disks
+        for iface in self._new_guest.devices.interface:
+            iface.target_dev = None
+            iface.macaddr = DeviceInterface.generate_mac(self.conn)
 
-    # MAC address for the new guest clone
-    def set_clone_macs(self, mac):
-        self._clone_macs = xmlutil.listify(mac)
-    def get_clone_macs(self):
-        return self._clone_macs
-    clone_macs = property(get_clone_macs, set_clone_macs)
+        # For guest agent channel, remove a path to generate a new one with
+        # new guest name
+        for channel in self._new_guest.devices.channel:
+            if (channel.type == DeviceChannel.TYPE_UNIX and
+                channel.target_name and channel.source.path and
+                channel.target_name in channel.source.path):
+                channel.source.path = None
 
-    # DeviceDisk instances of the original disks being cloned
-    @property
-    def original_disks(self):
-        return self._original_disks
-
-    # Generated XML for the guest clone
-    def get_clone_xml(self):
-        return self._clone_xml
-    def set_clone_xml(self, clone_xml):
-        self._clone_xml = clone_xml
-    clone_xml = property(get_clone_xml, set_clone_xml)
-
-    # Whether to attempt sparse allocation during cloning
-    def get_clone_sparse(self):
-        return self._clone_sparse
-    def set_clone_sparse(self, flg):
-        self._clone_sparse = flg
-    clone_sparse = property(get_clone_sparse, set_clone_sparse)
-
-    # If true, preserve ALL original disk devices
-    def get_preserve(self):
-        return self._preserve
-    def set_preserve(self, flg):
-        self._preserve = flg
-    preserve = property(get_preserve, set_preserve)
-
-    # If true, preserve ALL disk devices for the NEW guest.
-    # This means no storage cloning.
-    # This is a convenience access for not Cloner.preserve
-    @property
-    def preserve_dest_disks(self):
-        return not self.preserve
-
-    # List of disk targets that we force cloning despite
-    # Cloner's recommendation
-    def set_force_target(self, dev):
-        if isinstance(dev, list):
-            self._force_target = dev[:]
-        else:
-            self._force_target.append(dev)
-    def get_force_target(self):
-        return self._force_target
-    force_target = property(get_force_target, set_force_target)
-
-    # List of disk targets that we skip cloning despite Cloner's
-    # recommendation. This takes precedence over force_target.")
-    def set_skip_target(self, dev):
-        if isinstance(dev, list):
-            self._skip_target = dev[:]
-        else:
-            self._skip_target.append(dev)
-    def get_skip_target(self):
-        return self._skip_target
-    skip_target = property(get_skip_target, set_skip_target)
-
-    # List of policy rules for determining which vm disks to clone.
-    # See CLONE_POLICY_*
-    def set_clone_policy(self, policy_list):
-        self._clone_policy = policy_list
-    def get_clone_policy(self):
-        return self._clone_policy
-    clone_policy = property(get_clone_policy, set_clone_policy)
-
-    # Allow cloning a running VM. If enabled, domain state is not
-    # checked before cloning.
-    def get_clone_running(self):
-        return self._clone_running
-    def set_clone_running(self, val):
-        self._clone_running = bool(val)
-    clone_running = property(get_clone_running, set_clone_running)
-
-    # If enabled, don't check for clone name collision, simply undefine
-    # any conflicting guest.
-    def _get_replace(self):
-        return self._replace
-    def _set_replace(self, val):
-        self._replace = bool(val)
-    replace = property(_get_replace, _set_replace)
+        new_name = Cloner.generate_clone_name(self.conn, self.src_name)
+        log.debug("Auto-generated clone name '%s'", new_name)
+        self.set_clone_name(new_name)
 
-    # If true, use COW lightweight copy
-    def _get_reflink(self):
-        return self._reflink
-    def _set_reflink(self, reflink):
-        self._reflink = reflink
-    reflink = property(_get_reflink, _set_reflink)
 
+    ##############
+    # Properties #
+    ##############
 
-    ######################
-    # Functional methods #
-    ######################
+    @property
+    def src_name(self):
+        """
+        The name of the original VM we are cloning
+        """
+        return self._src_guest.name
 
-    def setup_original(self):
+    @property
+    def new_guest(self):
         """
-        Validate and setup all parameters needed for the original (cloned) VM
+        The Guest instance of the new XML we will create
         """
-        log.debug("Validating original guest parameters")
+        return self._new_guest
 
-        if self.original_guest is None and self.original_xml is None:
-            raise RuntimeError(_("Original guest name or XML is required."))
+    def set_clone_name(self, name):
+        self._new_guest.name = name
 
-        if self.original_guest is not None and not self.original_xml:
-            self.original_dom = self._lookup_vm(self.original_guest)
-            flags = libvirt.VIR_DOMAIN_XML_SECURE
-            self.original_xml = self.original_dom.XMLDesc(flags)
+    def set_clone_uuid(self, uuid):
+        """
+        Override the new VMs generated UUId
+        """
+        self._new_guest.uuid = uuid
+
+    def set_replace(self, val):
+        """
+        If True, don't check for clone name collision, simply undefine
+        any conflicting guest.
+        """
+        self._replace = bool(val)
 
-        log.debug("Original XML:\n%s", self.original_xml)
+    def set_reflink(self, reflink):
+        """
+        If true, use COW lightweight copy
+        """
+        self._reflink = reflink
 
-        self._guest = Guest(self.conn, parsexml=self.original_xml)
-        self._guest.id = None
+    def set_sparse(self, flg):
+        """
+        If True, attempt sparse allocation during cloning
+        """
+        self._sparse = flg
 
-        # Pull clonable storage info from the original xml
-        self._original_disks = self._get_original_disks_info()
+    def get_diskinfos(self):
+        """
+        Return the list of _CloneDiskInfo instances
+        """
+        return self._diskinfos[:]
 
-        log.debug("Original paths: %s",
-                      [d.path for d in self.original_disks])
-        log.debug("Original sizes: %s",
-                      [d.get_size() for d in self.original_disks])
+    def get_diskinfos_to_clone(self):
+        """
+        Return a list of _CloneDiskInfo that are tagged for cloning
+        """
+        return [di for di in self.get_diskinfos() if di.is_clone_requested()]
 
-        if not self.clone_running and self.original_dom:
-            status = self.original_dom.info()[0]
-            if status not in [libvirt.VIR_DOMAIN_SHUTOFF]:
-                raise RuntimeError(_("Domain to clone must be shutoff."))
+    def set_nvram_path(self, val):
+        """
+        If the VM needs to have nvram content cloned, this overrides the
+        destination path
+        """
+        self._new_nvram_path = val
 
-    def _setup_disk_clone_destination(self, orig_disk, clone_disk):
+    def set_overwrite(self, flg):
         """
-        Helper that validates the new path location
+        If False, no data is copied to the destination disks by default.
+        Storage may be created, but it is empty.
         """
-        if self.preserve_dest_disks:
-            return
-
-        if clone_disk.get_vol_object():
-            # Special case: non remote cloning of a guest using
-            # managed block devices: fall back to local cloning if
-            # we have permissions to do so. This validation check
-            # caused a few bug reports in a short period of time,
-            # so must be a common case.
-            if (self.conn.is_remote() or
-                clone_disk.type != clone_disk.TYPE_BLOCK or
-                not orig_disk.path or
-                not os.access(orig_disk.path, os.R_OK) or
-                not clone_disk.path or
-                not os.access(clone_disk.path, os.W_OK)):
-                raise RuntimeError(
-                    _("Clone onto existing storage volume is not "
-                      "currently supported: '%s'") % clone_disk.path)
-
-        # Setup proper cloning inputs for the new virtual disks
-        if (orig_disk.get_vol_object() and
-            clone_disk.get_vol_install()):
-            clone_vol_install = clone_disk.get_vol_install()
-
-            # Source and dest are managed. If they share the same pool,
-            # replace vol_install with a CloneVolume instance, otherwise
-            # simply set input_vol on the dest vol_install
-            if (clone_vol_install.pool.name() ==
-                orig_disk.get_parent_pool().name()):
-                vol_install = StorageVolume(self.conn)
-                vol_install.input_vol = orig_disk.get_vol_object()
-                vol_install.sync_input_vol()
-                vol_install.name = clone_vol_install.name
-            else:
-                # Cross pool cloning
-                # Sync only the format of the image.
-                clone_vol_install.input_vol = orig_disk.get_vol_object()
-                vol_install = clone_vol_install
-                vol_install.input_vol = orig_disk.get_vol_object()
-                vol_install.sync_input_vol(only_format=True)
-
-            if not self.clone_sparse:
-                vol_install.allocation = vol_install.capacity
-            vol_install.reflink = self.reflink
-            clone_disk.set_vol_install(vol_install)
-        elif orig_disk.path:
-            clone_disk.set_local_disk_to_clone(orig_disk, self.clone_sparse)
+        self._overwrite = flg
 
-        clone_disk.validate()
 
+    ######################
+    # Functional methods #
+    ######################
 
     def _prepare_nvram(self):
-        if self.clone_nvram is None:
-            nvram_dir = os.path.dirname(self._guest.os.nvram)
-            self.clone_nvram = os.path.join(nvram_dir,
-                                            "%s_VARS.fd" % self._clone_name)
+        new_nvram_path = self._new_nvram_path
+        if new_nvram_path is None:
+            nvram_dir = os.path.dirname(self._new_guest.os.nvram)
+            new_nvram_path = os.path.join(
+                    nvram_dir, "%s_VARS.fd" % self._new_guest.name)
 
         old_nvram = DeviceDisk(self.conn)
-        old_nvram.path = self._guest.os.nvram
-
+        old_nvram.path = self._new_guest.os.nvram
         nvram = DeviceDisk(self.conn)
-        nvram.path = self.clone_nvram
+        nvram.path = new_nvram_path
+        diskinfo = _CloneDiskInfo(old_nvram)
+        allow_create = self._overwrite
 
-        if (not self.preserve_dest_disks and
+        if (allow_create and
             nvram.wants_storage_creation() and
             old_nvram.get_vol_object()):
+            # We only run validation if there's some existing nvram we
+            # can copy. It's valid for nvram to not exist at VM define
+            # time, libvirt will create it for us
+            diskinfo.set_clone_path(new_nvram_path, allow_create, self._sparse)
+            self._nvram_disk = diskinfo.clone_disk
+            self._nvram_disk.get_vol_install().reflink = self._reflink
 
-            nvram_install = DeviceDisk.build_vol_install(
-                    self.conn, os.path.basename(nvram.path),
-                    nvram.get_parent_pool(), nvram.get_size(), False)
-            nvram_install.input_vol = old_nvram.get_vol_object()
-            nvram_install.sync_input_vol(only_format=True)
-            nvram_install.reflink = self.reflink
-            nvram.set_vol_install(nvram_install)
+        self._new_guest.os.nvram = nvram.path
 
-            nvram.validate()
-            self._nvram_disk = nvram
 
-        self._guest.os.nvram = nvram.path
-
-
-    def setup_clone(self):
+    def prepare(self):
         """
         Validate and set up all parameters needed for the new (clone) VM
         """
-        log.debug("Validating clone parameters.")
-
-        self._clone_xml = self.original_xml
-
-        if len(self.clone_disks) < len(self.original_disks):
-            raise ValueError(_("More disks to clone than new paths specified. "
-                               "(%(passed)d specified, %(need)d needed") %
-                               {"passed": len(self.clone_disks),
-                                "need": len(self.original_disks)})
-
-        log.debug("Clone paths: %s", [d.path for d in self.clone_disks])
-
-        self._guest.name = self._clone_name
-        self._guest.uuid = self._clone_uuid
-        self._guest.title = None
+        try:
+            Guest.validate_name(self.conn, self._new_guest.name,
+                                check_collision=not self._replace,
+                                validate=False)
+        except ValueError as e:
+            raise ValueError(_("Invalid name for new guest: %s") % e)
 
-        self._clone_macs.reverse()
-        for dev in self._guest.devices.graphics:
-            if dev.port and dev.port != -1:
-                log.warning(_("Setting the graphics device port to autoport, "
-                               "in order to avoid conflicting."))
-                dev.port = -1
+        for diskinfo in self.get_diskinfos_to_clone():
+            orig_disk = diskinfo.disk
 
-        clone_macs = self._clone_macs[:]
-        for iface in self._guest.devices.interface:
-            iface.target_dev = None
+            if not diskinfo.clone_disk:
+                # User didn't set a path, generate one
+                newpath = Cloner.generate_clone_disk_path(
+                         self.conn, self.src_name,
+                         self.new_guest.name,
+                         orig_disk.path)
+                diskinfo.set_clone_path(newpath,
+                        self._overwrite, self._sparse)
 
-            if clone_macs:
-                mac = clone_macs.pop()
-            else:
-                mac = DeviceInterface.generate_mac(self.conn)
-            iface.macaddr = mac
+            clone_disk = diskinfo.clone_disk
+            assert clone_disk
+            log.debug("Cloning srcpath=%s dstpath=%s",
+                    orig_disk.path, clone_disk.path)
 
-        # Changing storage XML
-        for i, orig_disk in enumerate(self._original_disks):
-            clone_disk = self._clone_disks[i]
+            if self._reflink:
+                vol_install = clone_disk.get_vol_install()
+                vol_install.reflink = self._reflink
 
-            for disk in self._guest.devices.disk:
+            for disk in self._new_guest.devices.disk:
                 if disk.target == orig_disk.target:
                     xmldisk = disk
 
-            self._setup_disk_clone_destination(orig_disk, clone_disk)
-
             # Change the XML
             xmldisk.path = None
             xmldisk.type = clone_disk.type
@@ -444,20 +465,11 @@ class Cloner(object):
             xmldisk.driver_type = orig_disk.driver_type
             xmldisk.path = clone_disk.path
 
-        # For guest agent channel, remove a path to generate a new one with
-        # new guest name
-        for channel in self._guest.devices.channel:
-            if (channel.type == DeviceChannel.TYPE_UNIX and
-                channel.target_name and channel.source.path and
-                channel.target_name in channel.source.path):
-                channel.source.path = None
-
-        if self._guest.os.nvram:
+        if self._new_guest.os.nvram:
             self._prepare_nvram()
 
         # Save altered clone xml
-        self._clone_xml = self._guest.get_xml()
-        log.debug("Clone guest xml is\n%s", self._clone_xml)
+        log.debug("Clone guest xml is\n%s", self._new_guest.get_xml())
 
     def start_duplicate(self, meter=None):
         """
@@ -470,14 +482,15 @@ class Cloner(object):
         dom = None
         try:
             # Replace orig VM if required
-            if self.replace:
-                _replace_vm(self.conn, self.clone_name)
+            if self._replace:
+                _replace_vm(self.conn, self._new_guest.name)
 
             # Define domain early to catch any xml errors before duping storage
-            dom = self.conn.defineXML(self.clone_xml)
+            dom = self.conn.defineXML(self._new_guest.get_xml())
 
-            if self.preserve:
-                for dst_dev in self.clone_disks:
+            if self._overwrite:
+                diskinfos = self.get_diskinfos_to_clone()
+                for dst_dev in [d.clone_disk for d in diskinfos]:
                     dst_dev.build_storage(meter)
                 if self._nvram_disk:
                     self._nvram_disk.build_storage(meter)
@@ -488,134 +501,3 @@ class Cloner(object):
             raise
 
         log.debug("Duplicating finished.")
-
-    def generate_clone_disk_path(self, origpath, newname=None):
-        origname = self.original_guest
-        newname = newname or self.clone_name
-        path = origpath
-        suffix = ""
-
-        # Try to split the suffix off the existing disk name. Ex.
-        # foobar.img -> foobar-clone.img
-        #
-        # If the suffix is greater than 7 characters, assume it isn't
-        # a file extension and is part of the disk name, at which point
-        # just stick '-clone' on the end.
-        if "." in origpath and len(origpath.rsplit(".", 1)[1]) <= 7:
-            path, suffix = origpath.rsplit(".", 1)
-            suffix = "." + suffix
-
-        dirname = os.path.dirname(path)
-        basename = os.path.basename(path)
-
-        clonebase = basename + "-clone"
-        if origname and basename == origname:
-            clonebase = newname
-
-        clonebase = os.path.join(dirname, clonebase)
-        def cb(p):
-            return DeviceDisk.path_definitely_exists(self.conn, p)
-        return generatename.generate_name(clonebase, cb, suffix=suffix)
-
-    def generate_clone_name(self, basename=None):
-        # If the orig name is "foo-clone", we don't want the clone to be
-        # "foo-clone-clone", we want "foo-clone1"
-        if not basename:
-            basename = self.original_guest
-
-        match = re.search("-clone[1-9]*$", basename)
-        start_num = 1
-        force_num = False
-        if match:
-            num_match = re.search("[1-9]+$", match.group())
-            if num_match:
-                start_num = int(str(num_match.group())) + 1
-                force_num = True
-            basename = basename.replace(match.group(), "")
-
-        def cb(n):
-            return generatename.check_libvirt_collision(
-                self.conn.lookupByName, n)
-        basename = basename + "-clone"
-        return generatename.generate_name(basename, cb,
-                sep="", start_num=start_num, force_num=force_num)
-
-
-    ############################
-    # Private helper functions #
-    ############################
-
-    # Parse disk paths that need to be cloned from the original guest's xml
-    # Return a list of DeviceDisk instances pointing to the original
-    # storage
-    def _get_original_disks_info(self):
-        clonelist = []
-        retdisks = []
-
-        for disk in self._guest.devices.disk:
-            if self._do_we_clone_device(disk):
-                clonelist.append(disk)
-                continue
-
-        # Set up virtual disk to encapsulate all relevant path info
-        for disk in clonelist:
-            validate = not self.preserve_dest_disks
-
-            try:
-                device = DeviceDisk.DEVICE_DISK
-                if not disk.path:
-                    # Tell DeviceDisk we are a cdrom to allow empty media
-                    device = DeviceDisk.DEVICE_CDROM
-
-                newd = DeviceDisk(self.conn)
-                newd.path = disk.path
-                newd.device = device
-                newd.driver_name = disk.driver_name
-                newd.driver_type = disk.driver_type
-                newd.target = disk.target
-                if validate:
-                    if newd.wants_storage_creation():
-                        raise ValueError(_("Disk path '%s' does not exist.") %
-                                         newd.path)
-            except Exception as e:
-                log.debug("Exception creating clone disk objects",
-                    exc_info=True)
-                raise ValueError(_("Could not determine original disk "
-                                   "information: %s" % str(e)))
-            retdisks.append(newd)
-
-        return retdisks
-
-    # Pull disk #i from the original guest xml, return it's source path
-    # if it should be cloned
-    # Cloning policy based on 'clone_policy', 'force_target' and 'skip_target'
-    def _do_we_clone_device(self, disk):
-        if disk.target in self.skip_target:
-            return False
-
-        if disk.target in self.force_target:
-            return True
-
-        # No media path
-        if (not disk.path and
-            self.CLONE_POLICY_NO_EMPTYMEDIA in self.clone_policy):
-            return False
-
-        # Readonly disks
-        if (disk.read_only and
-            self.CLONE_POLICY_NO_READONLY in self.clone_policy):
-            return False
-
-        # Shareable disks
-        if (disk.shareable and
-            self.CLONE_POLICY_NO_SHAREABLE in self.clone_policy):
-            return False
-
-        return True
-
-    # Simple wrapper for checking a vm exists and returning the domain
-    def _lookup_vm(self, name):
-        try:
-            return self.conn.lookupByName(name)
-        except libvirt.libvirtError:
-            raise ValueError(_("Domain '%s' was not found.") % str(name))