Nir Soffer has posted comments on this change. Change subject: External hypervisor VMs integration ......................................................................
Patch Set 12: (12 comments) Added few suggestions for making the code nicer. http://gerrit.ovirt.org/#/c/33309/12/vdsm/API.py File vdsm/API.py: Line 1384: def getExternalVMList(self, uri, username, password): Line 1385: "return information about the not-KVM virtual machines" Line 1386: vms = v2v.getExternalVMList(uri, username, password) Line 1387: if vms is None: Line 1388: return errCode['externalErr'] Returning None for error is little ugly - why not raise an exception with the underling error name, so this can be: try: vms = v2v.getExternalVMList(uri, username, password) except v2v.Error as e: return errCode[e.name] Line 1389: return {'status': doneCode, 'vmList': vms} Line 1390: Line 1391: # Networking-related functions Line 1392: def setupNetworks(self, networks, bondings, options): http://gerrit.ovirt.org/#/c/33309/12/vdsm/rpc/Bridge.py File vdsm/rpc/Bridge.py: Line 329: """ Line 330: Return list of VMs from non-KVM source Line 331: """ Line 332: uri = args.get('uri', None) Line 333: username = args.get('username', None) Noe is the default value, there is no need to repeat that here. "dict.get(key)" is the idomatic usage, everyone knows that this may return None. Line 334: password = args.get('password', None) Line 335: return API.Global().getExternalVMList(uri, username, password) Line 336: Line 337: http://gerrit.ovirt.org/#/c/33309/12/vdsm/rpc/vdsmapi-schema.json File vdsm/rpc/vdsmapi-schema.json: Line 3630: # @ExternalVmInfo: Line 3631: # Line 3632: # map of VM properties Line 3633: # Line 3634: # @vmName: The name of the VM Why not "name"? (ExternalVmInfo.vmName - do we have another type of name here?) Line 3635: # Line 3636: # @status: The VM status (up, down, etc) Line 3637: # Line 3638: # @vmId: The VM uuid Line 3632: # map of VM properties Line 3633: # Line 3634: # @vmName: The name of the VM Line 3635: # Line 3636: # @status: The VM status (up, down, etc) Here you agree with me that there is no need to repeat the "vm" prefix :-) Line 3637: # Line 3638: # @vmId: The VM uuid Line 3639: # Line 3640: # @smp: number of cpu Line 3634: # @vmName: The name of the VM Line 3635: # Line 3636: # @status: The VM status (up, down, etc) Line 3637: # Line 3638: # @vmId: The VM uuid Why not "id"? Line 3639: # Line 3640: # @smp: number of cpu Line 3641: # Line 3642: # @memSize: size of actual memory in MB Line 3642: # @memSize: size of actual memory in MB Line 3643: # Line 3644: # @disks: list of disk devices Line 3645: # Line 3646: # @networks: list of network devices Since most names do not use "vm" prefix, lets remove all the prefixes. Line 3647: # Line 3648: # Since: 4.17.0 Line 3649: ## Line 3650: {'type': 'ExternalVmInfo', http://gerrit.ovirt.org/#/c/33309/12/vdsm/v2v.py File vdsm/v2v.py: Line 32: libvirt.VIR_DOMAIN_CRASHED: 'Crushed', Line 33: libvirt.VIR_DOMAIN_PMSUSPENDED: 'PMSuspended'} Line 34: Line 35: Line 36: def _hypervisorConnect(uri, username, password): This connects to another hypervisor using libvirt? Seem that this function does nothing, and we can just inline it in the single call site. Or maybe another name will make this more clear. Line 37: conn = libvirtconnection.open_connection(uri=uri, username=username, Line 38: passwd=password) Line 39: return conn Line 40: Line 35: Line 36: def _hypervisorConnect(uri, username, password): Line 37: conn = libvirtconnection.open_connection(uri=uri, username=username, Line 38: passwd=password) Line 39: return conn Why not: return libvirtconnection.open_connection(...) Line 40: Line 41: Line 42: def _memToMb(size, unit): Line 43: if unit == 'bytes' or unit == 'b': Line 51: elif unit == 'Tib' or unit == 'T': Line 52: return size * 1024 * 1024 Line 53: Line 54: Line 55: def _parseVmElements(xml): This does not get elements - right? it accepts an xml document. How about naming it _parseVmXml? Line 56: ret = {} Line 57: root = ET.fromstring(xml) Line 58: Line 59: ''' VM ID ''' Line 88: if source is not None and 'file' in source.attrib: Line 89: d['alias'] = source.attrib['file'] Line 90: ret['disks'].append(d) Line 91: Line 92: ''' Network ''' The need to add these (non-standard) comments (should use # Networks) show that this should be a separate function. Line 93: ret['networks'] = [] Line 94: interfaces = root.findall('.//interface') Line 95: for iface in interfaces: Line 96: i = {} Line 108: ret['networks'].append(i) Line 109: return ret Line 110: Line 111: Line 112: def getExternalVMList(uri, username, password): This is the only public API here - why not put this on the top of the file? Why scroll to the bottom for the most important part? Line 113: try: Line 114: conn = _hypervisorConnect(uri, username, password) Line 115: if conn is None: Line 116: return None Line 118: ret = [] Line 119: for vm in conn.listAllDomains(): Line 120: params = _parseVmElements(vm.XMLDesc(0)) Line 121: params['vmName'] = vm.name() Line 122: params['status'] = _VM_STATUS[vm.state()[0]] There are 2 functions here building the params dict - this function, and _parseVmElements. It would be nicer if there is only once place building the dict. _parseVmElement is also much too long and hard to read. Both issues can be fix by: 1. Parsing the xml here 2. Adding couple of small functions that accept the parsed xml (root), and return relevant info. 3. Put all the code that build params in one place: root = root = ET.fromstring(xml) for vm in ...: params = {} params["name"] = vm.name() params["networks"] = _findNetworks(root) ... And maybe we can reuse such functions from other modules? there must be some duplication between this module and existing virt modules. Line 123: ret.append(params) Line 124: return ret Line 125: finally: -- To view, visit http://gerrit.ovirt.org/33309 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7dcfb860626a844d1d08590274b508519a33f4a3 Gerrit-PatchSet: 12 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
