From b3ff59c75ca5958fae861d13c92bcbb0e025ec66 Mon Sep 17 00:00:00 2001 From: Cole Robinson Date: Wed, 11 Nov 2020 09:41:47 -0500 Subject: [PATCH] device: disk: Move XML handling to its own class Makes DeviceDisk less complicated, helps with readability Signed-off-by: Cole Robinson --- tests/test_xmlparse.py | 20 +-- virtinst/cli.py | 32 ++-- virtinst/devices/disk.py | 289 ++++++++++++++++++---------------- virtinst/install/installer.py | 2 +- 4 files changed, 183 insertions(+), 160 deletions(-) diff --git a/tests/test_xmlparse.py b/tests/test_xmlparse.py index 0b230394..6a470bb5 100644 --- a/tests/test_xmlparse.py +++ b/tests/test_xmlparse.py @@ -339,22 +339,24 @@ def testAlterDisk(): disk = _get_disk("vdb") check = _make_checker(disk) - check("source_pool", "defaultPool", "anotherPool") - check("source_volume", "foobar", "newvol") + check("source.pool", "defaultPool", "anotherPool") + check("source.volume", "foobar", "newvol") disk = _get_disk("vdc") check = _make_checker(disk) - check("source_protocol", "rbd", "gluster") - check("source_name", "pool/image", "new-val/vol") - check("source_host_name", "mon1.example.org", "diff.example.org") - check("source_host_port", 6321, 1234) + check("source.protocol", "rbd", "gluster") + check("source.name", "pool/image", "new-val/vol") + hcheck = _make_checker(disk.source.hosts[0]) + hcheck("name", "mon1.example.org", "diff.example.org") + hcheck("port", 6321, 1234) check("path", "gluster://diff.example.org:1234/new-val/vol") disk = _get_disk("vdd") check = _make_checker(disk) - check("source_protocol", "nbd") - check("source_host_transport", "unix") - check("source_host_socket", "/var/run/nbdsock") + hcheck = _make_checker(disk.source.hosts[0]) + check("source.protocol", "nbd") + hcheck("transport", "unix") + hcheck("socket", "/var/run/nbdsock") check("path", "nbd+unix:///var/run/nbdsock") _alter_compare(conn, guest.get_xml(), outfile) diff --git a/virtinst/cli.py b/virtinst/cli.py index cdc43baa..301ac61c 100644 --- a/virtinst/cli.py +++ b/virtinst/cli.py @@ -3355,8 +3355,8 @@ class ParserDisk(VirtCLIParser): ################### def host_find_inst_cb(self, *args, **kwargs): - cliarg = "listens" # host[0-9]* - list_propname = "hosts" # disk.hosts + cliarg = "hosts" # host[0-9]* + list_propname = "source.hosts" # disk.hosts cb = self._make_find_inst_cb(cliarg, list_propname) return cb(*args, **kwargs) @@ -3381,22 +3381,22 @@ class ParserDisk(VirtCLIParser): cls.add_arg("cache", "driver_cache", cb=cls.noset_cb) # More standard XML props - cls.add_arg("source.dir", "source_dir") - cls.add_arg("source.file", "source_file") - cls.add_arg("source.dev", "source_dev") - cls.add_arg("source.pool", "source_pool") - cls.add_arg("source.volume", "source_volume") - cls.add_arg("source.name", "source_name") - cls.add_arg("source.protocol", "source_protocol") + cls.add_arg("source.dir", "source.dir") + cls.add_arg("source.file", "source.file") + cls.add_arg("source.dev", "source.dev") + cls.add_arg("source.pool", "source.pool") + cls.add_arg("source.volume", "source.volume") + cls.add_arg("source.name", "source.name") + cls.add_arg("source.protocol", "source.protocol") cls.add_arg("source.startupPolicy", "startup_policy") # type=nvme source props - cls.add_arg("source.type", "source_type") - cls.add_arg("source.namespace", "source_namespace") - cls.add_arg("source.managed", "source_managed", is_onoff=True) - cls.add_arg("source.address.domain", "source_address.domain") - cls.add_arg("source.address.bus", "source_address.bus") - cls.add_arg("source.address.slot", "source_address.slot") - cls.add_arg("source.address.function", "source_address.function") + cls.add_arg("source.type", "source.type") + cls.add_arg("source.namespace", "source.namespace") + cls.add_arg("source.managed", "source.managed", is_onoff=True) + cls.add_arg("source.address.domain", "source.address.domain") + cls.add_arg("source.address.bus", "source.address.bus") + cls.add_arg("source.address.slot", "source.address.slot") + cls.add_arg("source.address.function", "source.address.function") cls.add_arg("source.host[0-9]*.name", "name", find_inst_cb=cls.host_find_inst_cb) diff --git a/virtinst/devices/disk.py b/virtinst/devices/disk.py index 412216bd..134d5547 100644 --- a/virtinst/devices/disk.py +++ b/virtinst/devices/disk.py @@ -44,6 +44,136 @@ class _DiskSourceAddress(DeviceAddress): pass +class _DiskSource(XMLBuilder): + """ + Class representing disk block, and various helpers + that only operate on contents + """ + _XML_PROP_ORDER = [ + "file", "dev", "dir", + "volume", "pool", "protocol", "name", "hosts", + "type", "managed", "namespace", "address"] + XML_NAME = "source" + + file = XMLProperty("./@file") + dev = XMLProperty("./@dev") + dir = XMLProperty("./@dir") + + pool = XMLProperty("./@pool") + volume = XMLProperty("./@volume") + + hosts = XMLChildProperty(_Host) + + name = XMLProperty("./@name") + protocol = XMLProperty("./@protocol") + + type = XMLProperty("./@type") + managed = XMLProperty("./@managed", is_yesno=True) + namespace = XMLProperty("./@namespace", is_int=True) + address = XMLChildProperty(_DiskSourceAddress, is_single=True) + + def set_from_url(self, uri): + """ + For a passed in path URI like gluster:// or https://, split it + up and set the properties directly + """ + from ..uri import URI + uriobj = URI(uri) + + if uriobj.scheme: + self.protocol = uriobj.scheme + if ((uriobj.hostname or uriobj.port or uriobj.transport) and + not self.hosts): + self.hosts.add_new() + if uriobj.transport: + self.hosts[0].transport = uriobj.transport + if uriobj.hostname: + self.hosts[0].name = uriobj.hostname + if uriobj.port: + self.hosts[0].port = uriobj.port + if uriobj.path: + if self.hosts and self.hosts[0].transport: + self.hosts[0].socket = uriobj.path + else: + self.name = uriobj.path + if self.name.startswith("/"): + self.name = self.name[1:] + + def build_url_from_network(self): + """ + Build a URL from network contents of + """ + host = _Host(self.conn) + if self.hosts: + host = self.hosts[0] + + ret = self.protocol or "unknown" + if host.transport: + ret += "+%s" % host.transport + ret += "://" + if host.name: + ret += host.name + if host.port: + ret += ":" + str(host.port) + if self.name: + if not self.name.startswith("/"): + ret += "/" + ret += self.name + elif host.socket: + if not host.socket.startswith("/"): + ret += "/" + ret += host.socket + return ret + + def clear_source(self): + """ + Unset all XML properties that describe the actual source media + """ + self.file = None + self.dev = None + self.dir = None + self.volume = None + self.pool = None + self.name = None + self.protocol = None + for h in self.hosts[:]: + self.remove_child(h) + + def set_network_from_storage(self, volxml, poolxml): + """ + For the passed pool + vol object combo representing a network + volume, set the elements directly + """ + is_iscsi_direct = poolxml.type == "iscsi-direct" + protocol = poolxml.type + if is_iscsi_direct: + protocol = "iscsi" + + self.protocol = protocol + for idx, poolhost in enumerate(poolxml.hosts): + if len(self.hosts) < (idx + 1): + self.hosts.add_new() + self.hosts[idx].name = poolhost.name + self.hosts[idx].port = poolhost.port + + path = "" + if is_iscsi_direct: + # Vol path is like this: + # ip-10.66.144.87:3260-iscsi-iqn.2017-12.com.virttest:emulated-iscsi-noauth.target2-lun-1 + # Always seems to have -iscsi- embedded in it + if "-iscsi-iqn." in volxml.target_path: + path = volxml.target_path.split("-iscsi-", 1)[-1] + else: + if poolxml.source_name: + path += poolxml.source_name + if poolxml.source_path: + path += poolxml.source_path + if not path.endswith('/'): + path += "/" + path += volxml.name + self.name = path or None + + class DeviceDisk(Device): XML_NAME = "disk" @@ -299,12 +429,8 @@ class DeviceDisk(Device): "driver_name", "driver_type", "driver_cache", "driver_discard", "driver_detect_zeroes", "driver_io", "error_policy", - "source_file", "source_dev", "source_dir", "auth_username", "auth_secret_type", "auth_secret_uuid", - "source_volume", "source_pool", "source_protocol", "source_name", - "source_host_name", "source_host_port", - "source_host_transport", "source_host_socket", - "source_type", "source_managed", "source_namespace", "source_address", + "source", "target", "bus", ] @@ -412,90 +538,16 @@ class DeviceDisk(Device): # XML source media handling # ############################# - source_file = XMLProperty("./source/@file") - source_dev = XMLProperty("./source/@dev") - source_dir = XMLProperty("./source/@dir") - - source_pool = XMLProperty("./source/@pool") - source_volume = XMLProperty("./source/@volume") - - auth_username = XMLProperty("./auth/@username") - auth_secret_type = XMLProperty("./auth/secret/@type") - auth_secret_uuid = XMLProperty("./auth/secret/@uuid") - - hosts = XMLChildProperty(_Host, relative_xpath="./source") - - source_name = XMLProperty("./source/@name") - source_protocol = XMLProperty("./source/@protocol") - # Technically multiple host lines can be listed - source_host_name = XMLProperty("./source/host/@name") - source_host_port = XMLProperty("./source/host/@port", is_int=True) - source_host_transport = XMLProperty("./source/host/@transport") - source_host_socket = XMLProperty("./source/host/@socket") - - source_type = XMLProperty("./source/@type") - source_managed = XMLProperty("./source/@managed", is_yesno=True) - source_namespace = XMLProperty("./source/@namespace", is_int=True) - source_address = XMLChildProperty( - _DiskSourceAddress, is_single=True, relative_xpath="./source") - - def _set_source_network_from_url(self, uri): - from ..uri import URI - uriobj = URI(uri) - - if uriobj.scheme: - self.source_protocol = uriobj.scheme - if uriobj.transport: - self.source_host_transport = uriobj.transport - if uriobj.hostname: - self.source_host_name = uriobj.hostname - if uriobj.port: - self.source_host_port = uriobj.port - if uriobj.path: - if self.source_host_transport: - self.source_host_socket = uriobj.path - else: - self.source_name = uriobj.path - if self.source_name.startswith("/"): - self.source_name = self.source_name[1:] + source = XMLChildProperty(_DiskSource, is_single=True) def _set_source_network_from_storage(self, volxml, poolxml): - is_iscsi_direct = poolxml.type == "iscsi-direct" - protocol = poolxml.type - if is_iscsi_direct: - protocol = "iscsi" - - self.source_protocol = protocol + self.type = "network" if poolxml.auth_type: self.auth_username = poolxml.auth_username self.auth_secret_type = poolxml.auth_type self.auth_secret_uuid = poolxml.auth_secret_uuid - if poolxml.hosts: - self.source_host_name = poolxml.hosts[0].name - self.source_host_port = poolxml.hosts[0].port - for host in poolxml.hosts: - obj = self.hosts.add_new() - obj.name = host.name - obj.port = host.port - path = "" - if is_iscsi_direct: - # Vol path is like this: - # ip-10.66.144.87:3260-iscsi-iqn.2017-12.com.virttest:emulated-iscsi-noauth.target2-lun-1 - # Always seems to have -iscsi- embedded in it - if "-iscsi-iqn." in volxml.target_path: - path = volxml.target_path.split("-iscsi-", 1)[-1] - else: - if poolxml.source_name: - path += poolxml.source_name - if poolxml.source_path: - path += poolxml.source_path - if not path.endswith('/'): - path += "/" - path += volxml.name - self.source_name = path or None - - self.type = "network" + self.source.set_network_from_storage(volxml, poolxml) def _set_network_source_from_backend(self): if (self._storage_backend.get_vol_object() or @@ -504,60 +556,26 @@ class DeviceDisk(Device): poolxml = self._storage_backend.get_parent_pool_xml() self._set_source_network_from_storage(volxml, poolxml) elif self._storage_backend.get_path(): - self._set_source_network_from_url(self._storage_backend.get_path()) - - def _build_url_from_network_source(self): - ret = self.source_protocol or "unknown" - if self.source_host_transport: - ret += "+%s" % self.source_host_transport - ret += "://" - if self.source_host_name: - ret += self.source_host_name - if self.source_host_port: - ret += ":" + str(self.source_host_port) - if self.source_name: - if not self.source_name.startswith("/"): - ret += "/" - ret += self.source_name - elif self.source_host_socket: - if not self.source_host_socket.startswith("/"): - ret += "/" - ret += self.source_host_socket - return ret + self.source.set_from_url(self._storage_backend.get_path()) def _get_default_type(self): - if self.source_pool or self.source_volume: + if self.source.pool or self.source.volume: return DeviceDisk.TYPE_VOLUME if not self._storage_backend.is_stub(): return self._storage_backend.get_dev_type() - if self.source_protocol: + if self.source.protocol: return DeviceDisk.TYPE_NETWORK return self.TYPE_FILE - def _clear_source_xml(self): - """ - Unset all XML properties that describe the actual source media - """ - self.source_file = None - self.source_dev = None - self.source_dir = None - self.source_volume = None - self.source_pool = None - self.source_name = None - self.source_protocol = None - self.source_host_name = None - self.source_host_port = None - self.source_host_transport = None - self.source_host_socket = None def _disk_type_to_object_prop_name(self): disk_type = self.type if disk_type == DeviceDisk.TYPE_BLOCK: - return "source_dev" + return "dev" elif disk_type == DeviceDisk.TYPE_DIR: - return "source_dir" + return "dir" elif disk_type == DeviceDisk.TYPE_FILE: - return "source_file" + return "file" return None @@ -565,15 +583,15 @@ class DeviceDisk(Device): # they don't have any special properties aside from needing to match # 'type' value with the source property used. def _get_xmlpath(self): - if self.source_file: - return self.source_file - if self.source_dev: - return self.source_dev - if self.source_dir: - return self.source_dir + if self.source.file: + return self.source.file + if self.source.dev: + return self.source.dev + if self.source.dir: + return self.source.dir return None def _set_xmlpath(self, val): - self._clear_source_xml() + self.source.clear_source() if self._storage_backend.get_dev_type() == "network": self._set_network_source_from_backend() @@ -582,7 +600,7 @@ class DeviceDisk(Device): propname = self._disk_type_to_object_prop_name() if not propname: return - return setattr(self, propname, val) + return setattr(self.source, propname, val) ################## @@ -611,6 +629,9 @@ class DeviceDisk(Device): driver_name = XMLProperty("./driver/@name") driver_type = XMLProperty("./driver/@type") + auth_username = XMLProperty("./auth/@username") + auth_secret_type = XMLProperty("./auth/secret/@type") + auth_secret_uuid = XMLProperty("./auth/secret/@uuid") snapshot_policy = XMLProperty("./@snapshot") @@ -671,18 +692,18 @@ class DeviceDisk(Device): if self.type == DeviceDisk.TYPE_NETWORK: # Fill in a completed URL for virt-manager UI, path comparison, etc - path = self._build_url_from_network_source() + path = self.source.build_url_from_network() if typ == DeviceDisk.TYPE_VOLUME: try: parent_pool = self.conn.storagePoolLookupByName( - self.source_pool) + self.source.pool) vol_object = parent_pool.storageVolLookupByName( - self.source_volume) + self.source.volume) except Exception as e: self._source_volume_err = str(e) log.debug("Error fetching source pool=%s vol=%s", - self.source_pool, self.source_volume, exc_info=True) + self.source.pool, self.source.volume, exc_info=True) if vol_object is None and path is None: path = self._get_xmlpath() diff --git a/virtinst/install/installer.py b/virtinst/install/installer.py index ee5bc83c..28b14397 100644 --- a/virtinst/install/installer.py +++ b/virtinst/install/installer.py @@ -187,7 +187,7 @@ class Installer(object): if self.conn.in_testsuite(): # Hack to set just the XML path differently for the test suite. # Setting this via regular 'path' will error that it doesn't exist - dev.source_file = _make_testsuite_path(location) + dev.source.file = _make_testsuite_path(location) def _remove_unattended_install_cdrom_device(self, guest): dummy = guest