[ovs-dev] [PATCH v2 1/2] ovs python: ovs.stream.open_block() returns success even if the remote is unreachable

Ben Pfaff blp at ovn.org
Tue Jul 10 20:21:26 UTC 2018


On Tue, Jul 10, 2018 at 11:28:16PM +0530, Numan Siddique wrote:
> On Mon, Jul 9, 2018 at 11:36 PM Ben Pfaff <blp at ovn.org> wrote:
> 
> > On Sun, Jul 08, 2018 at 10:05:41PM +0530, nusiddiq at redhat.com wrote:
> > > From: Numan Siddique <nusiddiq at redhat.com>
> > >
> > > Calling ovs.stream.open_block(ovs.stream.open("tcp:127.0.0.1:6641"))
> > returns
> > > success even if there is no server listening on 6641. To check if the
> > connection
> > > is established or not, Stream class makes use of
> > ovs.socket_util.check_connection_completion().
> > > This function returns zero if the select for the socket fd signals. It
> > doesn't
> > > really check if the connection was established or not.
> > >
> > > This patch fixes this issue by adding a wrapper function -
> > check_connection_completion_status()
> > > which calls sock.connect_ex() to get the status of the connection if
> > > ovs.socket_util.check_connection_completion() returns success.
> > >
> > > The test cases added fails without the fix in this patch.
> > >
> > > Signed-off-by: Numan Siddique <nusiddiq at redhat.com>
> >
> > I don't understand the problem here.  I mean, I believe when you say
> > there is a problem, but the cause doesn't really make sense to me.  The
> > code for check_connection_completion in socket_util.py looks correct to
> > me and equivalent to the C implementation in socket-util.c.  Do you have
> > an idea of why it doesn't work properly?  (Is it somehow specific to
> > Python?)
> >
> > I don't think we have an equivalent test for the C version.  Does it
> > pass, or does it need a similar change?
> >
> >
> The C implementation works fine. The function stream_open_block() returns 0
> when connected and a proper errno when it fails. I see the problem only in
> python implementation.
> 
> In C implementation, poll() is used. Suppose if I give remote as tcp:
> 127.0.0.1:6641
> and there is no server listening on 6641, poll() returns 1 and POLLERR bit
> is
> set in struct pollfd's revents (
> https://github.com/openvswitch/ovs/blob/master/lib/socket-util.c#L291)
> 
> In python implementation, select() is used. For the same remote "tcp:
> 127.0.0.1:6641",
> select() is returning 1, but 'ovs.poller.POLLERR' is not set in revents
> (
> https://github.com/openvswitch/ovs/blob/master/python/ovs/socket_util.py#L180
> ).
> So the python function check_connection_completion() returns 0, which is
> false positive.
> I also tested by adding below line in check_connection_completion() but
> still I see the
> same behavior.
> 
> ****
> diff --git a/python/ovs/socket_util.py b/python/ovs/socket_util.py
> index 91f4532ea..01dc0af2e 100644
> --- a/python/ovs/socket_util.py
> +++ b/python/ovs/socket_util.py
> @@ -174,6 +174,8 @@ def check_connection_completion(sock):
>          p.register(event, ovs.poller.POLLOUT)
>      else:
>          p.register(sock, ovs.poller.POLLOUT)
> +        p.register(sock, ovs.poller.POLLERR)
> +
>      pfds = p.poll(0)
>      if len(pfds) == 1:
>          revents = pfds[0][1]
> *****
> 
> As per the select(2) man page,
> 
> *********************
> Correspondence between select() and poll() notifications
>        Within  the  Linux  kernel  source,  we find the following
> definitions which show the correspondence between the readable, writable,
> and exceptional condition notifications of select() and the event
>        notifications provided by poll(2) (and epoll(7)):
> 
>            #define POLLIN_SET (POLLRDNORM | POLLRDBAND | POLLIN | POLLHUP |
>                                POLLERR)
>                               /* Ready for reading */
>            #define POLLOUT_SET (POLLWRBAND | POLLWRNORM | POLLOUT | POLLERR)
>                               /* Ready for writing */
>            #define POLLEX_SET (POLLPRI)
>                               /* Exceptional condition */
> ***********************
> 
> To detect POLLERR, the python implementation is looking into the exceptfds
> list returned by select. But looks like exceptfds list will be set only for
> POLLPRI.

OK.  I didn't know about that difference between select() and poll().

Should we just make check_connection_completion() use select.poll()
directly (except on Windows)?


More information about the dev mailing list