Message138942
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 ;-) ? |
|
Date |
User |
Action |
Args |
2011-06-24 13:28:50 | rosslagerwall | set | recipients:
+ rosslagerwall, vstinner, giampaolo.rodola |
2011-06-24 13:28:49 | rosslagerwall | set | messageid: <1308922129.65.0.335775008549.issue12303@psf.upfronthosting.co.za> |
2011-06-24 13:28:49 | rosslagerwall | link | issue12303 messages |
2011-06-24 13:28:48 | rosslagerwall | create | |
|