| msg72787 - (view) |
Author: Gregory P. Smith (gregory.p.smith) *  |
Date: 2008-09-08 19:36 |
| Modules/_ssl.c in the py3k branch: PySSL_SSLread(): calls parsetuple expecting "|Oi" as arguments. However the logic below to interpret and use the arguments is very convoluted. it'd be better to reorder these as " |
iO" to match the api in Lib/ssl.py. Or even better to just get rid of the "O" all together (currently used to pass in a bytearray buffer to write into instead of allocating its own). also: it returns either a bytes or a long depending on the order and type of arguments given. yuck. |
|
| msg72789 - (view) |
Author: Bill Janssen (janssen) * |
Date: 2008-09-08 21:10 |
| There was a reason to do it that way. Now if I can only remember what it was... |
|
|
| msg72793 - (view) |
Author: Gregory P. Smith (gregory.p.smith) * |
Date: 2008-09-08 21:59 |
| i only had a brief look when i was going through code looking for potentially incorrect uses of PyByteArray_*. I've got a patch that i believe cleans it up a little but its sitting on a machine i don't have remote access to at the moment. i'll attach it when i get a chance. Looking at Lib/ssl.py it appears that recv_into needs this functionality. I'd still suggest changing the order to be "|iO" to simplify the code a little. The PyLong_Check(buf) could go away. I do not see any calls to _sslobj.read() within Lib.ssl.py that only pass in a buffer without passing in a length. When no bytearray is passed in, the code internally uses a temporary bytearray object which is later freed after being copied into a bytes object. I think it would be better to just use PyBytes_FromStringAndSize(0, len) and replace the "if (!buf_passed)" conversion data copy with a _PyBytes_Resize? The latter will only realloc and copy if needed. regardless, thanks for lowering the priority. looking over the code again I believe whats there is functionally correct even if a bit odd looking. |
|
|
| msg72841 - (view) |
Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * |
Date: 2008-09-09 11:14 |
| The same function has two distinct behaviours: - If you pass a number, it return a bytes object. - If you pass a buffer, it returns a number! Different arguments and different return types: they should have different names IMO. io.py proposes read(n) and readinto(buf) for this distinction, and I fail to see a reason why PySSL_SSLread need to be different: - readinto(buf, count=-1) could be similar to the actual PySSL_SSLread. the "count" defaults to len(buf). - read(count) could be implemented in C or python (or not at all), it is equivalent to: def read(self, count=1024): buf = bytearray(count) nb = self.readinto(buf) return bytes(buf[:nb]) |
|
|
| msg72883 - (view) |
Author: Bill Janssen (janssen) * |
Date: 2008-09-09 16:51 |
| Please, guys, let's not deep-end on this. It's an admittedly eccentric but working and purely internal interface. There are actual release-blockers that need to be addressed. On Tue, Sep 9, 2008 at 4:14 AM, Amaury Forgeot d'Arc <report@bugs.python.org > wrote: > > Amaury Forgeot d'Arc <amauryfa@gmail.com> added the comment: > > The same function has two distinct behaviours: > - If you pass a number, it return a bytes object. > - If you pass a buffer, it returns a number! > Different arguments and different return types: they should have > different names IMO. > > io.py proposes read(n) and readinto(buf) for this distinction, and I > fail to see a reason why PySSL_SSLread need to be different: > > - readinto(buf, count=-1) could be similar to the actual PySSL_SSLread. > the "count" defaults to len(buf). > - read(count) could be implemented in C or python (or not at all), it is > equivalent to: > def read(self, count=1024): > buf = bytearray(count) > nb = self.readinto(buf) > return bytes(buf[:nb]) > > ---------- > nosy: +amaury.forgeotdarc > > _______________________________________ > Python tracker <report@bugs.python.org> > <http://bugs.python.org/issue3805> > _______________________________________ > |
|
|
| msg72885 - (view) |
Author: Bill Janssen (janssen) * |
Date: 2008-09-09 17:14 |
| But I should say... Greg, I do appreciate the review and the comments. I was waiting for the bytes work in 3K to settle down before looking at that code again. When I do, I'll take your comments to heart. |
|
|
| msg72886 - (view) |
Author: Gregory P. Smith (gregory.p.smith) * |
Date: 2008-09-09 17:20 |
| yep, agreed. leave this for later. my bad for opening it with high priority in the first place. low is correct. :) i was just trying to write down comments on odd code that i came across before i forgot about it. |
|
|
| msg115481 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2010-09-03 18:40 |
| Cleanup done in r84464 (py3k), r84465 (3.1). |
|
|