Shahar Havivi has posted comments on this change.

Change subject: v2v: Convert VM from external source to Data Domain
......................................................................


Patch Set 4:

(4 comments)

http://gerrit.ovirt.org/#/c/37509/4/lib/vdsm/constants.py.in
File lib/vdsm/constants.py.in:

Line 154: EXT_UDEVADM = '@UDEVADM_PATH@'
Line 155: 
Line 156: EXT_UMOUNT = '@UMOUNT_PATH@'
Line 157: 
Line 158: EXT_VIRT_V2V = '@VIRT_V2V_PATH@'
> Please use CommandPath; constats.py junkyard is to be retired.
Done
Line 159: 
Line 160: EXT_VDSM_RESTORE_NET_CONFIG = '@VDSMDIR@/vdsm-restore-net-config'
Line 161: EXT_VDSM_STORE_NET_CONFIG = '@VDSMDIR@/vdsm-store-net-config'
Line 162: 


http://gerrit.ovirt.org/#/c/37509/4/vdsm/v2v.py
File vdsm/v2v.py:

Line 92:             return errCode['invalidV2VJob']
Line 93: 
Line 94:     try:
Line 95:         file_name = os.path.join(_P_V2V_DIR, "%s.ovf" % jobId)
Line 96:         with open(file_name, 'r') as f:
> this removes partially-written ovfs. We must first verify that the job at h
sure,
the check is:
self._status = 'done' since the status change to done only if virt-v2v return 
code is 0
Line 97:             ovf = f.read()
Line 98:         os.remove(file_name)
Line 99:     except IOError as exc:
Line 100:         if exc.errno == errno.ENOENT:


Line 206:                                 self._vmProperties['vmName'])
Line 207: 
Line 208:         logging.info('import vm, (jobId %s) started, cmd: %r', 
self._job_id,
Line 209:                      cmd)
Line 210:         proc = subprocess.Popen(cmd, env={'LIBGUESTFS_BACKEND': 
'direct'},
> yes, you should use utils.execCmd(), and must never touch shell=True.
We do have to use the shell=True since we do interact with the shell,
look at method: _handle_process_input()
Line 211:                                 shell=True,
Line 212:                                 stdout=subprocess.PIPE,
Line 213:                                 stderr=subprocess.PIPE)
Line 214: 


Line 313:                  'domainID': self._vmProperties['domainID'],
Line 314:                  'imageID': disk['imageID'],
Line 315:                  'volumeID': disk['volumeID']}
Line 316: 
Line 317:         volPath = self._cif.getInstance().prepareVolumePath(drive)
> if you pass ._cif all the way into here, you don't need getInstance.
Done
Line 318:         volPath = volPath.split('/images/')[0]
Line 319:         return volPath
Line 320: 
Line 321: 


-- 
To view, visit http://gerrit.ovirt.org/37509
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I34bd86d5a87ea8c42113c4a732f87ddd4ceab9ea
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <[email protected]>
Gerrit-Reviewer: Dan Kenigsberg <[email protected]>
Gerrit-Reviewer: Francesco Romani <[email protected]>
Gerrit-Reviewer: Michal Skrivanek <[email protected]>
Gerrit-Reviewer: Nir Soffer <[email protected]>
Gerrit-Reviewer: Piotr Kliczewski <[email protected]>
Gerrit-Reviewer: Saggi Mizrahi <[email protected]>
Gerrit-Reviewer: Shahar Havivi <[email protected]>
Gerrit-Reviewer: [email protected]
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Reply via email to