Francesco Romani has posted comments on this change. Change subject: v2v: Convert VM from external source to Data Domain ......................................................................
Patch Set 3: (5 comments) http://gerrit.ovirt.org/#/c/37509/3/vdsm/v2v.py File vdsm/v2v.py: Line 76: Line 77: Line 78: def convert(uri, username, password, vmProperties, jobId, cif): Line 79: global _jobs Line 80: t = _import_vm_thread(uri, username, password, vmProperties, jobId, cif) > Done, Yes, I'd name it that way. Line 81: _jobs[jobId] = t Line 82: t.start() Line 83: Line 84: Line 91: if not os.path.exists(file_name): Line 92: return errCode['invalidV2VOvfPath'] Line 93: Line 94: f = open(file_name, 'r') Line 95: with closing(f): > why not? in python 2.6 and onwards, the versions we care about, 'file' objects implement the so-called 'context manager protocol', so it is guaranteed that at the end of the `with' block close() will be called automatically as part of the said protocol. You (usually) need to explicitely use closing() when dealing with an object which doesn't yet implement the context manager protocol, so you want to make sure its cleanup code is run. Line 96: ovf = '\n'.join(f.readlines()) Line 97: os.remove(file_name) Line 98: Line 99: _jobs.pop(jobId) Line 93: Line 94: f = open(file_name, 'r') Line 95: with closing(f): Line 96: ovf = '\n'.join(f.readlines()) Line 97: os.remove(file_name) > ok, how is it best to handle returning a string (the ovf) and raising an er ok for me. Line 98: Line 99: _jobs.pop(jobId) Line 100: if len(_jobs) > 0: Line 101: kill_zombie_jobs() Line 97: os.remove(file_name) Line 98: Line 99: _jobs.pop(jobId) Line 100: if len(_jobs) > 0: Line 101: kill_zombie_jobs() > It is called once on vdsm start, a comment in the file and a mention in the commit message are probably enough. Someone else may find this redundant, but I believe it is worth stressing it. Line 102: return ovf Line 103: Line 104: Line 105: def kill_zombie_jobs(): Line 101: kill_zombie_jobs() Line 102: return ovf Line 103: Line 104: Line 105: def kill_zombie_jobs(): > this idea was suggested for case such as vdsm failing while there are runni OK, I get (and agree with) the rationale, it's the pgrep dance that concerns me. Let me think about that. Line 106: ret, out, err = execCmd([EXT_PGREP, 'virt-v2v'], sudo=True) Line 107: if len(err) > 0: Line 108: logging.error('error while trying to grep virt-v2v processes: %r', err) Line 109: return -- 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: 3 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
