Change in vdsm[master]: External hypervisor VMs integration

nsoffer at redhat.com nsoffer at redhat.com
Mon Nov 3 00:34:27 UTC 2014


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 <shavivi at redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list