Change in vdsm[master]: v2v: extract specific classes for libvirt and ova
nsoffer at redhat.com
nsoffer at redhat.com
Thu Dec 24 17:11:07 UTC 2015
Nir Soffer has posted comments on this change.
Change subject: v2v: extract specific classes for libvirt and ova
......................................................................
Patch Set 9: Code-Review-1
(8 comments)
Generally look good
https://gerrit.ovirt.org/#/c/49951/9/vdsm/v2v.py
File vdsm/v2v.py:
Line 360
Line 361
Line 362
Line 363
Line 364
Job must implement this.
Line 404
Line 405
Line 406
Line 407
Line 408
This looks more like _run_command now.
Line 278:
Line 279:
Line 280: class V2VCommand(object):
Line 281: def __init__(self, uri, vminfo, vmid, irs):
Line 282: self._uri = uri
uri does not belong here it should be parameter of LibvirtCommand.
Line 283: self._vminfo = vminfo
Line 284: self._vmid = vmid
Line 285: self._irs = irs
Line 286: self._prepared_volumes = []
Line 358: we need storage domain absolute path so we go up 3 levels
Line 359: '''
Line 360: return path.rsplit(os.sep, 3)[0]
Line 361:
Line 362: def _environments(self):
This common name for this is "environment"
Line 363: env = {'LIBGUESTFS_BACKEND': 'direct'}
Line 364: if 'virtio_iso_path' in self._vminfo:
Line 365: env['VIRTIO_WIN'] = self._vminfo['virtio_iso_path']
Line 366: return env
Line 366: return env
Line 367:
Line 368:
Line 369: class LibvirtCommand(V2VCommand):
Line 370: def __init__(self, uri, username, password, vminfo, vmid, irs):
Keep the order of arguments same as the super class, adding the specific arguments in the end.
uri, vminfo, vmid, irs, username, password
Line 371: super(self.__class__, self).__init__(uri, vminfo, vmid, irs)
Line 372: self._username = username
Line 373: self._password = password
Line 374:
Line 367:
Line 368:
Line 369: class LibvirtCommand(V2VCommand):
Line 370: def __init__(self, uri, username, password, vminfo, vmid, irs):
Line 371: super(self.__class__, self).__init__(uri, vminfo, vmid, irs)
This works for the current class, but is not the common idiom. Please use the standard way:
super(LibvirtCommand, self)...
Line 372: self._username = username
Line 373: self._password = password
Line 374:
Line 375: self._passwd_file = os.path.join(_V2V_DIR, "%s.tmp" % vmid)
Line 416: self._vmid, self._passwd_file)
Line 417:
Line 418:
Line 419: class OvaCommand(V2VCommand):
Line 420: def __init__(self, ova_path, vminfo, vmid, irs):
Keep the order of arguments same as the super class, adding the specific arguments in the end.
uri, vminfo, vmid, irs
In this case ova_path is used as uri when you call the super class, is this correct?
Maybe uri belong only to LibvirtCommand?
Line 421: super(self.__class__, self).__init__(ova_path, vminfo, vmid, irs)
Line 422: self._ova_path = ova_path
Line 423:
Line 424: def _command(self):
Line 417:
Line 418:
Line 419: class OvaCommand(V2VCommand):
Line 420: def __init__(self, ova_path, vminfo, vmid, irs):
Line 421: super(self.__class__, self).__init__(ova_path, vminfo, vmid, irs)
Same, use class name, not __class__
Line 422: self._ova_path = ova_path
Line 423:
Line 424: def _command(self):
Line 425: cmd = [_VIRT_V2V.cmd,
--
To view, visit https://gerrit.ovirt.org/49951
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1a9ecd4a2cde6f379188da647c3a6f8874c41abd
Gerrit-PatchSet: 9
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