Add signal.{signalfd,sigprocmask}

[brian.curtin, 2010-05-06 04:53:56]

Here are a few fairly minor comments. Nothing looks out of place or wrong to me,
but I don't have enough experience in this area to be reviewing the
implementation for it's correctness.

http://codereview.appspot.com/1132041/diff/1/5
File Doc/library/signal.rst (right):

http://codereview.appspot.com/1132041/diff/1/5#newcode286
Doc/library/signal.rst:286: *flags* is a bitvector which may include any
:const:`signal.SFD_*` flag.
"bit mask" is used elsewhere in the docs for this same purpose

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

http://codereview.appspot.com/1132041/diff/1/6#newcode421
Lib/test/test_signal.py:421: """A signal handler which raises SomeException.
No need for the closing """ on a new line in a one-liner

http://codereview.appspot.com/1132041/diff/1/6#newcode429
Lib/test/test_signal.py:429: Tests for sigprocmask.
could be on one line

http://codereview.appspot.com/1132041/diff/1/6#newcode447
Lib/test/test_signal.py:447: """If a valid other than SIG_BLOCK, SIG_UNBLOCK, or
SIG_SETMASK is
probably valid->value

http://codereview.appspot.com/1132041/diff/1/6#newcode472
Lib/test/test_signal.py:472: try:
Could use...
with self.assertRaises(SomeException):
    signal.sigprocmask(signal.SIG_SETMASK, previous)

(same with test_unblock below)

[Antoine Pitrou, 2010-05-06 10:45:11]

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 -> descriptor?

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

http://codereview.appspot.com/1132041/diff/1/6#newcode454
Lib/test/test_signal.py:454: self.assertEquals(str(exc), "value specified for
how (1700) invalid")
You could use assertRaisesRegexp instead. Also, it is useless to test for the
type of `exc`, since it's already done by assertRaises.

http://codereview.appspot.com/1132041/diff/1/6#newcode490
Lib/test/test_signal.py:490: time.sleep(1)
Why do you need time.sleep() here and not in test_block? The two tests should be
consistent in their expectations, IMO.

Also, as Brian said above, you could use assertRaises.

http://codereview.appspot.com/1132041/diff/1/6#newcode543
Lib/test/test_signal.py:543: self.assertEquals(str(exc), "signal number -2 out
of range")
Same remarks as in test_invalid_how.

http://codereview.appspot.com/1132041/diff/1/6#newcode557
Lib/test/test_signal.py:557: self.assertTrue(bytes)
The signalfd() man page mentions that the bytes read from the file descriptor
map to a signalfd_siginfo struct. Would it be worth providing a
read_signalfd_siginfo(fd) helper func to read this struct (for example as a
tuple, or structseq, or dict)?

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

http://codereview.appspot.com/1132041/diff/1/7#newcode481
Modules/signalmodule.c:481: for (item = PyIter_Next(iterator); item; item =
PyIter_Next(iterator)) {
`while (item = PyIter_Next(iterator))` looks more natural.

http://codereview.appspot.com/1132041/diff/1/7#newcode482
Modules/signalmodule.c:482: int signum = PyInt_AsLong(item);
You should accept non-ints such as longs. Using PyNumber_Int(), for example.

Also, you must Py_DECREF(item) as soon as you're done with it.

http://codereview.appspot.com/1132041/diff/1/7#newcode484
Modules/signalmodule.c:484: return -1;
You must first Py_DECREF(iterator) (same for other returns in this function).

http://codereview.appspot.com/1132041/diff/1/7#newcode487
Modules/signalmodule.c:487: PyOS_snprintf(range_buffer, sizeof(range_buffer),
range_format, signum);
Just use PyErr_SetFormat().

http://codereview.appspot.com/1132041/diff/1/7#newcode526
Modules/signalmodule.c:526: if (!valid || PY_SIGMASK(how, &mask, &previous) ==
-1) {
The man page for pthread_sigmask specifies that it can "return an error number",
not necessarily -1.

http://codereview.appspot.com/1132041/diff/1/7#newcode527
Modules/signalmodule.c:527: PyOS_snprintf(how_buffer, sizeof(how_buffer),
how_format, how);
You don't need PyOS_snprintf, just use PyErr_SetFormat.

http://codereview.appspot.com/1132041/diff/1/7#newcode553
Modules/signalmodule.c:553: }
You should also Py_DECREF(signum) if PyList_Append() succeeds.

http://codereview.appspot.com/1132041/diff/1/7#newcode594
Modules/signalmodule.c:594: "signalfd(fd, mask, flags)\n\
The flags argument doesn't seem to be handled or supported at all.

[jcalderone, 2010-05-07 06:28:08]

Thanks very much for the reviews.  I've addressed most of the points raised. 
There are a couple which I haven't, for which I've left replies explaining why
not or asking questions.

Thanks again.

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 2010/05/06 10:45:16, Antoine Pitrou wrote:
> description -> descriptor?
> 

Nope, "description" is correct.  Should I emphasize that in the docs somehow so
someone doesn't "correct" it later?

http://codereview.appspot.com/1132041/diff/1/5#newcode286
Doc/library/signal.rst:286: *flags* is a bitvector which may include any
:const:`signal.SFD_*` flag.
On 2010/05/06 04:53:56, brian.curtin wrote:
> "bit mask" is used elsewhere in the docs for this same purpose

Done.

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

http://codereview.appspot.com/1132041/diff/1/6#newcode421
Lib/test/test_signal.py:421: """A signal handler which raises SomeException.
On 2010/05/06 04:53:56, brian.curtin wrote:
> No need for the closing """ on a new line in a one-liner

Done.

http://codereview.appspot.com/1132041/diff/1/6#newcode429
Lib/test/test_signal.py:429: Tests for sigprocmask.
On 2010/05/06 04:53:56, brian.curtin wrote:
> could be on one line

Done.

http://codereview.appspot.com/1132041/diff/1/6#newcode447
Lib/test/test_signal.py:447: """If a valid other than SIG_BLOCK, SIG_UNBLOCK, or
SIG_SETMASK is
On 2010/05/06 04:53:56, brian.curtin wrote:
> probably valid->value

Done.

http://codereview.appspot.com/1132041/diff/1/6#newcode454
Lib/test/test_signal.py:454: self.assertEquals(str(exc), "value specified for
how (1700) invalid")
On 2010/05/06 10:45:16, Antoine Pitrou wrote:
> You could use assertRaisesRegexp instead. Also, it is useless to test for the
> type of `exc`, since it's already done by assertRaises.
> 

Done.  Although this complicates the message test slightly, since () have
special meaning in a regexp.

http://codereview.appspot.com/1132041/diff/1/6#newcode472
Lib/test/test_signal.py:472: try:
On 2010/05/06 04:53:56, brian.curtin wrote:
> Could use...
> with self.assertRaises(SomeException):
>     signal.sigprocmask(signal.SIG_SETMASK, previous)
> 
> (same with test_unblock below)

Done.

http://codereview.appspot.com/1132041/diff/1/6#newcode490
Lib/test/test_signal.py:490: time.sleep(1)
On 2010/05/06 10:45:16, Antoine Pitrou wrote:
> Why do you need time.sleep() here and not in test_block? 

Deleted.  It was necessary when the test was using SIGINT, but it's not needed
now that it uses SIGUSR1.

> The two tests should be consistent in their expectations, IMO.
> 
> Also, as Brian said above, you could use assertRaises.
>

http://codereview.appspot.com/1132041/diff/1/6#newcode543
Lib/test/test_signal.py:543: self.assertEquals(str(exc), "signal number -2 out
of range")
On 2010/05/06 10:45:16, Antoine Pitrou wrote:
> Same remarks as in test_invalid_how.
> 

Done.

http://codereview.appspot.com/1132041/diff/1/6#newcode557
Lib/test/test_signal.py:557: self.assertTrue(bytes)
On 2010/05/06 10:45:16, Antoine Pitrou wrote:
> The signalfd() man page mentions that the bytes read from the file descriptor
> map to a signalfd_siginfo struct. Would it be worth providing a
> read_signalfd_siginfo(fd) helper func to read this struct (for example as a
> tuple, or structseq, or dict)?
> 

It would probably be useful to have something like this, yes.  I'd rather have
an API for creating the structure from the right number of bytes rather than for
reading the bytes directly from the fd and constructing it, though.

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

http://codereview.appspot.com/1132041/diff/1/7#newcode481
Modules/signalmodule.c:481: for (item = PyIter_Next(iterator); item; item =
PyIter_Next(iterator)) {
On 2010/05/06 10:45:16, Antoine Pitrou wrote:
> `while (item = PyIter_Next(iterator))` looks more natural.
> 

Done.

http://codereview.appspot.com/1132041/diff/1/7#newcode482
Modules/signalmodule.c:482: int signum = PyInt_AsLong(item);
On 2010/05/06 10:45:16, Antoine Pitrou wrote:
> You should accept non-ints such as longs. Using PyNumber_Int(), for example.

I added a test that passes in longs for signal numbers.  It passed without
changing this code to PyNumber_Int, though.  Is there something more exotic I
should try?

> 
> Also, you must Py_DECREF(item) as soon as you're done with it.
> 

Done.

http://codereview.appspot.com/1132041/diff/1/7#newcode484
Modules/signalmodule.c:484: return -1;
On 2010/05/06 10:45:16, Antoine Pitrou wrote:
> You must first Py_DECREF(iterator) (same for other returns in this function).

Done.

http://codereview.appspot.com/1132041/diff/1/7#newcode487
Modules/signalmodule.c:487: PyOS_snprintf(range_buffer, sizeof(range_buffer),
range_format, signum);
On 2010/05/06 10:45:16, Antoine Pitrou wrote:
> Just use PyErr_SetFormat().
> 

There appears to be no such function?

http://codereview.appspot.com/1132041/diff/1/7#newcode526
Modules/signalmodule.c:526: if (!valid || PY_SIGMASK(how, &mask, &previous) ==
-1) {
On 2010/05/06 10:45:16, Antoine Pitrou wrote:
> The man page for pthread_sigmask specifies that it can "return an error
number",
> not necessarily -1.
> 

Ah, excellent!  That explains why it stopped failing for invalid values of "how"
when I made it use pthread_sigmask.  Fixing this error checking lets me delete
the "valid = ..." check immediately above.

http://codereview.appspot.com/1132041/diff/1/7#newcode527
Modules/signalmodule.c:527: PyOS_snprintf(how_buffer, sizeof(how_buffer),
how_format, how);
On 2010/05/06 10:45:16, Antoine Pitrou wrote:
> You don't need PyOS_snprintf, just use PyErr_SetFormat.
> 

I really must be missing something. :)

http://codereview.appspot.com/1132041/diff/1/7#newcode553
Modules/signalmodule.c:553: }
On 2010/05/06 10:45:16, Antoine Pitrou wrote:
> You should also Py_DECREF(signum) if PyList_Append() succeeds.
> 

Done.

http://codereview.appspot.com/1132041/diff/1/7#newcode594
Modules/signalmodule.c:594: "signalfd(fd, mask, flags)\n\
On 2010/05/06 10:45:16, Antoine Pitrou wrote:
> The flags argument doesn't seem to be handled or supported at all.
> 

Oh yes.  That's a pretty important thing I forgot.  Fortunately, quite simple. 
Done.

[Antoine Pitrou, 2010-05-07 07:24:54]

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 2010/05/07 06:28:08, jcalderone wrote:
> On 2010/05/06 10:45:16, Antoine Pitrou wrote:
> > description -> descriptor?
> > 
> 
> Nope, "description" is correct.  Should I emphasize that in the docs somehow
so
> someone doesn't "correct" it later?

I think you can put comments in a ReST document, but I don't know how.

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

http://codereview.appspot.com/1132041/diff/1/6#newcode557
Lib/test/test_signal.py:557: self.assertTrue(bytes)
On 2010/05/07 06:28:08, jcalderone wrote:
> On 2010/05/06 10:45:16, Antoine Pitrou wrote:
> > The signalfd() man page mentions that the bytes read from the file
descriptor
> > map to a signalfd_siginfo struct. Would it be worth providing a
> > read_signalfd_siginfo(fd) helper func to read this struct (for example as a
> > tuple, or structseq, or dict)?
> > 
> 
> It would probably be useful to have something like this, yes.  I'd rather have
> an API for creating the structure from the right number of bytes rather than
for
> reading the bytes directly from the fd and constructing it, though.

How would the user know the number of bytes to read, though? Would you make it a
module constant?

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

http://codereview.appspot.com/1132041/diff/1/7#newcode482
Modules/signalmodule.c:482: int signum = PyInt_AsLong(item);
On 2010/05/07 06:28:08, jcalderone wrote:
> On 2010/05/06 10:45:16, Antoine Pitrou wrote:
> > You should accept non-ints such as longs. Using PyNumber_Int(), for example.
> 
> I added a test that passes in longs for signal numbers.  It passed without
> changing this code to PyNumber_Int, though.  Is there something more exotic I
> should try?

No, I think it's enough.

http://codereview.appspot.com/1132041/diff/1/7#newcode487
Modules/signalmodule.c:487: PyOS_snprintf(range_buffer, sizeof(range_buffer),
range_format, signum);
On 2010/05/07 06:28:08, jcalderone wrote:
> On 2010/05/06 10:45:16, Antoine Pitrou wrote:
> > Just use PyErr_SetFormat().
> > 
> 
> There appears to be no such function?

Oops, sorry. It's PyErr_Format():
http://docs.python.org/dev/py3k/c-api/exceptions.html#PyErr_Format

[gregory.p.smith, 2010-05-08 05:37:52]

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 - I see

   :platform: Linux

used in other docs.  try that and see if it renders nicely.

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):
I think you need to add @unittest.Skip decorators to your unittests to avoid
trying to run them on platforms where the functions are not available.

[jcalderone, 2010-06-19 21:09:57]

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.