On Sat, Jan 21, 2012 at 4:21 PM, Benjamin Peterson <benja...@python.org> wrote:
> 2012/1/21 gregory.p.smith <python-check...@python.org>:
> ...
>> +/* Convert ASCII to a positive int, no libc call. no overflow. -1 on error.
>> */
>
> Is no libc call important?
Yes. strtol() is not on the async signal safe C library function list.
>
>> +static int _pos_int_from_ascii(char *name)
>
> To be consistent with the rest of posixmodule.c, "static int" should
> be on a different line from the signature. This also applies to all
> other function declarations added by this.
Python C style as a whole, yes. This file already has a mix of same
line vs two line declarations, I added these following the style of
the functions immediately surrounding them. Want a style fixup on the
whole file?
>
>> +{
>> + int num = 0;
>> + while (*name >= '0' && *name <= '9') {
>> + num = num * 10 + (*name - '0');
>> + ++name;
>> + }
>> + if (*name)
>> + return -1; /* Non digit found, not a number. */
>> + return num;
>> +}
>> +
>> +
>> +/* Returns 1 if there is a problem with fd_sequence, 0 otherwise. */
>> +static int _sanity_check_python_fd_sequence(PyObject *fd_sequence)
>> +{
>> + Py_ssize_t seq_idx, seq_len = PySequence_Length(fd_sequence);
>
> PySequence_Length can fail.
It has already been checked not to by the only entry point into the
code in this file.
>
>> + long prev_fd = -1;
>> + for (seq_idx = 0; seq_idx < seq_len; ++seq_idx) {
>> + PyObject* py_fd = PySequence_Fast_GET_ITEM(fd_sequence, seq_idx);
>> + long iter_fd = PyLong_AsLong(py_fd);
>> + if (iter_fd < 0 || iter_fd < prev_fd || iter_fd > INT_MAX) {
>> + /* Negative, overflow, not a Long, unsorted, too big for a fd.
>> */
>> + return 1;
>> + }
>> + }
>> + return 0;
>> +}
>> +
>> +
>> +/* Is fd found in the sorted Python Sequence? */
>> +static int _is_fd_in_sorted_fd_sequence(int fd, PyObject *fd_sequence)
>> +{
>> + /* Binary search. */
>> + Py_ssize_t search_min = 0;
>> + Py_ssize_t search_max = PySequence_Length(fd_sequence) - 1;
>> + if (search_max < 0)
>> + return 0;
>> + do {
>> + long middle = (search_min + search_max) / 2;
>> + long middle_fd = PyLong_AsLong(
>> + PySequence_Fast_GET_ITEM(fd_sequence, middle));
>
> No check for error?
_sanity_check_python_fd_sequence() already checked the entire list to
guarantee that there would not be any such error.
>> + if (fd == middle_fd)
>> + return 1;
>> + if (fd > middle_fd)
>> + search_min = middle + 1;
>> + else
>> + search_max = middle - 1;
>> + } while (search_min <= search_max);
>> + return 0;
>> +}
In general this is an extension module that is best viewed as a whole
including its existing comments rather than as a diff.
It contains code that will look "odd" in a diff because much of it
executes in a path where not much is allowed (post fork, pre exec) and
no useful way of responding to an error is possible so it attempts to
pre-check for any possible errors up front so that later code that is
unable to handle errors cannot possibly fail.
-gps
_______________________________________________
Python-Dev mailing list
Python-Dev@python.org
http://mail.python.org/mailman/listinfo/python-dev
Unsubscribe:
http://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com
