Ross Lagerwall <rosslagerw...@gmail.com> added the comment:
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 ;-) ?
----------
Added file: http://bugs.python.org/file22439/issue12303_v2.patch
_______________________________________
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
