Nir Soffer has posted comments on this change.
Change subject: utils: add CommandStream class
......................................................................
Patch Set 2:
(12 comments)
I like the direction.
http://gerrit.ovirt.org/#/c/33909/2/lib/vdsm/utils.py
File lib/vdsm/utils.py:
Line 327: if endtime is not None:
Line 328: timeout = max(0, endtime - time.time())
Line 329:
Line 330:
Line 331: class CommandStream(object):
The name is not very good, as this does not have a stream like interface. Maybe just
"Command"?
And we should move this out of utils to the commands module.
Line 332: def __init__(self, command, stdoutcb, stderrcb, cwd=None,
Line 333: deathSignal=0):
Line 334: self.command = command
Line 335: self.returncode = None
Line 330:
Line 331: class CommandStream(object):
Line 332: def __init__(self, command, stdoutcb, stderrcb, cwd=None,
Line 333: deathSignal=0):
Line 334: self.command = command
all the members could be made _private
+1 - if not everyone
will start to access these.
Do we need to keep self.command?
Line 335: self.returncode = None
Line 336:
Line 337: self.child = CPopen(self.command, cwd=cwd, close_fds=True,
Line 338: deathSignal=deathSignal)
Line 331: class CommandStream(object):
Line 332: def __init__(self, command, stdoutcb, stderrcb, cwd=None,
Line 333: deathSignal=0):
Line 334: self.command = command
Line 335: self.returncode = None
Why do we need self.returncode? we can have a property returning self.child.returncode.
Line 336:
Line 337: self.child = CPopen(self.command, cwd=cwd, close_fds=True,
Line 338: deathSignal=deathSignal)
Line 339:
Line 336:
Line 337: self.child = CPopen(self.command, cwd=cwd, close_fds=True,
Line 338: deathSignal=deathSignal)
Line 339:
Line 340: self.epoll = select.epoll()
I don't see a need for epoll. We are polling on 2 file descriptors, why do we need
epoll for that?
Line 341:
Line 342: self.iocb = {
Line 343: self.child.stdout.fileno(): stdoutcb,
Line 344: self.child.stderr.fileno(): stderrcb,
Line 359: def flush(self):
Line 360: self.child.stdin.flush()
Line 361:
Line 362: def close(self):
Line 363: self.child.stdin.close()
Both flush() and close() are not clear for this class. Accessing stdin directly is
better.
Line 364:
Line 365: def _epoll_input(self, fileno):
Line 366: self.iocb[fileno](os.read(fileno, io.DEFAULT_BUFFER_SIZE))
Line 367:
Line 361:
Line 362: def close(self):
Line 363: self.child.stdin.close()
Line 364:
Line 365: def _epoll_input(self, fileno):
I don't understand this name - maybe _call_iocb()?
Line 366: self.iocb[fileno](os.read(fileno, io.DEFAULT_BUFFER_SIZE))
Line 367:
Line 368: def _epoll_event(self, fileno):
Line 369: self.epoll.unregister(fileno)
Line 364:
Line 365: def _epoll_input(self, fileno):
Line 366: self.iocb[fileno](os.read(fileno, io.DEFAULT_BUFFER_SIZE))
Line 367:
Line 368: def _epoll_event(self, fileno):
This name is also strange - maybe _unregister_iocb()?
Line 369: self.epoll.unregister(fileno)
Line 370: del self.iocb[fileno]
Line 371:
Line 372: def _epoll_timeout(self, timeout):
Line 368: def _epoll_event(self, fileno):
Line 369: self.epoll.unregister(fileno)
Line 370: del self.iocb[fileno]
Line 371:
Line 372: def _epoll_timeout(self, timeout):
This should be something like _wait_for_io()
Line 373: fdevents = NoIntrPoll(self.epoll.poll, timeout)
Line 374:
Line 375: for fileno, event in fdevents:
Line 376: if event & select.EPOLLIN:
Line 379: self._epoll_event(fileno)
Line 380: # Trying to collect the child status in case the
Line 381: # file descriptor was closed because the process
Line 382: # terminated.
Line 383: self.returncode = self.child.poll()
We don't need to keep the return code, as the poll() update the child.returncode.
Line 384:
Line 385: def wait(self, timeout=None):
Line 386: if timeout is None:
Line 387: epoll_remaining = -1
Line 385: def wait(self, timeout=None):
Line 386: if timeout is None:
Line 387: epoll_remaining = -1
Line 388: else:
Line 389: endtime = os.times()[4] + timeout
usage of os.times() could be surprising (I must admit it was like
this for
+1
Line 390:
Line 391: while self.returncode is None:
Line 392: if timeout is not None:
Line 393: epoll_remaining = endtime - os.times()[4]
Line 387: epoll_remaining = -1
Line 388: else:
Line 389: endtime = os.times()[4] + timeout
Line 390:
Line 391: while self.returncode is None:
self.child.returncode?
Line 392: if timeout is not None:
Line 393: epoll_remaining = endtime - os.times()[4]
Line 394: if epoll_remaining <= 0:
Line 395: break
Line 405: # load: tens of children from tens of threads).
Line 406: # Anyway we reach this only when both stdout and
Line 407: # stderr are closed, which means that in most of
Line 408: # the cases the child is about to die.
Line 409: time.sleep(0.0005)
We should not do this.
The proper way to wait is to wait on a condition notified by SIGCHLD signal handler.
See
http://gerrit.ovirt.org/35055/
If you don't want to depend on it now, I think using 1 second wait between polls is
good enough.
Line 410: self.returncode = self.child.poll()
Line 411:
Line 412: return self.returncode
Line 413:
--
To view, visit
http://gerrit.ovirt.org/33909
To unsubscribe, visit
http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie015368bb9c5992e5c73a149277c59fc4ffbd570
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes