Nir Soffer has posted comments on this change.
Change subject: utils: add CommandStream class
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.ovirt.org/#/c/33909/3/lib/vdsm/utils.py
File lib/vdsm/utils.py:
Line 341:
Line 342: for fd in self.iocb:
Line 343: self.epoll.register(fd, select.EPOLLIN)
Line 344:
Line 345: def _epoll_input(self, fileno):
This method is handling the poll input event, the name seems
consistent.
These names are consistent, but they are not good method names. They do
not describe what the method does, and do not help readability.
The method that does polling is not the same thing as the method that handle certain
event, so their names should not be "consistent".
Having epoll in the name adds no value, because we don't have several method of
polling, one with epoll and one without another type. It would be better if we can replace
epoll with poll without modifying these names.
I would expect to have names like:
- _poll - wait for events. This is typically done with a timeout, and we don't have
another method for polling without a timeouts, so the _timeout suffix is not needed.
- _handle_input - handle input from a watched fd
- _handle_close - handle closed fd
Line 346: self.iocb[fileno](os.read(fileno, io.DEFAULT_BUFFER_SIZE))
Line 347:
Line 348: def _epoll_event(self, fileno):
Line 349: self.epoll.unregister(fileno)
Line 363: return len(self.iocb) == 0
Line 364:
Line 365: def receive(self, timeout=None):
Line 366: if timeout is None:
Line 367: epoll_remaining = -1
If we do it in the loop it will be set at every cycle unless we rely
on som
I'm not worried about setting this each iteration.
Line 368: else:
Line 369: endtime = monotonic_time() + timeout
Line 370:
Line 371: while not self.closed:
--
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: 3
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