STINNER Victor <victor.stin...@haypocalc.com> added the comment:

>> It is possible to pass a negative timeout
> It now raises an exception like select.

Great.

>> According to the manual page, sigwaitinfo() or sigtimedwait() 
>> can be interrupted (EINTR)
> Actually, PyErr_SetFromErrno() does this implicitly. I added a test case 
> anyway.

I would prefer an explicit call to PyErr_CheckSignals(), but if there is a 
test, it's just fine.

> So I've left out si_addr (I don't think it's needed)
> ... and si_value (not sure what it's for or how to represent a union :-))

I don't think that it means something in Python to have an address or this 
low-level "value" field. If someone needs them, we would need an use case with 
an example (to test it!). (So ok to not expose them)

> ... si_band (we don't have SIGPOLL)

What do you mean? signal.SIGPOLL exists in Python 3.3.

>> sigtimedwait() raises a OSError(EGAIN) on timeout.
> It now returns None.

Great.

--

>> I will do a deeper review on the second version of your patch :-)
> How much more in depth can it get ;-) ?

Here you have, issue12303_v2.patch:

 - "PyStructSequence_SET_ITEM(result, 4, PyLong_FromPid(si->si_uid));" looks 
wrong (we don't need to call PyLong_FromLongLong): si_uid type is uid_t, not 
pid_t. posix_getuid() simply uses PyLong_FromLong((long)getuid()).

 - sigwaitinfo() doc doesn't mention that the function can be interrupted if an 
unexpected signal is received. I don't know if it should be mentionned. It is 
mentionned in the manpage, in the ERRORS section (EINTR).

 - tests: test_sigtimedwait_negative_timeout() doesn't need to use 
_wait_helper() (which creates a subprocess!). You may also test (-1, 0) and (0, 
-1) timeouts.

 - test_sigwaitinfo(), test_sigtimedwait_poll(), test_sigwaitinfo_interrupted() 
are called from a child process. In test_wait(), I chose to write manually to 
stdout and call os._exit(1) (oh, I forgot an explicit sys.stdout.flush()). I 
don't know if you can use TestCase methods in a child process (I don't know 
what is written to stdout, _wait_helper() calls os._exit).

 - you may want to prepare the "What's new in Python 3.3" document (mention the 
2 new functions)

 - style: _wait_helper(): you don't have to mark the helper as private (-> 
wait_helper())

 - style: _fill_siginfo() is already a static function you don't need a "_" 
prefix

----------

_______________________________________
Python tracker <rep...@bugs.python.org>
<http://bugs.python.org/issue12303>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

Reply via email to