Shahar Havivi has posted comments on this change. Change subject: kvmstream: tool for streaming images from libvirt ......................................................................
Patch Set 1: (10 comments) https://gerrit.ovirt.org/#/c/55797/1//COMMIT_MSG Commit Message: PS1, Line 9: v2v is needed to a streaming/download tool f > "v2v needs"? Done 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 im Done PS1, Line 23: import libvirt > please group the non-standard-library imports below all the standard-librar Done PS1, Line 27: elapsed_time = 0 > I'd try to avoid global state unless absolutely needed. Here I think we can Done Line 30: def parse_arguments(): Line 31: parser = argparse.ArgumentParser() Line 32: parser.add_argument('args') Line 33: parser.add_argument('-u', dest='uri', Line 34: help='Libvirt URI') > This should be required argument Done Line 35: parser.add_argument('-s', dest='source', nargs='+', Line 36: help='Source remote volumes path') Line 37: parser.add_argument('-d', dest='dest', nargs='+', Line 38: help='Destination local volumes path') Line 32: parser.add_argument('args') Line 33: parser.add_argument('-u', dest='uri', Line 34: help='Libvirt URI') Line 35: parser.add_argument('-s', dest='source', nargs='+', Line 36: help='Source remote volumes path') > This should be required argument Done Line 37: parser.add_argument('-d', dest='dest', nargs='+', Line 38: help='Destination local volumes path') Line 39: return parser.parse_args(sys.argv) Line 40: Line 34: help='Libvirt URI') Line 35: parser.add_argument('-s', dest='source', nargs='+', Line 36: help='Source remote volumes path') Line 37: parser.add_argument('-d', dest='dest', nargs='+', Line 38: help='Destination local volumes path') > This should be required argument Done Line 39: return parser.parse_args(sys.argv) Line 40: Line 41: Line 42: def write_output(msg): Line 46: Line 47: def init_progres(): Line 48: global elapsed_time Line 49: write_output('[ %d.0] Opening the source -i libvirt\n' % elapsed_time) Line 50: elapsed_time = elapsed_time + 1 > This is left over from fake progress code No, I increase it intentionally. Line 51: write_output('[ %d.0] Creating an overlay to protect\n' % elapsed_time) Line 52: elapsed_time = elapsed_time + 1 Line 53: Line 54: Line 69: vol.download(stream, 0, 0, 0) Line 70: write_output(' (0/100%%)\r') Line 71: counter = 0 Line 72: while pos < size: Line 73: buf = s.recv(1024) > We use 1M in ovirt-imageio and vdsm when copying images. We should have a d Done Line 74: f.write(buf) Line 75: f.flush() Line 76: if not pos % jump: Line 77: counter = counter + 1 Line 82: write_output(' (100/100%%)\r') Line 83: Line 84: Line 85: options = parse_arguments() Line 86: con = libvirt.open(options.uri) > Some error handling? Done Line 87: Line 88: diskno = 1 Line 89: disksitems = len(options.source) Line 90: init_progres() -- 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 <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Michal Skrivanek <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Shahar Havivi <[email protected]> Gerrit-Reviewer: Yaniv Kaul <[email protected]> Gerrit-Reviewer: gerrit-hooks <[email protected]> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
