| Message ID | 20200915212906.679182-1-twilson@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,v2] Don't raise an Exception on failure to connect via SSL | expand |
> 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.SysCallError which has the same > arguments as socket.error (errno, string). A future enhancement > could be to go through SSLStream class and implement error > checking for all of the possible exceptions similar to how > lib/stream-ssl.c's interpret_ssl_error() works across the various > methods that are implemented. > > Signed-off-by: Terry Wilson <twilson at redhat.com <https://mail.openvswitch.org/mailman/listinfo/ovs-dev>> > --- > python/ovs/stream.py | 15 +++++++++++++-- > 1 file changed, 13 insertions(+), 2 deletions(-) > > diff --git a/python/ovs/stream.py b/python/ovs/stream.py > index e9bb0c854..f5a520862 100644 > --- a/python/ovs/stream.py > +++ b/python/ovs/stream.py > @@ -132,6 +132,10 @@ class Stream(object): > IPTOS_PREC_INTERNETCONTROL = 0xc0 > DSCP_DEFAULT = IPTOS_PREC_INTERNETCONTROL >> 2 > > + @staticmethod > + def check_connection_completion(sock): > + return ovs.socket_util.check_connection_completion(sock) > + Only thing I might question is making this a static method instead of, say, a class method. Especially considering its usage in "open()" below, and the fact that we're overriding it in the SSLStream subclass, it seems like that would be clearer to me. But not a huge deal to me either way, so I'll defer to others' judgement. > @staticmethod > def open(name, dscp=DSCP_DEFAULT): > """Attempts to connect a stream to a remote peer. 'name' is a > @@ -189,7 +193,7 @@ class Stream(object): > if error: > return error, None > else: > - err = ovs.socket_util.check_connection_completion(sock) > + err = cls.check_connection_completion(sock) > if err == errno.EAGAIN or err == errno.EINPROGRESS: > status = errno.EAGAIN > err = 0 > @@ -261,7 +265,7 @@ class Stream(object): > > def __scs_connecting(self): > if self.socket is not None: > - retval = ovs.socket_util.check_connection_completion(self.socket) > + retval = self.check_connection_completion(self.socket) > assert retval != errno.EINPROGRESS > elif sys.platform == 'win32': > if self.retry_connect: > @@ -761,6 +765,13 @@ Stream.register_method("tcp", TCPStream) > > > class SSLStream(Stream): > + @staticmethod > + def check_connection_completion(sock): > + try: > + return Stream.check_connection_completion(sock) > + except SSL.SysCallError as e: > + return ovs.socket_util.get_exception_errno(e) > + > @staticmethod > def needs_probes(): > return True >
On Mon, Oct 12, 2020 at 2:51 PM Thomas Neuman <thomas.neuman@nutanix.com> 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.SysCallError which has the same > > arguments as socket.error (errno, string). A future enhancement > > could be to go through SSLStream class and implement error > > checking for all of the possible exceptions similar to how > > lib/stream-ssl.c's interpret_ssl_error() works across the various > > methods that are implemented. > > > > Signed-off-by: Terry Wilson <twilson at redhat.com < > https://mail.openvswitch.org/mailman/listinfo/ovs-dev>> > > --- > > python/ovs/stream.py | 15 +++++++++++++-- > > 1 file changed, 13 insertions(+), 2 deletions(-) > > > > diff --git a/python/ovs/stream.py b/python/ovs/stream.py > > index e9bb0c854..f5a520862 100644 > > --- a/python/ovs/stream.py > > +++ b/python/ovs/stream.py > > @@ -132,6 +132,10 @@ class Stream(object): > > IPTOS_PREC_INTERNETCONTROL = 0xc0 > > DSCP_DEFAULT = IPTOS_PREC_INTERNETCONTROL >> 2 > > > > + @staticmethod > > + def check_connection_completion(sock): > > + return ovs.socket_util.check_connection_completion(sock) > > + > > Only thing I might question is making this a static method instead of, > say, a class > method. Especially considering its usage in "open()" below, and the fact > that we're > overriding it in the SSLStream subclass, it seems like that would be > clearer to me. > But not a huge deal to me either way, so I'll defer to others' judgement. > I get why it seems a little weird, but the reason I chose @staticmethod is because @classmethod would imply that we are going to actually reference the class that is passed in the first argument, and we won't. The only thing this method depends on is the socket, so it seemed a staticmethod that could be overridden in a subclass with a special need was the way to go. > > @staticmethod > > def open(name, dscp=DSCP_DEFAULT): > > """Attempts to connect a stream to a remote peer. 'name' is a > > @@ -189,7 +193,7 @@ class Stream(object): > > if error: > > return error, None > > else: > > - err = ovs.socket_util.check_connection_completion(sock) > > + err = cls.check_connection_completion(sock) > > if err == errno.EAGAIN or err == errno.EINPROGRESS: > > status = errno.EAGAIN > > err = 0 > > @@ -261,7 +265,7 @@ class Stream(object): > > > > def __scs_connecting(self): > > if self.socket is not None: > > - retval = > ovs.socket_util.check_connection_completion(self.socket) > > + retval = self.check_connection_completion(self.socket) > > assert retval != errno.EINPROGRESS > > elif sys.platform == 'win32': > > if self.retry_connect: > > @@ -761,6 +765,13 @@ Stream.register_method("tcp", TCPStream) > > > > > > class SSLStream(Stream): > > + @staticmethod > > + def check_connection_completion(sock): > > + try: > > + return Stream.check_connection_completion(sock) > > + except SSL.SysCallError as e: > > + return ovs.socket_util.get_exception_errno(e) > > + > > @staticmethod > > def needs_probes(): > > return True > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
Yup, fair enough. LGTM Acked-by: Thomas Neuman <thomas.neuman@nutanix.com> > > I get why it seems a little weird, but the reason I chose @staticmethod is > because @classmethod would imply that we are going to actually reference > the class that is passed in the first argument, and we won't. The only > thing this method depends on is the socket, so it seemed a staticmethod > that could be overridden in a subclass with a special need was the way to > go. >
Just bumping to make sure this doesn't fall through the cracks. 11 days since the last ack. Let me know if there's anything else that needs changing. Thanks! On Fri, Oct 16, 2020 at 1:37 AM Thomas Neuman <thomas.neuman@nutanix.com> wrote: > Yup, fair enough. LGTM > > Acked-by: Thomas Neuman <thomas.neuman@nutanix.com> > > > > > I get why it seems a little weird, but the reason I chose @staticmethod > is > > because @classmethod would imply that we are going to actually reference > > the class that is passed in the first argument, and we won't. The only > > thing this method depends on is the socket, so it seemed a staticmethod > > that could be overridden in a subclass with a special need was the way to > > go. > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
Hi Terry Acked-by: Mark Michelson <mmichels@redhat.com> On 10/27/20 12:16 PM, Terry Wilson wrote: > Just bumping to make sure this doesn't fall through the cracks. 11 days > since the last ack. Let me know if there's anything else that needs > changing. Thanks! > > On Fri, Oct 16, 2020 at 1:37 AM Thomas Neuman <thomas.neuman@nutanix.com> > wrote: > >> Yup, fair enough. LGTM >> >> Acked-by: Thomas Neuman <thomas.neuman@nutanix.com> >> >>> >>> I get why it seems a little weird, but the reason I chose @staticmethod >> is >>> because @classmethod would imply that we are going to actually reference >>> the class that is passed in the first argument, and we won't. The only >>> thing this method depends on is the socket, so it seemed a staticmethod >>> that could be overridden in a subclass with a special need was the way to >>> go. >>> >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >> > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 10/28/20 3:57 PM, Mark Michelson wrote: > Hi Terry > > Acked-by: Mark Michelson <mmichels@redhat.com> > > On 10/27/20 12:16 PM, Terry Wilson wrote: >> Just bumping to make sure this doesn't fall through the cracks. 11 days >> since the last ack. Let me know if there's anything else that needs >> changing. Thanks! >> >> On Fri, Oct 16, 2020 at 1:37 AM Thomas Neuman <thomas.neuman@nutanix.com> >> wrote: >> >>> Yup, fair enough. LGTM >>> >>> Acked-by: Thomas Neuman <thomas.neuman@nutanix.com> Thanks! Applied to master and backported down to 2.10. Best regards, Ilya Maximets.
diff --git a/python/ovs/stream.py b/python/ovs/stream.py index e9bb0c854..f5a520862 100644 --- a/python/ovs/stream.py +++ b/python/ovs/stream.py @@ -132,6 +132,10 @@ class Stream(object): IPTOS_PREC_INTERNETCONTROL = 0xc0 DSCP_DEFAULT = IPTOS_PREC_INTERNETCONTROL >> 2 + @staticmethod + def check_connection_completion(sock): + return ovs.socket_util.check_connection_completion(sock) + @staticmethod def open(name, dscp=DSCP_DEFAULT): """Attempts to connect a stream to a remote peer. 'name' is a @@ -189,7 +193,7 @@ class Stream(object): if error: return error, None else: - err = ovs.socket_util.check_connection_completion(sock) + err = cls.check_connection_completion(sock) if err == errno.EAGAIN or err == errno.EINPROGRESS: status = errno.EAGAIN err = 0 @@ -261,7 +265,7 @@ class Stream(object): def __scs_connecting(self): if self.socket is not None: - retval = ovs.socket_util.check_connection_completion(self.socket) + retval = self.check_connection_completion(self.socket) assert retval != errno.EINPROGRESS elif sys.platform == 'win32': if self.retry_connect: @@ -761,6 +765,13 @@ Stream.register_method("tcp", TCPStream) class SSLStream(Stream): + @staticmethod + def check_connection_completion(sock): + try: + return Stream.check_connection_completion(sock) + except SSL.SysCallError as e: + return ovs.socket_util.get_exception_errno(e) + @staticmethod def needs_probes(): return True
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.SysCallError which has the same arguments as socket.error (errno, string). A future enhancement could be to go through SSLStream class and implement error checking for all of the possible exceptions similar to how lib/stream-ssl.c's interpret_ssl_error() works across the various methods that are implemented. Signed-off-by: Terry Wilson <twilson@redhat.com> --- python/ovs/stream.py | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)