Change in vdsm[master]: misc: handle properly timeout=-1 in NoIntrPoll

List overview All Threads
Download

newer

older

Change in vdsm[master]: BZ#840407...

Change in vdsm[master]: tests:...

Federico Simoncelli

25 Jan 2013 25 Jan '13

4:10 p.m.

Federico Simoncelli has uploaded a new change for review.

Change subject: misc: handle properly timeout=-1 in NoIntrPoll ......................................................................

misc: handle properly timeout=-1 in NoIntrPoll

Change-Id: Iefc4d17559d3335ef6699cb83923c3bd255c916b Signed-off-by: Federico Simoncelli fsimonce@redhat.com --- M tests/miscTests.py M vdsm/storage/misc.py 2 files changed, 36 insertions(+), 9 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/94/11394/1

diff --git a/tests/miscTests.py b/tests/miscTests.py index 81b42ca..904026d 100644 --- a/tests/miscTests.py +++ b/tests/miscTests.py @@ -1128,12 +1128,15 @@ RETRIES = 3 SLEEP_INTERVAL = 0.1

+ def _waitAndSigchld(self): + time.sleep(self.SLEEP_INTERVAL) + os.kill(os.getpid(), signal.SIGCHLD) + def _startFakeSigchld(self): - def _fakeSigchld(): + def _repeatFakeSigchld(): for i in range(self.RETRIES): - time.sleep(self.SLEEP_INTERVAL) - os.kill(os.getpid(), signal.SIGCHLD) - intrThread = threading.Thread(target=_fakeSigchld) + self._waitAndSigchld() + intrThread = threading.Thread(target=_repeatFakeSigchld) intrThread.setDaemon(True) intrThread.start()

@@ -1166,6 +1169,26 @@ self._startFakeSigchld() self._noIntrWatchFd(myPipe, isEpoll=False) # caught select.error

+ def testNoTimeoutPipePoll(self): + def _sigChldAndWrite(fd): + self._waitAndSigchld() + time.sleep(self.SLEEP_INTERVAL) + os.close(fd) + + myPipe, hisPipe = os.pipe() + + poller = select.poll() + poller.register(myPipe) + + intrThread = threading.Thread(target=_sigChldAndWrite, args=(hisPipe,)) + intrThread.setDaemon(True) + intrThread.start() + + try: + self.assertTrue(len(misc.NoIntrPoll(poller.poll, -1)) > 0) + finally: + os.close(myPipe) + def testClosedPipe(self): def _closePipe(pipe): time.sleep(self.SLEEP_INTERVAL) diff --git a/vdsm/storage/misc.py b/vdsm/storage/misc.py index b1143be..d27fe43 100644 --- a/vdsm/storage/misc.py +++ b/vdsm/storage/misc.py @@ -1361,12 +1361,14 @@

def NoIntrPoll(pollfun, timeout=-1): """ - This wrapper is used to handle the interrupt exceptions that might occur - during a poll system call. The wrapped function must be defined as - poll([timeout]) where the special timeout value 0 is used to return + This wrapper is used to handle the interrupt exceptions that might + occur during a poll system call. The wrapped function must be defined + as poll([timeout]) where the special timeout value 0 is used to return immediately and -1 is used to wait indefinitely. """ - endtime = time.time() + timeout + # When the timeout < 0 we shouldn't compute a new timeout after an + # interruption. + endtime = None if timeout < 0 else time.time() + timeout

while True: try: @@ -1374,7 +1376,9 @@ except (IOError, select.error) as e: if e.args[0] != errno.EINTR: raise - timeout = max(0, endtime - time.time()) + + if endtime is not None: + timeout = max(0, endtime - time.time())

def isAscii(s):

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

Gerrit-MessageType: newchange Gerrit-Change-Id: Iefc4d17559d3335ef6699cb83923c3bd255c916b Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli fsimonce@redhat.com

Show replies by date

Jenkins CI RO

25 Jan 25 Jan

4:11 p.m.

oVirt Jenkins CI Server has posted comments on this change.

Change subject: misc: handle properly timeout=-1 in NoIntrPoll ......................................................................

Patch Set 1:

Build Started http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/909/ (1/3)

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

Jenkins CI RO

4:11 p.m.

oVirt Jenkins CI Server has posted comments on this change.

Change subject: misc: handle properly timeout=-1 in NoIntrPoll ......................................................................

Patch Set 1:

Build Started http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/944/ (2/3)

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

Jenkins CI RO

4:11 p.m.

oVirt Jenkins CI Server has posted comments on this change.

Change subject: misc: handle properly timeout=-1 in NoIntrPoll ......................................................................

Patch Set 1:

Build Started http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/54/ (3/3)

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

Jenkins CI RO

4:12 p.m.

oVirt Jenkins CI Server has posted comments on this change.

Change subject: misc: handle properly timeout=-1 in NoIntrPoll ......................................................................

Patch Set 1: Fails

Build Failed

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

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/944/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/54/ : FAILURE

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

Gerrit-MessageType: comment Gerrit-Change-Id: Iefc4d17559d3335ef6699cb83923c3bd255c916b Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Royce Lv lvroyce@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server

Federico Simoncelli

4:15 p.m.

Federico Simoncelli has posted comments on this change.

Change subject: misc: handle properly timeout=-1 in NoIntrPoll ......................................................................

Patch Set 1: Verified

Verified with the provided test (both positive and negative).

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

Gerrit-MessageType: comment Gerrit-Change-Id: Iefc4d17559d3335ef6699cb83923c3bd255c916b Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Royce Lv lvroyce@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server

Dan Kenigsberg

26 Jan 26 Jan

9:43 p.m.

Dan Kenigsberg has posted comments on this change.

Change subject: misc: handle properly timeout=-1 in NoIntrPoll ......................................................................

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

(3 inline comments)

.................................................... File tests/miscTests.py Line 1168: myPipe, hisPipe = os.pipe() Line 1169: self._startFakeSigchld() Line 1170: self._noIntrWatchFd(myPipe, isEpoll=False) # caught select.error Line 1171: Line 1172: def testNoTimeoutPipePoll(self): it might be the time of day, it might be me, but I do not see what you are trying to test here and why. I could have used a more fat commit message. Line 1173: def _sigChldAndWrite(fd): Line 1174: self._waitAndSigchld() Line 1175: time.sleep(self.SLEEP_INTERVAL) Line 1176: os.close(fd)

Line 1169: self._startFakeSigchld() Line 1170: self._noIntrWatchFd(myPipe, isEpoll=False) # caught select.error Line 1171: Line 1172: def testNoTimeoutPipePoll(self): Line 1173: def _sigChldAndWrite(fd): where is the "write" here? Line 1174: self._waitAndSigchld() Line 1175: time.sleep(self.SLEEP_INTERVAL) Line 1176: os.close(fd) Line 1177:

if timeout > 0:

instead of adding None to the possible values of endtime. Line 1381: timeout = max(0, endtime - time.time()) Line 1382: Line 1383: Line 1384: def isAscii(s):

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

Gerrit-MessageType: comment Gerrit-Change-Id: Iefc4d17559d3335ef6699cb83923c3bd255c916b Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Royce Lv lvroyce@linux.vnet.ibm.com

lvroyce＠linux.vnet.ibm.com

28 Jan 28 Jan

10:02 a.m.

Royce Lv has posted comments on this change.

Change subject: misc: handle properly timeout=-1 in NoIntrPoll ......................................................................

Patch Set 1: (2 inline comments)

.................................................... File tests/miscTests.py Line 1171: Line 1172: def testNoTimeoutPipePoll(self): Line 1173: def _sigChldAndWrite(fd): Line 1174: self._waitAndSigchld() Line 1175: time.sleep(self.SLEEP_INTERVAL) I'm not sure why do we sleep here? Line 1176: os.close(fd) Line 1177: Line 1178: myPipe, hisPipe = os.pipe() Line 1179:

Line 1184: intrThread.setDaemon(True) Line 1185: intrThread.start() Line 1186: Line 1187: try: Line 1188: self.assertTrue(len(misc.NoIntrPoll(poller.poll, -1)) > 0) This testcase proves infinite poll can catch POLLHUP after EINTR occurs, can you register fd with POLLHUP mask to make it more clear? Line 1189: finally: Line 1190: os.close(myPipe) Line 1191: Line 1192: def testClosedPipe(self):

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

Gerrit-MessageType: comment Gerrit-Change-Id: Iefc4d17559d3335ef6699cb83923c3bd255c916b Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Royce Lv lvroyce@linux.vnet.ibm.com

Federico Simoncelli

1:52 p.m.

Federico Simoncelli has posted comments on this change.

Change subject: misc: handle properly timeout=-1 in NoIntrPoll ......................................................................

Patch Set 1: (2 inline comments)

.................................................... File tests/miscTests.py Line 1171: Line 1172: def testNoTimeoutPipePoll(self): Line 1173: def _sigChldAndWrite(fd): Line 1174: self._waitAndSigchld() Line 1175: time.sleep(self.SLEEP_INTERVAL) We're sleeping here because we need to give some time to the main thread to receive the signal and loop inside NoIntrPoll (and eventually exit for the bugged NoIntrPoll). Line 1176: os.close(fd) Line 1177: Line 1178: myPipe, hisPipe = os.pipe() Line 1179:

Line 1184: intrThread.setDaemon(True) Line 1185: intrThread.start() Line 1186: Line 1187: try: Line 1188: self.assertTrue(len(misc.NoIntrPoll(poller.poll, -1)) > 0) Done Line 1189: finally: Line 1190: os.close(myPipe) Line 1191: Line 1192: def testClosedPipe(self):

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

Gerrit-MessageType: comment Gerrit-Change-Id: Iefc4d17559d3335ef6699cb83923c3bd255c916b Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Royce Lv lvroyce@linux.vnet.ibm.com

Federico Simoncelli

6 Feb 6 Feb

2:26 p.m.

Federico Simoncelli has posted comments on this change.

Change subject: misc: handle properly timeout=-1 in NoIntrPoll ......................................................................

Patch Set 1: (1 inline comment)

.................................................... File vdsm/storage/misc.py Line 1376: except (IOError, select.error) as e: Line 1377: if e.args[0] != errno.EINTR: Line 1378: raise Line 1379: Line 1380: if endtime is not None: We can use "timeout" here as you suggest but nonetheless the operand that can cause trouble is "endtime" (see next line). I prefer to test the relevant operand rather than a different variable on which it is depending. Line 1381: timeout = max(0, endtime - time.time()) Line 1382: Line 1383: Line 1384: def isAscii(s):

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

Gerrit-MessageType: comment Gerrit-Change-Id: Iefc4d17559d3335ef6699cb83923c3bd255c916b Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Royce Lv lvroyce@linux.vnet.ibm.com

Jenkins CI RO

2:34 p.m.

oVirt Jenkins CI Server has posted comments on this change.

Change subject: misc: handle properly timeout=-1 in NoIntrPoll ......................................................................

Patch Set 2:

Build Started http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/1198/ (2/3)

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

Gerrit-MessageType: comment Gerrit-Change-Id: Iefc4d17559d3335ef6699cb83923c3bd255c916b Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Royce Lv lvroyce@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server

Jenkins CI RO

2:34 p.m.

oVirt Jenkins CI Server has posted comments on this change.

Change subject: misc: handle properly timeout=-1 in NoIntrPoll ......................................................................

Patch Set 2:

Build Started http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/1163/ (1/3)

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

Jenkins CI RO

2:34 p.m.

oVirt Jenkins CI Server has posted comments on this change.

Change subject: misc: handle properly timeout=-1 in NoIntrPoll ......................................................................

Patch Set 2:

Build Started http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/309/ (3/3)

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

Jenkins CI RO

2:37 p.m.

oVirt Jenkins CI Server has posted comments on this change.

Change subject: misc: handle properly timeout=-1 in NoIntrPoll ......................................................................

Patch Set 2:

Build Successful

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

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/1198/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/309/ : SUCCESS

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

Jenkins CI RO

2:42 p.m.

oVirt Jenkins CI Server has posted comments on this change.

Change subject: misc: handle properly timeout=-1 in NoIntrPoll ......................................................................

Patch Set 3:

Build Started http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/310/ (2/3)

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

Gerrit-MessageType: comment Gerrit-Change-Id: Iefc4d17559d3335ef6699cb83923c3bd255c916b Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Royce Lv lvroyce@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server

Jenkins CI RO

2:42 p.m.

oVirt Jenkins CI Server has posted comments on this change.

Change subject: misc: handle properly timeout=-1 in NoIntrPoll ......................................................................

Patch Set 3:

Build Started http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/1199/ (3/3)

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

Jenkins CI RO

2:42 p.m.

oVirt Jenkins CI Server has posted comments on this change.

Change subject: misc: handle properly timeout=-1 in NoIntrPoll ......................................................................

Patch Set 3:

Build Started http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/1164/ (1/3)

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

Jenkins CI RO

2:45 p.m.

oVirt Jenkins CI Server has posted comments on this change.

Change subject: misc: handle properly timeout=-1 in NoIntrPoll ......................................................................

Patch Set 3:

Build Successful

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

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/1199/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/310/ : SUCCESS

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

Federico Simoncelli

7 Feb 7 Feb

12:53 a.m.

Federico Simoncelli has posted comments on this change.

Change subject: misc: handle properly timeout=-1 in NoIntrPoll ......................................................................

Patch Set 3: Verified

Verified with the provided test (both positive and negative).

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

Dan Kenigsberg

9 Feb 9 Feb

7:21 p.m.

Dan Kenigsberg has posted comments on this change.

Change subject: misc: handle properly timeout=-1 in NoIntrPoll ......................................................................

Patch Set 3: Looks good to me, approved

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

Dan Kenigsberg

7:21 p.m.

Dan Kenigsberg has submitted this change and it was merged.

Change subject: misc: handle properly timeout=-1 in NoIntrPoll ......................................................................

misc: handle properly timeout=-1 in NoIntrPoll

The parameter timeout=-1 (no timeout) of NoIntrPoll wasn't properly handled by the previous implementation; it was in fact returning an empty set of file descriptors if the process was interrupted by a signal. This patch ensures that the parameter is taken in account (do not return until there is an event) even when the process is interrupted.

Approvals: Federico Simoncelli: Verified Dan Kenigsberg: Looks good to me, approved

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

Gerrit-MessageType: merged Gerrit-Change-Id: Iefc4d17559d3335ef6699cb83923c3bd255c916b Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Royce Lv lvroyce@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server

4159

Age (days ago)

4174

Last active (days ago)

vdsm-patches@lists.fedorahosted.org

20 comments

4 participants

tags (0)

participants (4)

Dan Kenigsberg
Federico Simoncelli
Jenkins CI RO
lvroyce＠linux.vnet.ibm.com