Change in vdsm[master]: fix error handling for misc.NoIntrPoll

danken at redhat.com danken at redhat.com
Fri Nov 30 07:40:10 UTC 2012


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 <lvroyce at linux.vnet.ibm.com>
Gerrit-Reviewer: Barak Azulay <bazulay at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Royce Lv <lvroyce at linux.vnet.ibm.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: ShaoHe Feng <shaohef at linux.vnet.ibm.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server


More information about the vdsm-patches mailing list