This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

Author rosslagerwall
Recipients giampaolo.rodola, rosslagerwall, vstinner
Date 2011-06-24.13:28:47
SpamBayes Score 2.1116442e-12
Marked as misclassified No
Message-id <1308922129.65.0.335775008549.issue12303@psf.upfronthosting.co.za>
In-reply-to
Content
Thanks for the review.

> Why do you wait until the end of PyInit_signal() to set initialized to 1?
Fixed.

> If this variable is only present ... but I see that the init function of the posixmodule.c has also a static initialized variable.
I simply followed the same format of the posix module.

> "(If one of the signals in set is already pending for the calling thread,  sigwaitinfo() will return immediately with information about that signal.)"
Added.

> "If both fields of this structure are specified as  0,  a  poll  is  performed:  sigtimedwait() returns  immediately,  either with information about a signal that was pending for the caller, or with an error if none of the signals in set was pending."
Added.

> The manpage doesn't tell that the signal handler is not called, should we say it in the Python doc?
Added - let's rather be more explicit.

> Doc: you may add links between pause(), sigwait(), sigwaitinfo() and sigtimedwait() functions.
Added.

> The timeout is a tuple. Usually, Python uses float for timeout (e.g. see select.select). I suppose that you chose a tuple to keep the precision of the timespec structure. We may accept both types: (sec: int, nanosec: int) or sec: float. It would be nice to have the opinion of our time expect, Alexander Belopolsky.
The are some uses of this tuple that were added in #10812 (e.g. futimens) that was reviewed by Alexander.
However, there was a conflict about it at #11457.
I'd say for now, let's keep it as a tuple and if a decision is eventually made as to how to
represent nanosecond timestamps, we can change them all then.

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

> signal_sigwaitinfo() and signal_sigtimedwait() use iterable_to_sigset()
Fixed.

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

> Your patch doesn't touch configure nor pyconfig.h.in, only configure.in.
I prefer to keep the generated changes out of the patch. When I commit, I'll run autoreconf.

> siginfo_t structure contains more fields, but I don't know if we need all of them. It can be improved later.
POSIX 2008 specifies:
int           si_signo  Signal number. 
int           si_code   Signal code. 
int           si_errno  If non-zero, an errno value associated with 
                        this signal, as described in <errno.h>. 
pid_t         si_pid    Sending process ID. 
uid_t         si_uid    Real user ID of sending process. 
void         *si_addr   Address of faulting instruction. 
int           si_status Exit value or signal. 
long          si_band   Band event for SIGPOLL. 
union sigval  si_value  Signal value.

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

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

> test_sigtimedwait_timeout(): why do you call signal.alarm()?
Copy/paste error.

> You may also add a test for timeout=0, I suppose that it is a special timeout value.
Added.

> I will do a deeper review on the second version of your patch :-)
How much more in depth can it get ;-) ?
History
Date User Action Args
2011-06-24 13:28:50rosslagerwallsetrecipients: + rosslagerwall, vstinner, giampaolo.rodola
2011-06-24 13:28:49rosslagerwallsetmessageid: <1308922129.65.0.335775008549.issue12303@psf.upfronthosting.co.za>
2011-06-24 13:28:49rosslagerwalllinkissue12303 messages
2011-06-24 13:28:48rosslagerwallcreate