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: