Change in vdsm[master]: v2v: externalVMList Xen+Kvm support

fromani at redhat.com fromani at redhat.com
Tue Dec 8 18:14:25 UTC 2015


Francesco Romani has posted comments on this change.

Change subject: v2v: externalVMList Xen+Kvm support
......................................................................


Patch Set 16: Code-Review+1

(2 comments)

looks OK, could be improved

https://gerrit.ovirt.org/#/c/48672/16/vdsm/v2v.py
File vdsm/v2v.py:

Line 642:         for vm in conn.listAllDomains():
Line 643:             yield vm
Line 644:     except libvirt.libvirtError as e:
Line 645:         # TODO: use new API: listAllDomain() when supported in Xen
Line 646:         #       under RHEL 5.x
this comment is useful, but seems misplaced here.
I believe this whole function should go away once we could always use listAllDomains, right?
If so maybe better turn this comment in the docstring of this function, making clear that we need all this dance precisely because we cannot use listAllDomains().
Line 647:         if e.get_error_code() != libvirt.VIR_ERR_NO_SUPPORT:
Line 648:             raise
Line 649:         seen = set()
Line 650:         for name in conn.listDefinedDomains():


Line 654:         for vmid in conn.listDomainsID():
Line 655:             vm = conn.lookupByID(vmid)
Line 656:             if vm.name() in seen:
Line 657:                 continue
Line 658:             yield vm
codewise seems fine, but I'd pack this in an helper function. Maybe:

  def _list_all_domain(conn):
    seen = set()
    for name in conn.listDefinedDomains():
      seen.add(name)
      vm = conn.lookupByName(name)
      yield vm
    for vmid in conn.listDomainsID():
      vm = conn.lookupByID(vmid)
      if vm.name() in seen:
        continue
      yield vm

the idea is to make it obvious at first glance what we could drop once we don't need to support RHEL 5 anymore
Line 659: 
Line 660: 
Line 661: def _add_vm(conn, vms, vm):
Line 662:     params = {}


-- 
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: 16
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik <mpolednik at redhat.com>
Gerrit-Reviewer: Michal Skrivanek <mskrivan at redhat.com>
Gerrit-Reviewer: Milan Zamazal <mzamazal at redhat.com>
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