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 <[email protected]>
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 <[email protected]> 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 <[email protected]> Signed-off-by: Ben Pfaff <[email protected]> --- 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 _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
