Change in vdsm[master]: kvmstream: tool for streaming images from libvirt

fromani at redhat.com fromani at redhat.com
Thu Apr 7 10:26:45 UTC 2016


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 <shavivi at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Michal Skrivanek <mskrivan at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list