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

Royce Lv has uploaded a new change for review.

Change subject: fix error handling for misc.NoIntrPoll
......................................................................

fix error handling for misc.NoIntrPoll

when recieving signal, poll() raise select.error
fix this to misc.NoIntrPoll to avoid exception
Found in:
http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/170/

Change-Id: I272654a9006fdab77e5fab608ac287416d75843e
Signed-off-by: Royce Lv
---
M tests/miscTests.py
M vdsm/storage/misc.py
2 files changed, 20 insertions(+), 2 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/58/9458/1

diff --git a/tests/miscTests.py b/tests/miscTests.py
index fb37b72..d47c883 100644
--- a/tests/miscTests.py
+++ b/tests/miscTests.py
@@ -22,6 +22,8 @@
 import uuid
 import time
 import threading
+import select
+import signal
 from testrunner import VdsmTestCase as TestCaseBase
 import inspect
 from vdsm import utils
@@ -1117,3 +1119,19 @@
             return misc.findCaller()

self.assertRaises(AssertionError, self._assertFindCaller, _foo)
+
+
+class NoIntrPollTests(TestCaseBase):
+    def testNoIntr(self):
+        def fakeSigchld():
+            for i in range(5):
+                time.sleep(1)
+                os.kill(os.getpid(), signal.SIGCHLD)
+
+        intrThread = threading.Thread(target=fakeSigchld)
+        intrThread.setDaemon(True)
+        intrThread.start()
+        poller = select.poll()
+        tempFd, tempPath = tempfile.mkstemp()
+        poller.register(tempFd, select.POLLERR)
+        misc.NoIntrPoll(poller.poll, 10000)
diff --git a/vdsm/storage/misc.py b/vdsm/storage/misc.py
index 6360dcf..d2ac851 100644
--- a/vdsm/storage/misc.py
+++ b/vdsm/storage/misc.py
@@ -1346,8 +1346,8 @@
     while True:
         try:
             return pollfun(timeout)
-        except (OSError, IOError), e:
-            if not e.errno in (errno.EINTR, errno.EAGAIN):
+        except (os.error, select.error), e:
+            if e.args[0] != errno.EINTR:
                 raise
         timeout = max(0, endtime - time.time())

--
To view, visit http://gerrit.ovirt.org/9458
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I272654a9006fdab77e5fab608ac287416d75843e
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: fix error handling for misc.NoIntrPoll
......................................................................

Patch Set 1:

Build Started http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/138/ (1/2)

--
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: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: fix error handling for misc.NoIntrPoll
......................................................................

Patch Set 1:

Build Started http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/172/ (2/2)

--
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: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: fix error handling for misc.NoIntrPoll
......................................................................

Patch Set 1:

Build Successful

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/138/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/172/ : SUCCESS

--
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: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: fix error handling for misc.NoIntrPoll
......................................................................

Patch Set 1: I would prefer that you didn't submit this

(2 inline comments)

Thanks for nailing this bug!

....................................................
File tests/miscTests.py
Line 1124: class NoIntrPollTests(TestCaseBase):
Line 1125:     def testNoIntr(self):
Line 1126:         def fakeSigchld():
Line 1127:             for i in range(5):
Line 1128:                 time.sleep(1)
Could you use time CONSTANTS, and make them shorter? I'd hate to spend 10 more seconds...
Line 1129:                 os.kill(os.getpid(), signal.SIGCHLD)
Line 1130: 
Line 1131:         intrThread = threading.Thread(target=fakeSigchld)
Line 1132:         intrThread.setDaemon(True)

....................................................
File vdsm/storage/misc.py
Line 1345: 
Line 1346:     while True:
Line 1347:         try:
Line 1348:             return pollfun(timeout)
Line 1349:         except (os.error, select.error), e:
some of the change is really required since

os.error is OSError
Line 1350:             if e.args[0] != errno.EINTR:
Line 1351:                 raise
Line 1352:         timeout = max(0, endtime - time.time())
Line 1353:

--
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: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: fix error handling for misc.NoIntrPoll
......................................................................

Patch Set 2:

Build Started http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/140/ (1/2)

--
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: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Barak Azulay 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Yaniv Bronhaim 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: fix error handling for misc.NoIntrPoll
......................................................................

Patch Set 2:

Build Started http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/174/ (2/2)

--
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: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Barak Azulay 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Yaniv Bronhaim 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: fix error handling for misc.NoIntrPoll
......................................................................

Patch Set 2:

Build Successful

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/140/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/174/ : SUCCESS

--
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: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Barak Azulay 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Yaniv Bronhaim 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: fix error handling for misc.NoIntrPoll
......................................................................

Patch Set 2: I would prefer that you didn't submit this

(1 inline comment)

sorry for not being clear earlier: the important thing about having CONSTANTS, is that other intervals are calculated based on them. Now, if I decide make the test even shorter, I still have to change it in two places.

....................................................
File tests/miscTests.py
Line 1132:         intrThread = threading.Thread(target=fakeSigchld)
Line 1133:         intrThread.setDaemon(True)
Line 1134:         intrThread.start()
Line 1135: 
Line 1136:         pollIntervalMsec = 1000
for me, the important thing would be to have

RETRIES = 3  #the arg to range()
 pollIntervalMsec = 1000 * sleepTime * 2 * RETRIES

(and it's nicer to have ALLCAPS for constants)
Line 1137:         tempFd, tempPath = tempfile.mkstemp()
Line 1138: 
Line 1139:         poller = select.poll()
Line 1140:         poller.register(tempFd, select.POLLERR)

--
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: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Barak Azulay 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Yaniv Bronhaim 
Gerrit-Reviewer: oVirt Jenkins CI Server

Saggi Mizrahi has posted comments on this change.

Change subject: fix error handling for misc.NoIntrPoll
......................................................................

Patch Set 2: Looks good to me, but someone else must approve

--
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: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Barak Azulay 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Yaniv Bronhaim 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: fix error handling for misc.NoIntrPoll
......................................................................

Patch Set 2: I would prefer that you didn't submit this

(2 inline comments)

....................................................
File tests/miscTests.py
Line 1121:         self.assertRaises(AssertionError, self._assertFindCaller, _foo)
Line 1122: 
Line 1123: 
Line 1124: class NoIntrPollTests(TestCaseBase):
Line 1125:     def testNoIntr(self):
Please add another test that checks the behavior with a multiprocessing Pipe().
Line 1126:         def fakeSigchld():
Line 1127:             sleepTime = 0.1
Line 1128:             for i in range(3):
Line 1129:                 time.sleep(sleepTime)

....................................................
File vdsm/storage/misc.py
Line 1345: 
Line 1346:     while True:
Line 1347:         try:
Line 1348:             return pollfun(timeout)
Line 1349:         except (OSError, select.error), e:
I have the feeling that the previous format was to support the multiprocessing Pipe(). Now we switched to select and we didn't think of this. (I'm not really happy to see that select is wrapping this error though).
Line 1350:             if e.args[0] != errno.EINTR:
Line 1351:                 raise
Line 1352:         timeout = max(0, endtime - time.time())
Line 1353:

--
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: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Barak Azulay 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Yaniv Bronhaim 
Gerrit-Reviewer: oVirt Jenkins CI Server

Royce Lv has posted comments on this change.

Change subject: fix error handling for misc.NoIntrPoll
......................................................................

Patch Set 2: (3 inline comments)

....................................................
File tests/miscTests.py
Line 1121:         self.assertRaises(AssertionError, self._assertFindCaller, _foo)
Line 1122: 
Line 1123: 
Line 1124: class NoIntrPollTests(TestCaseBase):
Line 1125:     def testNoIntr(self):
Done
Line 1126:         def fakeSigchld():
Line 1127:             sleepTime = 0.1
Line 1128:             for i in range(3):
Line 1129:                 time.sleep(sleepTime)

Line 1132:         intrThread = threading.Thread(target=fakeSigchld)
Line 1133:         intrThread.setDaemon(True)
Line 1134:         intrThread.start()
Line 1135: 
Line 1136:         pollIntervalMsec = 1000
Done
Line 1137:         tempFd, tempPath = tempfile.mkstemp()
Line 1138: 
Line 1139:         poller = select.poll()
Line 1140:         poller.register(tempFd, select.POLLERR)

....................................................
File vdsm/storage/misc.py
Line 1345: 
Line 1346:     while True:
Line 1347:         try:
Line 1348:             return pollfun(timeout)
Line 1349:         except (OSError, select.error), e:
I checked with pipe closed and pipe write overflow raise EAGAIN, neither raised error in the poll end. Do I ignore any usecase?
Line 1350:             if e.args[0] != errno.EINTR:
Line 1351:                 raise
Line 1352:         timeout = max(0, endtime - time.time())
Line 1353:

--
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: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Barak Azulay 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Yaniv Bronhaim 
Gerrit-Reviewer: oVirt Jenkins CI Server

ShaoHe Feng has posted comments on this change.

Change subject: fix error handling for misc.NoIntrPoll
......................................................................

Patch Set 3: (1 inline comment)

....................................................
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/
oh, I can not open this link.
Line 15: 
Line 16: Change-Id: I272654a9006fdab77e5fab608ac287416d75843e

--
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 
Gerrit-Reviewer: Barak Azulay 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: ShaoHe Feng 
Gerrit-Reviewer: Yaniv Bronhaim 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Dan Kenigsberg]

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 
Gerrit-Reviewer: Barak Azulay 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: ShaoHe Feng 
Gerrit-Reviewer: Yaniv Bronhaim 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: fix error handling for misc.NoIntrPoll
......................................................................

Patch Set 3: (1 inline comment)

....................................................
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/
here's another manifestation at http://jenkins.ovirt.org/job/vdsm_unit_tests/1668/testReport/junit/remoteFil... :

Traceback (most recent call last):
  File "/usr/lib64/python2.7/unittest/case.py", line 365, in run
    testMethod()
  File "/home/jenkins/workspace/vdsm_unit_tests/tests/remoteFileHandlerTests.py", line 54, in testRegeneration
    self.testEcho()
  File "/home/jenkins/workspace/vdsm_unit_tests/tests/remoteFileHandlerTests.py", line 41, in testEcho
    self.assertEquals(self.pool.callCrabRPCFunction(1, "echo", data), data)
  File "/home/jenkins/workspace/vdsm_unit_tests/vdsm/storage/remoteFileHandler.py", line 280, in callCrabRPCFunction
    *args, **kwargs)
  File "/home/jenkins/workspace/vdsm_unit_tests/vdsm/storage/remoteFileHandler.py", line 191, in callCrabRPCFunction
    raise Timeout()
Timeout: 
-------------------- >> begin captured logging << --------------------
Storage.CrabRPCProxy: ERROR: Problem with handler, treating as timeout
Traceback (most recent call last):
  File "/home/jenkins/workspace/vdsm_unit_tests/vdsm/storage/remoteFileHandler.py", line 180, in callCrabRPCFunction
    rawLength = self._recvAll(LENGTH_STRUCT_LENGTH, timeout)
  File "/home/jenkins/workspace/vdsm_unit_tests/vdsm/storage/remoteFileHandler.py", line 149, in _recvAll
    timeLeft):
  File "/usr/lib64/python2.7/contextlib.py", line 84, in helper
    return GeneratorContextManager(func(*args, **kwds))
  File "/home/jenkins/workspace/vdsm_unit_tests/vdsm/storage/remoteFileHandler.py", line 133, in _poll
    res = misc.NoIntrPoll(self._poller.poll, timeout)
  File "/home/jenkins/workspace/vdsm_unit_tests/vdsm/storage/misc.py", line 1348, in NoIntrPoll
    return pollfun(timeout)
error: (4, 'Interrupted system call')
--------------------- >> end captured logging << ---------------------
Line 15: 
Line 16: Change-Id: I272654a9006fdab77e5fab608ac287416d75843e

--
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 
Gerrit-Reviewer: Barak Azulay 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: ShaoHe Feng 
Gerrit-Reviewer: Yaniv Bronhaim 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: fix error handling for misc.NoIntrPoll
......................................................................

Patch Set 4: I would prefer that you didn't submit this

(1 inline comment)

minor comment. other than that, I'm good.

....................................................
File tests/miscTests.py
Line 1152:         poller.unregister(fd)
Line 1153: 
Line 1154:     def testWatchFile(self):
Line 1155:         try:
Line 1156:             tempFd, tempPath = tempfile.mkstemp()
"try" should be put right after tempPath has been created, not before it - in case mkstemp() fails.

In this particular case, rmFile would swallow the error, but this is bad practice.
Line 1157:             self._startFakeSigchld()
Line 1158:             # only poll can support regular file
Line 1159:             self._noIntrWatchFd(tempFd, isEpoll=False)
Line 1160:         finally:

--
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: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Barak Azulay 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: ShaoHe Feng 
Gerrit-Reviewer: Yaniv Bronhaim 
Gerrit-Reviewer: oVirt Jenkins CI Server

Saggi Mizrahi has posted comments on this change.

Change subject: fix error handling for misc.NoIntrPoll
......................................................................

Patch Set 5: Looks good to me, but someone else must approve

--
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: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Barak Azulay 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: ShaoHe Feng 
Gerrit-Reviewer: Yaniv Bronhaim 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: fix error handling for misc.NoIntrPoll
......................................................................

fix error handling for misc.NoIntrPoll

When recieving signal, poll raises select.error, and epoll raises OSError.
Former exception wasn't caught by NoIntrPoll, which defies the purpose of that
wrapper (see backtrace).

Tested poll/epoll, pipe/file, closed pipe/pipe buffer overflow
pipe write/read end error will not raise poll error.

Storage.CrabRPCProxy: ERROR: Problem with handler, treating as timeout
Traceback (most recent call last):
  File "/home/jenkins/workspace/vdsm_unit_tests/vdsm/storage/remoteFileHandler.py", line 180, in callCrabRPCFunction
    rawLength = self._recvAll(LENGTH_STRUCT_LENGTH, timeout)
  File "/home/jenkins/workspace/vdsm_unit_tests/vdsm/storage/remoteFileHandler.py", line 149, in _recvAll
    timeLeft):
  File "/usr/lib64/python2.7/contextlib.py", line 84, in helper
    return GeneratorContextManager(func(*args, **kwds))
  File "/home/jenkins/workspace/vdsm_unit_tests/vdsm/storage/remoteFileHandler.py", line 133, in _poll
    res = misc.NoIntrPoll(self._poller.poll, timeout)
  File "/home/jenkins/workspace/vdsm_unit_tests/vdsm/storage/misc.py", line 1348, in NoIntrPoll
    return pollfun(timeout)
error: (4, 'Interrupted system call')

Change-Id: I272654a9006fdab77e5fab608ac287416d75843e
Signed-off-by: Royce Lv
Signed-off-by: Dan Kenigsberg 
---
M tests/miscTests.py
M vdsm/storage/misc.py
2 files changed, 82 insertions(+), 2 deletions(-)

Approvals:
  Saggi Mizrahi: Looks good to me, but someone else must approve
  Dan Kenigsberg: Verified; Looks good to me, approved

--
To view, visit http://gerrit.ovirt.org/9458
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I272654a9006fdab77e5fab608ac287416d75843e
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Barak Azulay 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: ShaoHe Feng 
Gerrit-Reviewer: Yaniv Bronhaim 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: fix error handling for misc.NoIntrPoll
......................................................................

Patch Set 5: Verified; Looks good to me, approved

--
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: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Barak Azulay 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: ShaoHe Feng 
Gerrit-Reviewer: Yaniv Bronhaim 
Gerrit-Reviewer: oVirt Jenkins CI Server