Nir Soffer has posted comments on this change. Change subject: External hypervisor VMs integration ......................................................................
Patch Set 27: Code-Review-1 (7 comments) http://gerrit.ovirt.org/#/c/33309/27/vdsm/v2v.py File vdsm/v2v.py: Line 22: from virt import vmstatus Line 23: from vdsm import libvirtconnection Line 24: Line 25: Line 26: class InvalidVMConfiguration(ValueError): Subclassing ValueError make it possible that code looking for ValueError (stdlib error) will catch your exception (v2v error) by mistake: try: ... except ValueError: # handle stdlib error Mixing stdlib errors and application errors is bad practice that we should eliminate from vdsm. Application errors should subclass from Exception. Line 27: ''' Unexpected value while parsing libvirt domain xml ''' Line 28: def __init__(self, field_name, bad_value): Line 29: ValueError.__init__(self, "Invalid %s value: '%r'" % (field_name, Line 30: bad_value)) Line 25: Line 26: class InvalidVMConfiguration(ValueError): Line 27: ''' Unexpected value while parsing libvirt domain xml ''' Line 28: def __init__(self, field_name, bad_value): Line 29: ValueError.__init__(self, "Invalid %s value: '%r'" % (field_name, %r is used to show a Python representation of the value - you don't need to quote it: >>> print "'%r'" % "3.14" ''3.14'' >>> print "%r" % "3.14" '3.14' Line 30: bad_value)) Line 31: Line 32: Line 33: def getExternalVMs(uri, username, password): Line 43: params['status'] = vmstatus.statuses[vm.state()[0]] Line 44: try: Line 45: _addGeneralInfo(root, params) Line 46: except InvalidVMConfiguration: Line 47: logging.error('v2v::getExternalVMs: error parsing domain xml,' We don't need to log the module and function name, since vdsm log is configured to log the module name, function name. Please check your log message in real logs as part of the verification. Line 48: 'msg: %s xml: %s', vm.XMLDesc(0)) Line 49: _addDisks(root, params) Line 50: _addNetworks(root, params) Line 51: ret.append(params) Line 44: try: Line 45: _addGeneralInfo(root, params) Line 46: except InvalidVMConfiguration: Line 47: logging.error('v2v::getExternalVMs: error parsing domain xml,' Line 48: 'msg: %s xml: %s', vm.XMLDesc(0)) A space before "msg" is missing - this will log: error parsing domain xml,msg: ... Line 49: _addDisks(root, params) Line 50: _addNetworks(root, params) Line 51: ret.append(params) Line 52: return ret Line 64: return size * 1024 Line 65: elif lunit in ('tib', 't'): Line 66: return size * 1024 * 1024 Line 67: else: Line 68: raise ValueError("Invalid memory unit: '%r'", unit) Did you mean InvalidVMConfiguration(...)? >>> raise ValueError("Invalid memory unit: '%r'", "foo") Traceback (most recent call last): File "<stdin>", line 1, in <module> ValueError: ("Invalid memory unit: '%r'", 'foo') Line 69: Line 70: Line 71: def _addGeneralInfo(root, params): Line 72: e = root.find('./uuid') Line 77: if e is not None: Line 78: try: Line 79: size = int(e.text) Line 80: except ValueError: Line 81: raise InvalidVMConfiguration("memory value", e.text) This will create the error message (assuming that e.text is 3.14): "Invalid memory value value: ... Should be: raise InvalidVMConfiguration("memory", e.text) Or better: InvalidVMConfiguration("currentMemory", e.text) Because we want to make it easy to debug. Line 82: unit = e.get('unit', 'KiB') Line 83: params['memSize'] = _memToMb(size, unit) Line 84: Line 85: e = root.find('./vcpu') Line 86: if e is not None: Line 87: try: Line 88: params['smp'] = int(e.text) Line 89: except ValueError: Line 90: raise InvalidVMConfiguration("vcpu value", e.text) Same Line 91: Line 92: Line 93: def _addDisks(root, params): Line 94: params['disks'] = [] -- 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: 27 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Antoni Segura Puimedon <asegu...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczew...@gmail.com> Gerrit-Reviewer: Saggi Mizrahi <smizr...@redhat.com> Gerrit-Reviewer: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
