On Tue, Sep 15, 2020 at 2:55 PM Ilya Maximets <[email protected]> wrote:
> On 9/15/20 7:17 PM, Terry Wilson wrote: > > With other socket types, trying to connect and failing will return > > an error code, but if an SSL Stream is used, then when > > check_connection_completion(sock) is called, SSL will raise an > > exception that doesn't derive from socket.error which is handled. > > > > This adds handling for SSL.error, which get_exception_errno > > will simply return errno.EPROTO for. > > > > Signed-off-by: Terry Wilson <[email protected]> > > --- > > Hi, Terry. Thanks for working on this! > Thanks for the review! > > - except socket.error as e: > > + except (socket.error, SSL.Error) if SSL else socket.error > as e: > > This doesn't look right because this breaks the stream abstraction. > I think, SSL.Error should not go outside of SSLStream class in the first > place. > If that's true, doesn't having the staticmethod Stream.open() call check_connection_completion(sock) break the abstraction by operating directly on the sock object and not the Stream object? > Looking at methods of SSLStream class, I see that it catches only few of > SSL > errors and translates them to errno, but it must catch all possible SSL > errors. > Right, it already catches the errors we need in the SSLStream.send(), it's just that check_connection_completion() is in socket_util.py which doesn't operate on streams, so calls socket.send(). Stream.open() is a staticmethod, so it's not going to be class-aware, so also kludgy to add the SSL-specific stuff there. But basically it seems like either wrap everywhere that calls check_connection_completion(), come up with some kind of wrapper for check_connection_completion in stream.py classes that is at least a classmethod that , or just handle it in socket_util. I'm not sure there is an easy solution that I'd find 'pretty', but I'm willing to do whatever is least ugly to everyone. ;) See interpret_ssl_error() of the C implementation in lib/stream-ssl.c. > I think, we need to implement similar handling in python code to unify > behavior. > > What do you think? > I think most of that already exists with the SSLStream class. I remember using it for 5fe179987d to do the same thing stream-ssl.c did for closing an SSL connection. If Stream.open() wasn't called as a staticmethod/factory function, then it'd be a lot easier to make a cleaner implementation.The cleanest thing I can think of is to define a staticmethod Stream.check_connection_completion(sock) that returns the sock_util version, then override with a staticmethod in SSLStream that try's calling Stream.check_connection_completion but catches SSL.Error and returns the errno. And replace calls to the sock_util.check_connection_completion with the class-based ones. I'll send a patch shortly with that implementation to see if looks better. Thanks again for the review! Terry _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
