Dan Kenigsberg has posted comments on this change. Change subject: fix error handling for misc.NoIntrPoll ......................................................................
Patch Set 3: I would prefer that you didn't submit this (4 inline comments) .................................................... Commit Message Line 10: and epoll raise OSError. Line 11: tested poll/epoll, pipe/file, closed pipe/pipe buffer overflow Line 12: pipe write/read end error will not raise poll error. Line 13: Found in: Line 14: http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/170/ yeah, it has expired (low disk space). It would have been better to quote the exception here. Line 15: Line 16: Change-Id: I272654a9006fdab77e5fab608ac287416d75843e .................................................... File tests/miscTests.py Line 49: SUDO_USER = "root" Line 50: SUDO_GROUP = "root" Line 51: Line 52: PIPE_BUF_BYTES = 65536 Line 53: RETRIES = 3 I would very much prefer these constants as class-level. they have nothing to do with other classes of this module. I personally do not see the point of having both EPOLL_INTERVAL_SEC and SLEEP_INTERVAL. Line 54: SLEEP_INTERVAL = 0.1 Line 55: EPOLL_INTERVAL_SEC = SLEEP_INTERVAL * RETRIES * 2 Line 56: POLL_INTERVAL_MSEC = EPOLL_INTERVAL_SEC * 1000 Line 57: Line 1154: misc.NoIntrPoll(poller.poll, pollInterval) Line 1155: poller.unregister(fd) Line 1156: Line 1157: def testWatchFile(self): Line 1158: tempFd, tempPath = tempfile.mkstemp() better remove this dir in a "finally" clause. Line 1159: self._startFakeSigchld() Line 1160: # epoll don't support regular file Line 1161: self._noIntrWatchFd(tempFd, isEpoll=False) Line 1162: Line 1183: proc.join() Line 1184: Line 1185: def testPipeWriteEAGAIN(self): Line 1186: def _raiseEAGAIN(pipe): Line 1187: str = os.urandom(1 + PIPE_BUF_BYTES) why bother urandom? isn't '0' * (1 + PIPE_BUF_BYTES) enough? also, "str" is a builtin python type. better choose another name for your strings. Line 1188: for i in range(RETRIES): Line 1189: time.sleep(SLEEP_INTERVAL) Line 1190: try: Line 1191: os.write(pipe, str) -- To view, visit http://gerrit.ovirt.org/9458 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I272654a9006fdab77e5fab608ac287416d75843e Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Royce Lv <[email protected]> Gerrit-Reviewer: Barak Azulay <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Royce Lv <[email protected]> Gerrit-Reviewer: Saggi Mizrahi <[email protected]> Gerrit-Reviewer: ShaoHe Feng <[email protected]> Gerrit-Reviewer: Yaniv Bronhaim <[email protected]> Gerrit-Reviewer: oVirt Jenkins CI Server _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
