Re: Bug#748903: python-tornado: FTBFS on hurd-i386

[Samuel Thibault <sthibault@debian.org>]

Svante Signell, le Fri 23 May 2014 16:02:16 +0200, a écrit :
> On Fri, 2014-05-23 at 14:42 +0200, Samuel Thibault wrote:
> > Svante Signell, le Fri 23 May 2014 14:28:49 +0200, a écrit :
> > > On Fri, 2014-05-23 at 13:58 +0200, Samuel Thibault wrote:
> > > > Svante Signell, le Fri 23 May 2014 13:45:50 +0200, a écrit :
> > > > > Index: python-tornado-3.2.0/tornado/iostream.py
> > > > > ===================================================================
> > > > > --- python-tornado-3.2.0.orig/tornado/iostream.py
> > > > > +++ python-tornado-3.2.0/tornado/iostream.py
> > > > > @@ -748,7 +748,11 @@ class IOStream(BaseIOStream):
> > > > >          self._add_io_state(self.io_loop.WRITE)
> > > > >  
> > > > >      def _handle_connect(self):
> > > > > -        err = self.socket.getsockopt(socket.SOL_SOCKET, socket.SO_ERROR)
> > > > > +        try:
> > > > > +            err = self.socket.getsockopt(socket.SOL_SOCKET, socket.SO_ERROR)
> > > > > +        except socket.error as e:
> > > > > +            if e.args[0] == errno.ENOPROTOOPT:
> > > > > +                err = 0
> > > > >          if err != 0:
> > > > >              self.error = socket.error(err, os.strerror(err))
> > > > >              # IOLoop implementations may vary: some of them return
> > > > 
> > > > But if it's another error, err and self.error will not be set. Rather
> > > > use:
> > > > 
> > > > @@ -748,7 +748,12 @@ class IOStream(BaseIOStream):
> > > >          self._add_io_state(self.io_loop.WRITE)
> > > >  
> > > >      def _handle_connect(self):
> > > > -        err = self.socket.getsockopt(socket.SOL_SOCKET, socket.SO_ERROR)
> > > > +        try:
> > > > +            err = self.socket.getsockopt(socket.SOL_SOCKET, socket.SO_ERROR)
> > > > +        except socket.error as e:
> > > > +            err = e.args[0]
> > > > +            if err == errno.ENOPROTOOPT:
> > > > +                err = 0
> > > >          if err != 0:
> > > >              self.error = socket.error(err, os.strerror(err))
> > > >              # IOLoop implementations may vary: some of them return
> > > 
> > > I thoughts it did exactly the same before, indentation stops at err = 0,
> > > i.e. outside the exception, so other errors are not affected.
> > 
> > Other exceptions (with different e.args[0]) will still be caught by
> > except. Those need to be handled too.
> > 
> > > But I can do as you wish, just let me know. Otherwise al else
> > > statement would be needed.
> > 
> > You can go with an else, yes, but it's simpler the way I proposed.
> 
> I still don't see the difference: with 
> 
> try:
>   err = self.socket.getsockopt(socket.SOL_SOCKET, socket.SO_ERROR)
>   except socket.error as e:
>   if e.args[0] == errno.ENOPROTOOPT:
>    err = 0
> if err != 0:
> ...
> 
> the exception handling is only treating the case when
> e.args[0] ==  errno.ENOPROTOOPT and an else case would not be needed.

Take this scenario: getsockopt raises exception socket.error(errno.EIO).
What will be catched by the except socket.error, but nothing will be
done about it, nothing will be stored in err, and execution continues
with if err != 0. Worse, since nothing will be stored in err, err won't
even exist at all, and if err itself will actually error out.

> For other exceptions there is no explicit handler defined in
> _handle_connect(self) and err is unchanged, or is python different?
> 
> In C:
> if (condition)
> {do something}
> do something else
> 
> No need for an else here

This is not a strict equivalent of the python code above. A more strict
equivalent would be

if (socketerror)
{
  if (socketerror == ENOPROTOOPT)
    err = 0;
}

if (err != 0)

when socketerror is != 0 but not ENOPROTOOPT, nothing is stored in err.

Samuel