Change in vdsm[master]: qemuimg: add support for convert progress

fromani at redhat.com fromani at redhat.com
Thu Nov 20 14:56:59 UTC 2014


Francesco Romani has posted comments on this change.

Change subject: qemuimg: add support for convert progress
......................................................................


Patch Set 2:

(5 comments)

more initial comments

http://gerrit.ovirt.org/#/c/33910/2/lib/vdsm/qemuimg.py
File lib/vdsm/qemuimg.py:

Line 196: class QemuImgProcess(object):
Line 197:     REGEXPR = re.compile(r'\(([\d.]+)/100%\)')
Line 198: 
Line 199:     def __init__(self, cmd, cwd=None):
Line 200:         self.progress = 0.0
maybe it is just me, but I'd like it more as read-only property built on a private field.
Line 201: 
Line 202:         self._stdout = bytearray()
Line 203:         self._stderr = bytearray()
Line 204: 


Line 204: 
Line 205:         cmd = utils.ionice_cmd(cmd, utils.IOCLASS.IDLE)
Line 206:         cmd = utils.nice_cmd(cmd, utils.NICENESS.HIGH)
Line 207: 
Line 208:         self.execution = utils.CommandStream(
couldn't this be made _private?
Line 209:             cmd, self._recvstdout, self._recvstderr, cwd=cwd,
Line 210:             deathSignal=signal.SIGKILL)
Line 211: 
Line 212:     def _recvstderr(self, buffer):


Line 219:         while True:
Line 220:             try:
Line 221:                 idx = self._stdout.index('\r')
Line 222:             except ValueError:
Line 223:                 break
it seems to me that if there are multiple progress report to parse, everyone but the last one will be overwritten
and silently discarded.

If the above is right, why don't just do rindex (or rfind) once instead of looping?
Line 224: 
Line 225:             last_progress = self._stdout[:idx]
Line 226:             del self._stdout[:idx + 1]
Line 227: 


Line 222:             except ValueError:
Line 223:                 break
Line 224: 
Line 225:             last_progress = self._stdout[:idx]
Line 226:             del self._stdout[:idx + 1]
we discussed about a possible deque usage, although I find it clearer, I can happily live without.
Line 227: 
Line 228:         if last_progress:
Line 229:             m = self.REGEXPR.match(last_progress.strip())
Line 230:             if m is None:


http://gerrit.ovirt.org/#/c/33910/2/vdsm/storage/image.py
File vdsm/storage/image.py:

Line 110: 
Line 111:     def __init__(self, repoPath):
Line 112:         self.repoPath = repoPath
Line 113: 
Line 114:     def qemuImgConvert(self, *args, **kwargs):
No big deal, but could this be made a function, instead of a method? it seems to use very little of the class context.
Line 115:         self.log.debug('starting qemu-img operation')
Line 116:         command = qemuimg.convert(*args, **kwargs)
Line 117: 
Line 118:         def abortImgConversion():


-- 
To view, visit http://gerrit.ovirt.org/33910
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id0b53e418c62bb2e91444ba5f351c916ca417299
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list