Francesco Romani has posted comments on this change. Change subject: kvmstream: tool for streaming images from libvirt ......................................................................
Patch Set 1: (5 comments) initial review. The idea is neat and the logic seems fine, only comments about style and possible improvements. https://gerrit.ovirt.org/#/c/55797/1/kvmstream.py File kvmstream.py: PS1, Line 22: from itertools import izip nit: better to avoid from ... imports and just import modules unless the imported name is overly long. e.g.: contextlib.contextmanager is too long here I'd just import itertools and use like itertools.izip PS1, Line 23: import libvirt please group the non-standard-library imports below all the standard-library imports, separated using one blank line PS1, Line 27: elapsed_time = 0 I'd try to avoid global state unless absolutely needed. Here I think we can. PS1, Line 47: init_progres typo: progress We could maybe add a class to encapsulate our progress reporting. Let me think about it for a little while. PS1, Line 68: f = open(dest, 'w') please use context managers: with open(dest, 'w') as f: # ... also, let's use 'wb' for extra safety. -- To view, visit https://gerrit.ovirt.org/55797 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9d95c3bf4b2605e71f899171259d4721204eb8e2 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Michal Skrivanek <mskri...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches