Change in vdsm[master]: v2v: refactor get_external_vms

shavivi at redhat.com shavivi at redhat.com
Wed Dec 23 12:37:34 UTC 2015


Shahar Havivi has posted comments on this change.

Change subject: v2v: refactor get_external_vms
......................................................................


Patch Set 5:

(8 comments)

https://gerrit.ovirt.org/#/c/49636/5/vdsm/v2v.py
File vdsm/v2v.py:

Line 646:     try:
Line 647:         try:
Line 648:             _add_vm_info(vm, params)
Line 649:         except libvirt.libvirtError as e:
Line 650:             logging.exception("error getting domain information")
> Logging error should be good enough, and including the error in the log wil
well it conflict with what Francesco asked here when then patch is made:
https://gerrit.ovirt.org/#/c/49507/4/vdsm/v2v.py@158
Line 651:             return
Line 652:         try:
Line 653:             xml = vm.XMLDesc(0)
Line 654:         except libvirt.libvirtError as e:


Line 652:         try:
Line 653:             xml = vm.XMLDesc(0)
Line 654:         except libvirt.libvirtError as e:
Line 655:             logging.error("error getting domain xml for vm %r: %s",
Line 656:                           vm.name(), e)
> return here
Done
Line 657:         else:
Line 658:             try:
Line 659:                 root = ET.fromstring(xml)
Line 660:             except ET.ParseError as e:


Line 653:             xml = vm.XMLDesc(0)
Line 654:         except libvirt.libvirtError as e:
Line 655:             logging.error("error getting domain xml for vm %r: %s",
Line 656:                           vm.name(), e)
Line 657:         else:
> Avoid this nesting.
Done
Line 658:             try:
Line 659:                 root = ET.fromstring(xml)
Line 660:             except ET.ParseError as e:
Line 661:                 logging.exception('error parsing domain xml')


Line 657:         else:
Line 658:             try:
Line 659:                 root = ET.fromstring(xml)
Line 660:             except ET.ParseError as e:
Line 661:                 logging.exception('error parsing domain xml')
> logging.error would be good enough, we don't care about the internals of ET
Done
Line 662:                 return
Line 663:             try:
Line 664:                 _add_general_info(root, params)
Line 665:             except InvalidVMConfiguration as e:


Line 662:                 return
Line 663:             try:
Line 664:                 _add_general_info(root, params)
Line 665:             except InvalidVMConfiguration as e:
Line 666:                 logging.exception('error parsing domain xml, msg: %s  xml: %s',
> Is this really an error parsing the xml, or invalid configuration?
Done
Line 667:                                   e.message, vm.XMLDesc(0))
Line 668:                 return
Line 669:             else:
Line 670:                 _add_networks(root, params)


Line 663:             try:
Line 664:                 _add_general_info(root, params)
Line 665:             except InvalidVMConfiguration as e:
Line 666:                 logging.exception('error parsing domain xml, msg: %s  xml: %s',
Line 667:                                   e.message, vm.XMLDesc(0))
> Don't use e.message, it is undocumented and deprecated. Simply use e.
Done
Line 668:                 return
Line 669:             else:
Line 670:                 _add_networks(root, params)
Line 671:                 _add_disks(root, params)


Line 665:             except InvalidVMConfiguration as e:
Line 666:                 logging.exception('error parsing domain xml, msg: %s  xml: %s',
Line 667:                                   e.message, vm.XMLDesc(0))
Line 668:                 return
Line 669:             else:
> You return on error, so the else is not needed.
Done
Line 670:                 _add_networks(root, params)
Line 671:                 _add_disks(root, params)
Line 672:                 for disk in params['disks']:
Line 673:                     _add_disk_info(conn, disk)


Line 671:                 _add_disks(root, params)
Line 672:                 for disk in params['disks']:
Line 673:                     _add_disk_info(conn, disk)
Line 674:                 vms.append(params)
Line 675:     except libvirt.libvirtError as e:
> Do we need this huge try block? maybe you can limit it to the methods that 
now that we surrounded all the internal code there is no need to the outer try/raise,
I will remove it
Line 676:         logging.error('error parsing domain: %s', e)
Line 677: 
Line 678: 
Line 679: def _add_vm_info(vm, params):


-- 
To view, visit https://gerrit.ovirt.org/49636
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If5c331b3136b61b0ee81829d7e71eedbbc1fa6b9
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list