Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(209)

#27744: Add AF_ALG (Linux Kernel crypto) to socket module

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 2 months ago by lists
Modified:
1 year, 1 month ago
Reviewers:
victor.stinner, vadmium+py
CC:
haypo, christian.heimes, devnull_psf.upfronthosting.co.za, Martin Panter, Lukasa, xiang.zhang, kalvdans_gmail.com
Visibility:
Public.

Patch Set 1 #

Total comments: 13

Patch Set 2 #

Patch Set 3 #

Total comments: 16

Patch Set 4 #

Total comments: 27

Patch Set 5 #

Total comments: 7
Unified diffs Side-by-side diffs Delta from patch set Stats Patch
Doc/library/socket.rst View 1 2 3 4 5 chunks +49 lines, -7 lines 2 comments Download
Lib/test/test_socket.py View 1 2 3 4 3 chunks +165 lines, -0 lines 3 comments Download
Modules/socketmodule.c View 1 2 3 4 21 chunks +378 lines, -53 lines 2 comments Download
configure View 1 2 3 4 6 chunks +36 lines, -13 lines 0 comments Download
configure.ac View 1 2 3 4 1 chunk +13 lines, -0 lines 0 comments Download

Messages

Total messages: 10
haypo
First review. https://bugs.python.org/review/27744/diff/18144/Doc/library/socket.rst File Doc/library/socket.rst (right): https://bugs.python.org/review/27744/diff/18144/Doc/library/socket.rst#newcode887 Doc/library/socket.rst:887: Set mode, IV, AEAD assoc length and ...
1 year, 2 months ago #1
haypo
Hum, would you mind to split your patch to extract the change on setsockopt()? At ...
1 year, 1 month ago #2
haypo
https://bugs.python.org/review/27744/diff/18243/Doc/library/socket.rst File Doc/library/socket.rst (left): https://bugs.python.org/review/27744/diff/18243/Doc/library/socket.rst#oldcode334 Doc/library/socket.rst:334: i like two empty lines for readability, please keep ...
1 year, 1 month ago #3
christian.heimes
Thanks Victor. I could move the setsockopt() change into a different patch but then I ...
1 year, 1 month ago #4
haypo
Another round of comments. https://bugs.python.org/review/27744/diff/18262/Doc/library/socket.rst File Doc/library/socket.rst (left): https://bugs.python.org/review/27744/diff/18262/Doc/library/socket.rst#oldcode334 Doc/library/socket.rst:334: Would you mind to keep ...
1 year, 1 month ago #5
christian.heimes
I have pushed the fixes to https://github.com/tiran/cpython/commits/feature/af_alg https://bugs.python.org/review/27744/diff/18262/Modules/socketmodule.c File Modules/socketmodule.c (right): https://bugs.python.org/review/27744/diff/18262/Modules/socketmodule.c#newcode296 Modules/socketmodule.c:296: #endif On ...
1 year, 1 month ago #6
haypo
https://bugs.python.org/review/27744/diff/18262/Modules/socketmodule.c File Modules/socketmodule.c (right): https://bugs.python.org/review/27744/diff/18262/Modules/socketmodule.c#newcode2003 Modules/socketmodule.c:2003: if (strlen(type) > sizeof(sa->salg_type)) { Can you please add ...
1 year, 1 month ago #7
haypo
http://bugs.python.org/review/27744/diff/18285/Modules/socketmodule.c File Modules/socketmodule.c (right): http://bugs.python.org/review/27744/diff/18285/Modules/socketmodule.c#newcode2547 Modules/socketmodule.c:2547: unsigned int optlen; Oh, thanks for fixing optlen type ...
1 year, 1 month ago #8
Martin Panter
https://bugs.python.org/review/27744/diff/18285/Doc/library/socket.rst File Doc/library/socket.rst (right): https://bugs.python.org/review/27744/diff/18285/Doc/library/socket.rst#newcode1388 Doc/library/socket.rst:1388: None or or a :term:`bytes-like object` representing a buffer. ...
1 year, 1 month ago #9
christian.heimes
1 year, 1 month ago #10
https://bugs.python.org/review/27744/diff/18262/Modules/socketmodule.c
File Modules/socketmodule.c (right):

https://bugs.python.org/review/27744/diff/18262/Modules/socketmodule.c#newcod...
Modules/socketmodule.c:2003: if (strlen(type) > sizeof(sa->salg_type)) {
On 2016/08/30 17:49:09, haypo wrote:
> Can you please add a comment just saying that? It would avoid that someone
else
> asks the same question one year later.

That makes sense. I've added a comment.

https://bugs.python.org/review/27744/diff/18262/Modules/socketmodule.c#newcod...
Modules/socketmodule.c:2560: &level, &optname, Py_TYPE(Py_None), &none,
&socklen)) {
On 2016/08/30 17:49:09, haypo wrote:
> > socklen_t is an unsigned int. Let's keep it simple and define optlen as
> unsigned
> > int and cast it to socklen_t in setsockopt call.
> 
> Please add at least the following assertion before the PyArg_ParseTuple()
call:
> 
> assert(sizeof(socklen) == sizeof(unsigned int));

I have added the assertion assert(sizeof(socklen_t) >= sizeof(unsigned int)).
There is no harm having socklen_t larger than uint32_t.

https://bugs.python.org/review/27744/diff/18285/Doc/library/socket.rst
File Doc/library/socket.rst (right):

https://bugs.python.org/review/27744/diff/18285/Doc/library/socket.rst#newcod...
Doc/library/socket.rst:1388: None or or a :term:`bytes-like object` representing
a buffer. In the later
On 2016/09/04 16:01:03, vadmium wrote:
> “or” is doubled

thanks, fixed

https://bugs.python.org/review/27744/diff/18285/Lib/test/test_socket.py
File Lib/test/test_socket.py (right):

https://bugs.python.org/review/27744/diff/18285/Lib/test/test_socket.py#newco...
Lib/test/test_socket.py:5343: self.assertEqual(hexlify(op.recv(512)), expected)
On 2016/09/04 16:01:03, vadmium wrote:
> Python 3.5 has a bytes.hex() method. Perhaps you can change “expected” to a
text
> string and write
> 
> self.assertEqual(op.recv(512).hex(), expected)

Nice, I still have to write polyglot code and wasn't able to use the new bytes
methods. I have removed import binascii from my patch.
Sign in to reply to this message.

RSS Feeds Recent Issues | This issue
This is Rietveld 894c83f36cb7