Nir Soffer has posted comments on this change.
Change subject: utils: add CommandStream class
......................................................................
Patch Set 4:
(2 comments)
http://gerrit.ovirt.org/#/c/33909/4/lib/vdsm/utils.py
File lib/vdsm/utils.py:
Line 356: # entries in the dictionary)
Line 357: self._iocb = {
Line 358: self._command.stderr.fileno(): stderrcb,
Line 359: self._command.stdout.fileno(): stdoutcb,
Line 360: }
This practically works, but I'm not sure the behavior is defined. It would be better
to have a method to add a descriptor and invoke it in a well defined order, with the
comment explaining the correct order.
Line 361:
Line 362: for fd in self._iocb:
Line 363: self._poll.register(fd, select.EPOLLIN)
Line 364:
Line 362: for fd in self._iocb:
Line 363: self._poll.register(fd, select.EPOLLIN)
Line 364:
Line 365: def _poll_input(self, fileno):
Line 366: self._iocb[fileno](os.read(fileno, io.DEFAULT_BUFFER_SIZE))
We don't set the descriptors to non-blocking mode, so this may block when handling an
event.
We should set the descriptors to non-blocking when command is given, or maybe dup them and
set the dup as non-blocking. Then we should handle EAGAIN here.
Another issue is handling errors - if os.read raises, the caller should get the exception
and handle it. We can have an errorcb called when os.read() fails with the fd and the
exception.
Or if we choose to let the caller of receive handle such errors, we should document the
expected errors that received may raise.
Finally, if the caller wants to stop listening for events - how can you "close"
this stream?
Line 367:
Line 368: def _poll_event(self, fileno):
Line 369: self._poll.unregister(fileno)
Line 370: del self._iocb[fileno]
--
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: 4
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