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