[ovs-dev] [PATCH] python/ovs/stream: TypeError: bad operand type for unary -: 'NoneType'

[Ben Pfaff]

On Wed, Nov 21, 2012 at 03:35:48PM +0900, Isaku Yamahata wrote:
> This fixes the following exception.
> When Stream.__scs_connecting doesn't change self.state, Stream.connect()
> returns None as an implicit return value. Then, the following exception
> is raised.
> I guess this case doesn't happen in unix socket case, but does in TCP socket
> case.
> 
> >   File "ovs/jsonrpc.py", line 306, in transact_block
> >     error = self.send(request)
> >   File "ovs/jsonrpc.py", line 240, in send
> >     self.run()
> >   File "ovs/jsonrpc.py", line 200, in run
> >     retval = self.stream.send(self.output)
> >   File "ovs/stream.py", line 213, in send
> >     return -retval
> > TypeError: bad operand type for unary -: 'NoneType'
> 
> Signed-off-by: Isaku Yamahata <yamahata at valinux.co.jp>

Good catch.  Thank you for finding and reporting the problem.

> diff --git a/python/ovs/stream.py b/python/ovs/stream.py
> index c4d243d..b4f5ba6 100644
> --- a/python/ovs/stream.py
> +++ b/python/ovs/stream.py
> @@ -152,9 +152,10 @@ class Stream(object):
>          assert retval != errno.EINPROGRESS
>          if retval == 0:
>              self.state = Stream.__S_CONNECTED
> -        elif retval != errno.EAGAIN:
> -            self.state = Stream.__S_DISCONNECTED
> +        else:
>              self.error = retval
> +            if retval != errno.EAGAIN:
> +                self.state = Stream.__S_DISCONNECTED

I don't think it's a good idea to set self.error to something nonzero,
even EAGAIN, if nothing has gone wrong.  The 'error' member isn't used
much yet, but in the more or less parallel C version of the code we only
use it for reporting real errors.

>      def connect(self):
>          """Tries to complete the connection on this stream.  If the connection
> @@ -166,6 +167,11 @@ class Stream(object):
>              last_state = self.state
>              if self.state == Stream.__S_CONNECTING:
>                  self.__scs_connecting()
> +                if self.state == Stream.__S_CONNECTING:
> +                    # try again
> +                    assert self.error == errno.EAGAIN
> +                    last_state = -1
> +                assert self.state != last_state
>              elif self.state == Stream.__S_CONNECTED:
>                  return 0
>              elif self.state == Stream.__S_DISCONNECTED:

I think there's a simpler way to handle this.  Here is my version.  Will
you review it?  Thanks.

--8<--------------------------cut here-------------------------->8--

From: Ben Pfaff <blp at nicira.com>
Date: Wed, 21 Nov 2012 09:00:39 -0800
Subject: [PATCH] python/ovs/stream: Fix Stream.connect() retval for
 incomplete connection.

If the loop condition in Stream.connect() was false, which is especially
likely for TCP connections, then Stream.connect() would return None,
which violates its documented behavior.  This commit fixes the problem.

Reported-by: Isaku Yamahata <yamahata at valinux.co.jp>
Signed-off-by: Ben Pfaff <blp at nicira.com>
---
 python/ovs/stream.py |   20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/python/ovs/stream.py b/python/ovs/stream.py
index c4d243d..cbe7f42 100644
--- a/python/ovs/stream.py
+++ b/python/ovs/stream.py
@@ -161,15 +161,17 @@ class Stream(object):
         is complete, returns 0 if the connection was successful or a positive
         errno value if it failed.  If the connection is still in progress,
         returns errno.EAGAIN."""
-        last_state = -1         # Always differs from initial self.state
-        while self.state != last_state:
-            last_state = self.state
-            if self.state == Stream.__S_CONNECTING:
-                self.__scs_connecting()
-            elif self.state == Stream.__S_CONNECTED:
-                return 0
-            elif self.state == Stream.__S_DISCONNECTED:
-                return self.error
+
+        if self.state == Stream.__S_CONNECTING:
+            self.__scs_connecting()
+
+        if self.state == Stream.__S_CONNECTING:
+            return errno.EAGAIN
+        elif self.state == Stream.__S_CONNECTED:
+            return 0
+        else:
+            assert self.state == Stream.__S_DISCONNECTED
+            return self.error
 
     def recv(self, n):
         """Tries to receive up to 'n' bytes from this stream.  Returns a
-- 
1.7.10.4