Implementing Solaris "poll" in the "select" module

[AntoinePitrou]

Here are some comments. I haven't tried your patch since I don't have a Solaris
box.

http://bugs.python.org/review/6397/diff/3606/11276
File Lib/test/test_devpoll.py (right):

http://bugs.python.org/review/6397/diff/3606/11276#newcode53
Lib/test/test_devpoll.py:53: raise RuntimeError("no pipes ready for writing")
You should use self.fail("...").

http://bugs.python.org/review/6397/diff/3606/11276#newcode61
Lib/test/test_devpoll.py:61: rd = random.choice(ready_readers)
I'm surprised there may be several ready readers. Shouldn't there be only one,
and precisely w2r[wr]?

http://bugs.python.org/review/6397/diff/3606/11276#newcode72
Lib/test/test_devpoll.py:72: def poll_unit_tests(self):
What is this method? If it isn't named test_<something>, it won't get run.

http://bugs.python.org/review/6397/diff/3606/11276#newcode74
Lib/test/test_devpoll.py:74: FD = 42
The whole "FD = 42" test seems fragile and unnecessary, since you also test
POLLNVAL later.

http://bugs.python.org/review/6397/diff/3606/11276#newcode90
Lib/test/test_devpoll.py:90: f.close()
Better to use ``with open(...) as f`` than manual closing.

http://bugs.python.org/review/6397/diff/3606/11276#newcode119
Lib/test/test_devpoll.py:119: def test_devpoll2(self):
Is this test meaningful? It doesn't seem to bring anything more.

http://bugs.python.org/review/6397/diff/3606/11276#newcode144
Lib/test/test_devpoll.py:144: def test_devpoll3(self):
It would be better to give meaningful names to test methods. e.g.
"test_overflow" instead of "test_devpoll3".

http://bugs.python.org/review/6397/diff/3606/11276#newcode153
Lib/test/test_devpoll.py:153: self.fail('Overflow must have occurred')
I don't think these last lines should be kept. They probably tested poll()
against a known regression of some sort.

http://bugs.python.org/review/6397/diff/3606/11277
File Modules/selectmodule.c (right):

http://bugs.python.org/review/6397/diff/3606/11277#newcode685
Modules/selectmodule.c:685: return !0;
Er... please use "-1", not "!0".

http://bugs.python.org/review/6397/diff/3606/11277#newcode688
Modules/selectmodule.c:688: PyErr_SetString(PyExc_IOError, "failed to write all
pollfds");
Are you sure this is an error? Shouldn't you try to write() until everything is
written out?

http://bugs.python.org/review/6397/diff/3606/11277#newcode794
Modules/selectmodule.c:794: if (devpoll_flush(self))
Better to call write() *after* parsing the arguments, IMO.

http://bugs.python.org/review/6397/diff/3606/11277#newcode840
Modules/selectmodule.c:840: value = PyTuple_New(2);
You could use Py_BuildValue() or PyTuple_Pack() instead of all this.

http://bugs.python.org/review/6397/diff/3606/11277#newcode911
Modules/selectmodule.c:911: PyErr_NoMemory();
You need to close(fd_devpoll) here.

http://bugs.python.org/review/6397/diff/3606/11277#newcode917
Modules/selectmodule.c:917: return NULL;
You need to close(fd_devpoll) and PyMem_DEL(fds) here.

http://bugs.python.org/review/6397/diff/3606/11280
File pyconfig.h.in (right):

http://bugs.python.org/review/6397/diff/3606/11280#newcode591
pyconfig.h.in:591: #undef HAVE_SYS_DEVPOLL_H
This file looked alphabetically ordered :)

[jcea]

Thanks for the detailed review, Antoine. I just have a question: test_devpoll.py
is actually a literal copy of current test_poll.py. Try doing "diff -u
Lib/test/test_poll.py Lib/test/test_devpoll.py".

Should I apply your suggestions only to "test_devpoll.py" or improve
"test_poll.py" in the process?.

[jcea]

Pending to know if I must change the original "test_poll.py" code.

http://bugs.python.org/review/6397/diff/3606/11277
File Modules/selectmodule.c (right):

http://bugs.python.org/review/6397/diff/3606/11277#newcode685
Modules/selectmodule.c:685: return !0;
On 2011/11/08 15:20:39, AntoinePitrou wrote:
> Er... please use "-1", not "!0".
> 

Done.

http://bugs.python.org/review/6397/diff/3606/11277#newcode688
Modules/selectmodule.c:688: PyErr_SetString(PyExc_IOError, "failed to write all
pollfds");
On 2011/11/08 15:20:39, AntoinePitrou wrote:
> Are you sure this is an error? Shouldn't you try to write() until everything
is
> written out?
> 

Solaris "/dev/poll" manpage does this in the example.

Also in examples posted in Sun website:

http://developers.sun.com/solaris/articles/polling_efficient_devpollperf.c

http://developers.sun.com/solaris/articles/using_devpoll.pdf Page 8-9

http://bugs.python.org/review/6397/diff/3606/11277#newcode794
Modules/selectmodule.c:794: if (devpoll_flush(self))
On 2011/11/08 15:20:39, AntoinePitrou wrote:
> Better to call write() *after* parsing the arguments, IMO.
> 

It is safe to call the write anytime. The buffering is only for performance
reasons. In this case, even if the arguments are wrong, the kernel data will be
updated, and that is OK.

http://bugs.python.org/review/6397/diff/3606/11277#newcode840
Modules/selectmodule.c:840: value = PyTuple_New(2);
On 2011/11/08 15:20:39, AntoinePitrou wrote:
> You could use Py_BuildValue() or PyTuple_Pack() instead of all this.
> 

You are right. I just copied this from the somewhere else of the previous poll
code. The gain is not big, but the code is more readable.

Done.

http://bugs.python.org/review/6397/diff/3606/11277#newcode911
Modules/selectmodule.c:911: PyErr_NoMemory();
On 2011/11/08 15:20:39, AntoinePitrou wrote:
> You need to close(fd_devpoll) here.
> 

Done.

http://bugs.python.org/review/6397/diff/3606/11277#newcode917
Modules/selectmodule.c:917: return NULL;
On 2011/11/08 15:20:39, AntoinePitrou wrote:
> You need to close(fd_devpoll) and PyMem_DEL(fds) here.
> 
> 

Done.

Is it OK to call PyMem_DEL and then return NULL expecting that PyMEM_DEL didn't
mess with the original exception?

http://bugs.python.org/review/6397/diff/3606/11280
File pyconfig.h.in (right):

http://bugs.python.org/review/6397/diff/3606/11280#newcode591
pyconfig.h.in:591: #undef HAVE_SYS_DEVPOLL_H
On 2011/11/08 15:20:39, AntoinePitrou wrote:
> This file looked alphabetically ordered :)
> 

Done.

[jcea]

Please, check the new changeset, with all your feedback. Thanks!.

http://bugs.python.org/review/6397/diff/3606/11276
File Lib/test/test_devpoll.py (right):

http://bugs.python.org/review/6397/diff/3606/11276#newcode53
Lib/test/test_devpoll.py:53: raise RuntimeError("no pipes ready for writing")
On 2011/11/08 15:20:39, AntoinePitrou wrote:
> You should use self.fail("...").
> 

Done.

http://bugs.python.org/review/6397/diff/3606/11276#newcode61
Lib/test/test_devpoll.py:61: rd = random.choice(ready_readers)
On 2011/11/08 15:20:39, AntoinePitrou wrote:
> I'm surprised there may be several ready readers. Shouldn't there be only one,
> and precisely w2r[wr]?
> 

You are right. Incorporating that to the test.

Done.

http://bugs.python.org/review/6397/diff/3606/11276#newcode72
Lib/test/test_devpoll.py:72: def poll_unit_tests(self):
On 2011/11/08 15:20:39, AntoinePitrou wrote:
> What is this method? If it isn't named test_<something>, it won't get run.
> 

test_poll.py needs some love...

Deleting.

http://bugs.python.org/review/6397/diff/3606/11276#newcode74
Lib/test/test_devpoll.py:74: FD = 42
On 2011/11/08 15:20:39, AntoinePitrou wrote:
> The whole "FD = 42" test seems fragile and unnecessary, since you also test
> POLLNVAL later.
> 

All this function has been deleted.

http://bugs.python.org/review/6397/diff/3606/11276#newcode90
Lib/test/test_devpoll.py:90: f.close()
On 2011/11/08 15:20:39, AntoinePitrou wrote:
> Better to use ``with open(...) as f`` than manual closing.
> 

All this function has been deleted.

http://bugs.python.org/review/6397/diff/3606/11276#newcode119
Lib/test/test_devpoll.py:119: def test_devpoll2(self):
On 2011/11/08 15:20:39, AntoinePitrou wrote:
> Is this test meaningful? It doesn't seem to bring anything more.
> 

Deleting.

http://bugs.python.org/review/6397/diff/3606/11276#newcode144
Lib/test/test_devpoll.py:144: def test_devpoll3(self):
On 2011/11/08 15:20:39, AntoinePitrou wrote:
> It would be better to give meaningful names to test methods. e.g.
> "test_overflow" instead of "test_devpoll3".
> 

I agree. Done.

I modify the module to check for "timeout" overflows and manage them correctly.
Add tests for this.

http://bugs.python.org/review/6397/diff/3606/11276#newcode153
Lib/test/test_devpoll.py:153: self.fail('Overflow must have occurred')
On 2011/11/08 15:20:39, AntoinePitrou wrote:
> I don't think these last lines should be kept. They probably tested poll()
> against a known regression of some sort.
> 

I agree. This seems to be something forgotten...

[AntoinePitrou]

Hi,

On 2011/11/08 18:53:33, jcea wrote:
> On 2011/11/08 15:20:39, AntoinePitrou wrote:
> > Are you sure this is an error? Shouldn't you try to write() until everything
> is
> > written out?
> > 
> 
> Solaris "/dev/poll" manpage does this in the example.
> 
> Also in examples posted in Sun website:
> 
> http://developers.sun.com/solaris/articles/polling_efficient_devpollperf.c
> 
> http://developers.sun.com/solaris/articles/using_devpoll.pdf Page 8-9

I wouldn't really trust these examples. udpdevpoll.c calls close() on an
initialized variable!
But, if you say it works, I think it's ok anyway. Someone will notice soon
enough if there's a problem.

> http://bugs.python.org/review/6397/diff/3606/11277#newcode794
> Modules/selectmodule.c:794: if (devpoll_flush(self))
> On 2011/11/08 15:20:39, AntoinePitrou wrote:
> > Better to call write() *after* parsing the arguments, IMO.
> > 
> 
> It is safe to call the write anytime. The buffering is only for performance
> reasons. In this case, even if the arguments are wrong, the kernel data will
be
> updated, and that is OK.

I still think argument errors (TypeError/ValueError) should be reported in
priority to write() errors. It shouldn't make the code more complicated anyway
:-))

> Please, check the new changeset, with all your feedback.

I'll take a look later.

Thanks

Antoine.

[AntoinePitrou]

http://bugs.python.org/review/6397/diff/3615/11301
File Lib/test/test_devpoll.py (right):

http://bugs.python.org/review/6397/diff/3615/11301#newcode77
Lib/test/test_devpoll.py:77: pollster.poll(-2)
Isn't this supposed to block? Or does it return immediately because stdout is
ready for writing? (if the latter, it would be nice to use sys.stdout.fileno()
instead)

http://bugs.python.org/review/6397/diff/3615/11302
File Misc/NEWS (right):

http://bugs.python.org/review/6397/diff/3615/11302#newcode1467
Misc/NEWS:1467: - Issue #6397: Support "/dev/poll" polling objects in select
module,
Please put it at the top of the "library" section. Also, you don't have to
submit it in the patch :)

http://bugs.python.org/review/6397/diff/3615/11303
File Modules/selectmodule.c (right):

http://bugs.python.org/review/6397/diff/3615/11303#newcode856
Modules/selectmodule.c:856: value = PyTuple_Pack(2, num1, num2);
You must still decref num1 and num2 after this (Py_BuildValue is higher level).

[jcea]

On 2011/11/08 23:13:22, AntoinePitrou wrote:
> Hi,
> 
> On 2011/11/08 18:53:33, jcea wrote:
> > On 2011/11/08 15:20:39, AntoinePitrou wrote:
> > > Are you sure this is an error? Shouldn't you try to write() until
everything
> > is
> > > written out?
> > > 
> > 
> > Solaris "/dev/poll" manpage does this in the example.
> > 
> > Also in examples posted in Sun website:
> > 
> > http://developers.sun.com/solaris/articles/polling_efficient_devpollperf.c
> > 
> > http://developers.sun.com/solaris/articles/using_devpoll.pdf Page 8-9
> 
> I wouldn't really trust these examples. udpdevpoll.c calls close() on an
> initialized variable!
> But, if you say it works, I think it's ok anyway. Someone will notice soon
> enough if there's a problem.

Thinking about it, the data writed are binary structs. If the write is
incomplete and not a multiple of struct size, the write can't simply retried.
Better write not to be incomplete...

I change the code so the exception raised indicates a "if you see this, please
report!". No user should see it, ever :-).

> > http://bugs.python.org/review/6397/diff/3606/11277#newcode794
> > Modules/selectmodule.c:794: if (devpoll_flush(self))
> > On 2011/11/08 15:20:39, AntoinePitrou wrote:
> > > Better to call write() *after* parsing the arguments, IMO.
> > > 
> > 
> > It is safe to call the write anytime. The buffering is only for performance
> > reasons. In this case, even if the arguments are wrong, the kernel data will
> be
> > updated, and that is OK.
> 
> I still think argument errors (TypeError/ValueError) should be reported in
> priority to write() errors. It shouldn't make the code more complicated anyway
> :-))

Well, theorically the write shouldn't fail ever, but I see your point. Changing
code...

[jcea]

Please review next changeset. Thanks for your time and your feedback!

http://bugs.python.org/review/6397/diff/3615/11301
File Lib/test/test_devpoll.py (right):

http://bugs.python.org/review/6397/diff/3615/11301#newcode77
Lib/test/test_devpoll.py:77: pollster.poll(-2)
On 2011/11/09 00:03:39, AntoinePitrou wrote:
> Isn't this supposed to block? Or does it return immediately because stdout is
> ready for writing? (if the latter, it would be nice to use sys.stdout.fileno()
> instead)

Doesn't block because sys.stdout accepts writing. Changing the "1" to
"sys.stdout" and documenting.

http://bugs.python.org/review/6397/diff/3615/11302
File Misc/NEWS (right):

http://bugs.python.org/review/6397/diff/3615/11302#newcode1467
Misc/NEWS:1467: - Issue #6397: Support "/dev/poll" polling objects in select
module,
On 2011/11/09 00:03:39, AntoinePitrou wrote:
> Please put it at the top of the "library" section. Also, you don't have to
> submit it in the patch :)
> 

Done.

http://bugs.python.org/review/6397/diff/3615/11303
File Modules/selectmodule.c (right):

http://bugs.python.org/review/6397/diff/3615/11303#newcode856
Modules/selectmodule.c:856: value = PyTuple_Pack(2, num1, num2);
On 2011/11/09 00:03:39, AntoinePitrou wrote:
> You must still decref num1 and num2 after this (Py_BuildValue is higher
level).
> 

Done.

[neologix_free.fr]

A little late...

http://bugs.python.org/review/6397/diff/3645/11417
File Modules/selectmodule.c (right):

http://bugs.python.org/review/6397/diff/3645/11417#newcode674
Modules/selectmodule.c:674: if (!self->n_fds) return 0;
Just a nit, but the usual coding style is to have return on its own line, and I
personally find (self->n_fds == 0) more clear.

http://bugs.python.org/review/6397/diff/3645/11417#newcode676
Modules/selectmodule.c:676: size = sizeof(struct pollfd)*self->n_fds;
A space around '*' would be welcome here.

http://bugs.python.org/review/6397/diff/3645/11417#newcode770
Modules/selectmodule.c:770: {
You're duplicating what's done in internal_devpoll_register(): instead of
`remove`, it could take a simple integer flag indicating what to do (i.e. add,
remove or modify).

http://bugs.python.org/review/6397/diff/3645/11417#newcode789
Modules/selectmodule.c:789: "poll( [timeout] ) -> list of (fd, event)
2-tuples\n\n\
You should probably make it clear that timeout is in milliseconds (and another
nit, but I don't think we usually place spaces around ] and ).

http://bugs.python.org/review/6397/diff/3645/11417#newcode851
Modules/selectmodule.c:851: return NULL;
Since you return, you don't need the else: this saves an indentation level (but
it's a question of style :-).

http://bugs.python.org/review/6397/diff/3645/11417#newcode854
Modules/selectmodule.c:854: num1 = PyLong_FromLong(self->fds[i].fd);
`num1` and `num2` aren't clear: something like `fd` and `event_mask` would be
better.

http://bugs.python.org/review/6397/diff/3645/11417#newcode906
Modules/selectmodule.c:906: */
I doubt getrlimit makes a blocking syscall: you could get it out of the
Py_BEGIN_ALLOW_THREADS (and the error condition could be right after the call).