New submission from STINNER Victor: Hi,
I just saw that the changeset a79003f25a41 was merged and it adds support for memory BIO to the Python ssl module, great! It's a nice addition, especially because it becomes possible to implement SSL for IOCP event loop in asyncio (#22560). I read the changeset and I have a few comments. Sorry, I didn't have time before to review the patch. I prefer to open a new issue. I'm interested to work on patches, but I would prefer a first feedback before working on a patch. +.. method:: SSLSocket.read(len=0, buffer=None) Hum, I fear that some people will call read() with two parameters by mistake. I would prefer a keyword-only parameter: put a "$" in the call to PyArg_ParseTuple() in PySSL_SSLread(). The "read()" method is quite generic, many Python functions expect a "file-like" object with a read() method which only take one indexed parameter. An alternative would be to implemented readinto() and drop the buffer parameter from read(). Same remark for SSLObject.read() and _ssl._SSLSocket.read(). +.. attribute:: SSLSocket.server_hostname + + A ``bytes`` instance (...) + + .. versionadded:: 3.5 Oh, I would prefer to have a Unicode string here, but I see that the attribute is *not* new, it's already present in Python 3.4. The versionadded field is wrong. It looks like the attribute was introduced in Python 3.2: https://docs.python.org/dev/whatsnew/3.2.html#ssl Maybe the change is that the attribute now also exists in the _ssl module? But the _ssl module is not documented. +.. method:: MemoryBIO.write_eof() + + Write an EOF marker to the memory BIO. After this method has been called, it + is illegal to call :meth:`~MemoryBIO.write`. What does it mean, "illegal"? An exception is raised? Maybe it would be more explicit to say that an exception is raised. +- All IO on an :class:`SSLObject` is non-blocking. This means that for example + :meth:`~SSLSocket.read` will raise an :exc:`SSLWantReadError` if it needs + more data than the incoming BIO has available. It would be nice to repeat this information in the documentation of the SSLObject.read() method. +.. method:: SSLContext.wrap_bio(incoming, outgoing, server_side=False, \ Why not puting the documentation of this method where the SSLContext is documented? +Some notes related to the use of :class:`SSLObject`: I suggest to move these notes just after the documentation of the SSLObject class. +The following methods are available from :class:`SSLSocket`: methods and *attributes*. +class SSLObject: + """This class implements an interface on top of a low-level SSL object as + implemented by OpenSSL. This object captures the state of an SSL connection + but does not provide any network IO itself. IO needs to be performed + through separate "BIO" objects which are OpenSSL's IO abstraction layer. + + This class does not have a public constructor. Instances are returned by + ``SSLContext.wrap_bio``. This class is typically used by framework authors + that want to implement asynchronous IO for SSL through memory buffers. + + When compared to ``SSLSocket``, this object lacks the following features: + + * Any form of network IO incluging methods such as ``recv`` and ``send``. + * The ``do_handshake_on_connect`` and ``suppress_ragged_eofs`` machinery. + """ This documentation is useful. It should be copied in the documentation of the SSLObject class. +.. class:: MemoryBIO + + A memory buffer that can be used to pass data between Python and an SSL + protocol instance. + +.. attribute:: MemoryBIO.pending + + Return the number of bytes currently in the memory buffer. I prefer when class methods and attributes are part of the "class" block in the documentation, the documentation is rendered differently and it's more readable IMO. Compare: https://docs.python.org/dev/library/ssl.html#ssl.MemoryBIO with: https://docs.python.org/dev/library/io.html#io.IOBase But the choice how to document methods and attributes was not made in the memory BIO changeset, it is older. I suggest to "upgrade" to style to use the same style than the io module (put everything in the ".. class:" block). What do you think? ---------- messages: 228653 nosy: haypo priority: normal severity: normal status: open title: ssl: post-commit review of the new memory BIO API versions: Python 3.5 _______________________________________ Python tracker <rep...@bugs.python.org> <http://bugs.python.org/issue22564> _______________________________________ _______________________________________________ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
