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

nsoffer at redhat.com nsoffer at redhat.com
Tue Apr 28 11:27:44 UTC 2015


Nir Soffer has posted comments on this change.

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


Patch Set 6:

(2 comments)

Nice, but I think we should simplify and clarify the code receiving data.

Looked only at the new QimgProgress code, will look at the rest later today.

https://gerrit.ovirt.org/#/c/33910/6/lib/vdsm/qemuimg.py
File lib/vdsm/qemuimg.py:

Line 224:         self._term_lock = threading.Lock()
Line 225: 
Line 226:         _log.debug(utils.commandLogFormat(cmd, cwd=cwd))
Line 227:         self._command = CPopen(cmd, cwd=cwd, deathSignal=signal.SIGKILL)
Line 228:         self._execution = utils.CommandStream(
_execution is a strange term here - this is a CommandStream - this is a good example that CommandStream is not a good name.

How about:

    self._receiver = utils.CommandRceiver(...)

I think this is what this class does - allow you to register recv_stdout and recv_stderr and call wait until there is nothing to receive.
Line 229:             self._command, self._recvstdout, self._recvstderr)
Line 230: 
Line 231:     def _recvstderr(self, buffer):
Line 232:         self._stderr += buffer


Line 241:             except ValueError:
Line 242:                 break
Line 243: 
Line 244:             last_progress = self._stdout[:idx]
Line 245:             del self._stdout[:idx + 1]
I think it will be simpler and more clear:

    end = buffer.rindex('\r')
    if end < 0:
        self._stdout += buffer
        return  # Needs more data
 
    received = self._stdout + buffer[:end]
    self._stdout = buffer[end + 1:]

    # Skip older progress chunks
    last_progress = received.rsplit('\r', 1)[-1]

    handle last_progress...
Line 246: 
Line 247:         if last_progress:
Line 248:             m = self.REGEXPR.match(last_progress.strip())
Line 249:             if m is None:


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id0b53e418c62bb2e91444ba5f351c916ca417299
Gerrit-PatchSet: 6
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: Federico Simoncelli <fsimonce 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