Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(449)

Issue 1132041: Add signal.{signalfd,sigprocmask}

Can't Edit
Can't Publish+Mail
Start Review
Created:
13 years, 11 months ago by jcalderone
Modified:
13 years, 9 months ago
Base URL:
http://svn.python.org/view/*checkout*/python/trunk/
Visibility:
Public.

Patch Set 1 #

Total comments: 47

Patch Set 2 : ported to py3k, addressed all previous review comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+475 lines, -7 lines) Patch
M Doc/glossary.rst View 1 chunk +10 lines, -0 lines 0 comments Download
M Doc/library/signal.rst View 1 3 chunks +78 lines, -3 lines 0 comments Download
M Lib/test/test_signal.py View 1 1 chunk +198 lines, -2 lines 0 comments Download
M Misc/NEWS View 1 1 chunk +5 lines, -0 lines 0 comments Download
M Modules/signalmodule.c View 1 4 chunks +176 lines, -0 lines 0 comments Download
M configure View 1 1 chunk +1 line, -1 line 0 comments Download
M configure.in View 1 1 chunk +1 line, -1 line 0 comments Download
M pyconfig.h.in View 1 1 chunk +6 lines, -0 lines 0 comments Download

Messages

Total messages: 6
brian.curtin
Here are a few fairly minor comments. Nothing looks out of place or wrong to ...
13 years, 11 months ago (2010-05-06 04:53:56 UTC) #1
Antoine Pitrou
http://codereview.appspot.com/1132041/diff/1/5 File Doc/library/signal.rst (right): http://codereview.appspot.com/1132041/diff/1/5#newcode150 Doc/library/signal.rst:150: the new file description to be set non-blocking. description ...
13 years, 11 months ago (2010-05-06 10:45:11 UTC) #2
jcalderone
Thanks very much for the reviews. I've addressed most of the points raised. There are ...
13 years, 11 months ago (2010-05-07 06:28:08 UTC) #3
Antoine Pitrou
http://codereview.appspot.com/1132041/diff/1/5 File Doc/library/signal.rst (right): http://codereview.appspot.com/1132041/diff/1/5#newcode150 Doc/library/signal.rst:150: the new file description to be set non-blocking. On ...
13 years, 11 months ago (2010-05-07 07:24:54 UTC) #4
gregory.p.smith
http://codereview.appspot.com/1132041/diff/1/5 File Doc/library/signal.rst (right): http://codereview.appspot.com/1132041/diff/1/5#newcode277 Doc/library/signal.rst:277: Availability: Linux (See the manpage :manpage:`signalfd(2)` for further fyi ...
13 years, 10 months ago (2010-05-08 05:37:52 UTC) #5
jcalderone
13 years, 9 months ago (2010-06-19 21:09:57 UTC) #6
http://codereview.appspot.com/1132041/diff/1/5
File Doc/library/signal.rst (right):

http://codereview.appspot.com/1132041/diff/1/5#newcode150
Doc/library/signal.rst:150: the new file description to be set non-blocking.
I just ended up adding some emphasis.

http://codereview.appspot.com/1132041/diff/1/5#newcode277
Doc/library/signal.rst:277: Availability: Linux (See the manpage
:manpage:`signalfd(2)` for further
It looks like :platform: is only for modules, so I left it as it was.

http://codereview.appspot.com/1132041/diff/1/6
File Lib/test/test_signal.py (right):

http://codereview.appspot.com/1132041/diff/1/6#newcode497
Lib/test/test_signal.py:497: class SignalfdTests(unittest.TestCase):
Done.

http://codereview.appspot.com/1132041/diff/1/6#newcode557
Lib/test/test_signal.py:557: self.assertTrue(bytes)
I suppose I could do that.  Do you think I should?  I was thinking people could
look at the signalfd man page and figure it out from the struct, though (which
is 128 bytes, conveniently added up for us in the comment for the `pad` member).

It's unlikely that the size (or layout) of the structure will change, since it's
essentially part of the kernel ABI.

http://codereview.appspot.com/1132041/diff/1/7
File Modules/signalmodule.c (right):

http://codereview.appspot.com/1132041/diff/1/7#newcode487
Modules/signalmodule.c:487: PyOS_snprintf(range_buffer, sizeof(range_buffer),
range_format, signum);
Ah, thanks.  Fixed.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b