Yaniv Kaul has posted comments on this change. Change subject: kvmstream: tool for streaming images from libvirt ......................................................................
Patch Set 1: (3 comments) Few notes in the code, but generally I'd give -1 since I think it's critical to use the imageio stuff here. https://gerrit.ovirt.org/#/c/55797/1/kvmstream.py File kvmstream.py: 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) The 1024 is probably a low value - I'd increase it (but in any case, I think Nir is completely right in his above comment - you must use the imageio stuff. 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? if con == None: ... Line 87: Line 88: diskno = 1 Line 89: disksitems = len(options.source) Line 90: init_progres() Line 87: Line 88: diskno = 1 Line 89: disksitems = len(options.source) Line 90: init_progres() Line 91: for item in izip(options.source, options.dest): I wonder if we should test downloading multiple disks in parallel. Not sure we need to do it one by one, though it adds complexity. Line 92: vol = con.storageVolLookupByPath(item[0]) Line 93: s = con.newStream() Line 94: download_volume(s, vol, item[1], diskno, disksitems) Line 95: diskno = diskno + 1 -- 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: Yaniv Kaul <yk...@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
