Re: [PATCH v3] Add IPv6 support for outgoing remote TCP connections

[Paul Fertser <fercerpav at gmail dot com>]

Hi Jan,

Thank you for the review!

On Wed, Feb 12, 2014 at 05:53:18PM +0100, Jan Kratochvil wrote:
> > +AC_SEARCH_LIBS(getaddrinfo, [socket network net], [], [], [-lnsl])
> 
> Where is it stated in gnulib?  I could not find it.

http://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=blob;f=m4/getaddrinfo.m4;h=2e66584865e9b45c201219071a5abc454ef43937;hb=HEAD#l21

-lnsl is needed per Oracle getaddrinfo man page.

> >  #ifdef USE_WIN32API
> >  #include <winsock2.h>
> > +#include <ws2tcpip.h>
> 
> Put there AC_CHECK_HEADERS for it unless someone confirms all MS-Windows hosts
> really have that include file.  I have no idea.

Hm, ok, I'll try to come up with something justifiable here.

> > +  tmp = getaddrinfo (hostname, port_str, &hints, &result);
> > +  if (tmp)
> >      {
> >        fprintf_unfiltered (gdb_stderr, "%s: unknown host\n", hostname);
> 
> It should use gai_strerror().  I understand you wrote
> # In this patch I was trying to change current behaviour as little as
> # possible.
> It can be a separate patch 2/2 or even in this one.

Ok.

> > +  for (rp = result; ; rp = rp->ai_next ? rp->ai_next : result)
> > +    {
> > +      scb->fd = gdb_socket_cloexec (rp->ai_family, rp->ai_socktype,
> > +                                    rp->ai_protocol);
> >  
> > -  if (scb->fd == -1)
> > -    return -1;
> > +      if (scb->fd == -1)
> > +        continue;
> 
> Neither 'return -1' nor 'continue' are right here.  It should not lock-up if
> none of the sockets can be created, it should return so that the error gets
> reported.

Indeed, thank you for spotting this! So can it be fprintf_unfiltered +
return -1 here?

I'm still puzzled about native windows behaviour; at least when run
with wine this code I proposed performs differently compared to
GNU/Linux, but I think it is the case with the unmodified code too.

-- 
Be free, use free (http://www.gnu.org/philosophy/free-sw.html) software!
mailto:fercerpav@gmail.com