|
|
|
Created:
15 years, 7 months ago by baikie Modified:
10 years, 1 month ago Reviewers:
Base URL:
http://svn.python.org/view/*checkout*/python/branches/py3k/ Visibility:
Public. |
From http://bugs.python.org/issue6560
Based on Heiko Wundram's patch from
http://bugs.python.org/issue1194378
Patch Set 1 #
Total comments: 7
Patch Set 2 : Argh, I shouldn't have used quilt. Let's try that again. #Patch Set 3 : Updated to use new BEGIN/END_SELECT_LOOP macros, fixed interrupted-timeout bug #
Total messages: 8
|
baikie
|
15 years, 7 months ago (2010年06月02日 22:37:09 UTC) #1 | ||||||||||||||||||||||||||||||||||||||||||
Here is a couple of comments that came to my mind while reading the patch. Overall the patch looks good, although I'm not an expert in the field. I also tested it works fine on recent x86-64 Linux system. http://codereview.appspot.com/1487041/diff/1/2 File Doc/library/socket.rst (right): http://codereview.appspot.com/1487041/diff/1/2#newcode450 Doc/library/socket.rst:450: .. function:: CMSG_LEN(length) The documentation for both CMSG_LEN and CMSG_SPACE is quite cryptic IMO. Would it be possible to state what they are useful for? http://codereview.appspot.com/1487041/diff/1/5 File Lib/test/test_socket.py (right): http://codereview.appspot.com/1487041/diff/1/5#newcode389 Lib/test/test_socket.py:389: class TCPTestBase(InetTestBase): Is such a forest of subclasses and mixins really needed? For example, TCP will always function in connected mode and UDP in connectionless mode. http://codereview.appspot.com/1487041/diff/1/5#newcode1297 Lib/test/test_socket.py:1297: self.assertIsNone(cm.exception.errno) This line should be indented to the left, otherwise it doesn't get executed (the previous line raises an error and leaves the "with" scope).
Thanks for wading through the patch - especially all that test code. I've been testing on 32-bit only, so it's good to know that it works on 64-bit Linux. http://codereview.appspot.com/1487041/diff/1/2 File Doc/library/socket.rst (right): http://codereview.appspot.com/1487041/diff/1/2#newcode450 Doc/library/socket.rst:450: .. function:: CMSG_LEN(length) On 2010年06月10日 19:05:20, Antoine Pitrou wrote: > The documentation for both CMSG_LEN and CMSG_SPACE is quite cryptic IMO. Would > it be possible to state what they are useful for? Yes, I suppose it doesn't make much sense if you haven't read the recvmsg() docs. I could perhaps add something about the need to restrict the amount of ancillary data received. Using CMSG_LEN() to size the ancillary buffer is actually not officially sanctioned, but CMSG_SPACE() might leave too much space and allow an unwanted extra file descriptor to be received, for example (I haven't actually seen this, but I presume it can happen on a 64-bit system). http://codereview.appspot.com/1487041/diff/1/5 File Lib/test/test_socket.py (right): http://codereview.appspot.com/1487041/diff/1/5#newcode389 Lib/test/test_socket.py:389: class TCPTestBase(InetTestBase): On 2010年06月10日 19:05:20, Antoine Pitrou wrote: > Is such a forest of subclasses and mixins really needed? For example, TCP will > always function in connected mode and UDP in connectionless mode. Perhaps it could be tidied up, but I modelled them on the existing classes such as SocketTCPTest and SocketConnectedTest with a view to replacing those (SocketTCPTest doesn't connect the socket). I just didn't want to make the patch actually delete them yet. You can actually connect a UDP socket - it will then report the errors you got from ICMP or the local network stack, etc. I don't know, maybe I should have tested sendmsg with that as well :) http://codereview.appspot.com/1487041/diff/1/5#newcode1297 Lib/test/test_socket.py:1297: self.assertIsNone(cm.exception.errno) On 2010年06月10日 19:05:20, Antoine Pitrou wrote: > This line should be indented to the left, otherwise it doesn't get executed (the > previous line raises an error and leaves the "with" scope). Well spotted, thanks.
I forgot one small thing. http://codereview.appspot.com/1487041/diff/1/5 File Lib/test/test_socket.py (right): http://codereview.appspot.com/1487041/diff/1/5#newcode1041 Lib/test/test_socket.py:1041: class TestError(Exception): You don't need this. Just use self.fail().
Here is a revised version of the patch. I've done some minor cleanup in the tests, fixed the cm.exception.errno line, and clarified the CMSG_*() docs and made them link to recvmsg(). The TestError thing in checkFlags is meant to indicate an incorrectly-written test rather than a failing one. I can't remember why I made a new exception for it though, so I've just made it raise Exception (so it won't get caught by mistake). I made a separate patch to replace SocketTCPTest, etc. with the base/mixin classes from the sendmsg patch, which I'll post back at the Tracker. With that done, I didn't see an obvious way to reduce the number of classes, except perhaps making the stream servers listen unconditionally, or merging ThreadedSocketTestMixin into the base (but then all the tests would need threading available).
Revised version of patch