Nir Soffer has posted comments on this change. Change subject: External hypervisor VMs integration ......................................................................
Patch Set 16: (4 comments) http://gerrit.ovirt.org/#/c/33309/16/vdsm/v2v.py File vdsm/v2v.py: Line 63: return size * 1024 Line 64: elif unit in ('TiB', 'T'): Line 65: return size * 1024 * 1024 Line 66: else: Line 67: raise InvalidVMConfigurationException('Invalid memory unit: ' + unit) Better use "description: %r" % value, so we can see what was the bad value. For example, if it was empty string we will see: Invalid memory unit: '' Which is more helpful then: Invalid memory unit: Same comment apply to other exceptions bellow Line 68: Line 69: Line 70: def _addGeneralInfo(root, params): Line 71: e = root.find('./uuid') Line 75: e = root.find('./currentMemory') Line 76: if e is not None: Line 77: try: Line 78: size = int(e.text) Line 79: except: Handle only ValueError - any other error is not expected. Same apply to other int() calls bellow Line 80: raise InvalidVMConfigurationException('Invalid memory value: ' + Line 81: e.text) Line 82: unit = e.get('unit', 'KiB') Line 83: params['memSize'] = _memToMb(size, unit) Line 123: i['dev'] = target.get('dev') Line 124: params['networks'].append(i) Line 125: Line 126: Line 127: class InvalidVMConfigurationException(Exception): No need to add "Exception" to the name. What elese can be "InvalidVMConfiguration"? Line 128: def __init__(self, message): Line 129: self.message = message Line 130: Line 131: def __str__(self): Line 128: def __init__(self, message): Line 129: self.message = message Line 130: Line 131: def __str__(self): Line 132: return self.message It is better not to implement __str__ since Exception builtin implementation is better - it will show the class name in a traceback or when you log the instance. For rexample: >>> class InvalidVMConfiguration(Exception): ... pass ... >>> raise InvalidVMConfiguration("description") Traceback (most recent call last): File "<stdin>", line 1, in <module> v2v.InvalidVMConfiguration: description If you don't implement __str__, there is no need to implement __init__. -- 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: 16 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi <[email protected]> Gerrit-Reviewer: Antoni Segura Puimedon <[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
