Change in vdsm[master]: utils: add CommandStream class

nsoffer at redhat.com nsoffer at redhat.com
Fri Jan 23 13:33:32 UTC 2015


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 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: Saggi Mizrahi <smizrahi 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