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

Paul Fertser
2014-02-12T17:32:15Z

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