Nir Soffer has posted comments on this change. Change subject: v2v: externalVMList Xen+Kvm support ......................................................................
Patch Set 21: (5 comments) https://gerrit.ovirt.org/#/c/48672/21/vdsm/v2v.py File vdsm/v2v.py: Line 648: for name in conn.listDefinedDomains(): Line 649: try: Line 650: vm = conn.lookupByName(name) Line 651: except libvirt.libvirtError as e: Line 652: logging.error('Error in lookupByName: %s', e) Including the error message is great, but you should show also the name of the vm that failed, since we don't know if libvirt erorr message will include it. The common way to do this is something like: logging.error("Error looking up vm %r: %s", name, e) Line 653: else: Line 654: seen.add(name) Line 655: yield vm Line 656: for vmid in conn.listDomainsID(): Line 656: for vmid in conn.listDomainsID(): Line 657: try: Line 658: vm = conn.lookupByID(vmid) Line 659: except libvirt.libvirtError as e: Line 660: logging.exception('Error in listDomainsID: %s', e) Use logging.error (requested in the previous version). And include also the vm id in the message. Line 661: else: Line 662: if vm.name() not in seen: Line 663: yield vm Line 664: Line 667: params = {} Line 668: try: Line 669: _add_vm_info(vm, params) Line 670: except libvirt.libvirtError as e: Line 671: logging.error("error getting domain information: %s", e) This changes belong to to the previous patch. Line 672: return Line 673: try: Line 674: xml = vm.XMLDesc(0) Line 675: except libvirt.libvirtError as e: Line 674: xml = vm.XMLDesc(0) Line 675: except libvirt.libvirtError as e: Line 676: logging.error("error getting domain xml for vm %r: %s", Line 677: vm.name(), e) Line 678: return This changes belong to to the previous patch. Line 679: try: Line 680: root = ET.fromstring(xml) Line 681: except ET.ParseError as e: Line 682: logging.exception('error parsing domain xml') Line 683: return Line 684: try: Line 685: _add_general_info(root, params) Line 686: except InvalidVMConfiguration as e: Line 687: logging.error("error adding general info: %s", e) This changes belong to to the previous patch. Line 688: return Line 689: _add_networks(root, params) Line 690: _add_disks(root, params) Line 691: for disk in params['disks']: -- To view, visit https://gerrit.ovirt.org/48672 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic7d7e211a9343a528f260da2686b34cea00c53a4 Gerrit-PatchSet: 21 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com> Gerrit-Reviewer: Michal Skrivanek <mskri...@redhat.com> Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
