Nir Soffer has posted comments on this change. Change subject: v2v: add volume size to disk info ......................................................................
Patch Set 4: (4 comments) http://gerrit.ovirt.org/#/c/36263/4/vdsm/v2v.py File vdsm/v2v.py: Line 49 Line 50 Line 51 Line 52 Line 53 Idea for another patch - the code getting info from the vm object can be extracted to new function to keep this function more clear. Line 19: import xml.etree.ElementTree as ET Line 20: from contextlib import closing Line 21: Line 22: import logging Line 23: from vdsm.define import errCode This should be bellow near the other "from vdsm ..." import. Line 24: import caps Line 25: import libvirt Line 26: from vdsm import libvirtconnection Line 27: Line 107: if e is not None: Line 108: params['arch'] = e.get('arch') Line 109: Line 110: Line 111: def _add_disks(root, params, conn): It is little ugly that this function mix extracting info from xml and performing additional libvirt requests. How about separating the query to new function, and invoke it from get_external_vms after we finished extracting info from the xml? Also, do we really need to collect size of all disks on all vms, or we can query this separately later? Line 112: params['disks'] = [] Line 113: disks = root.findall('.//disk[@type="file"]') Line 114: for disk in disks: Line 115: d = {} Line 123: vol = conn.storageVolLookupByPath(source.get('file')) Line 124: d['truesize'] = str(vol.info()[1]) Line 125: except libvirt.libvirtError as e: Line 126: logging.error('error parsing storage size, msg: %s xml: %s', Line 127: e.message) > I see two '%s' in the format string, but just one more argument. Also there is no need to access e.message, just format e as string: "error parsing volume size: %s" % e Line 128: params['disks'].append(d) Line 129: Line 130: Line 131: def _add_networks(root, params): -- To view, visit http://gerrit.ovirt.org/36263 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic7e9ba73514292cc2bb4a025d286e2c732e88a75 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Shahar Havivi <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
