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

Jan Kratochvil
2014-02-12T20:10:16Z

Hi Paul,

On Wed, 12 Feb 2014 18:32:05 +0100, Paul Fertser wrote:
> 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.

Thanks, fine with it now.

> > > + 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?

Currently I think it should do:

any_socket_opened = 0;
for (rp = result; ; rp = rp->ai_next ? rp->ai_next : result) {
scb->fd = gdb_socket_cloexec (...);
if (scb->fd == -1) {
if (rp->ai_next == NULL && (!any_socket_opened || !tcp_auto_retry))
return -1;
continue;
}
any_socket_opened = 1;
[...]
}

It is not perfect for the case
if (rp->ai_next == NULL && any_socket_opened && !tcp_auto_retry)
as errno will be returned from the failed gdb_socket_cloexec() while it would
be more appropriate to return errno from formerly failed connect(). But I do
not think it matters too much, sure it can be also fixed.

But that "return -1" part also depends on the requested gai_strerror()
implementation - currently the caller always calls perror_with_name() which
will be no longer right with gai_strerror().

I also think that it does not behave correctly now with:
set tcp auto-retry off
It will abort on first IPv6 "connection refused" despite IMO it should still
try even the IPv4 entry (although only once due to "set tcp auto-retry off").

> 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.

I do not think the Wine run is too important. I was trying to use Wine to
test the MinGW port before but I do not find the Wine run of MinGW GDB usable
for too may other reasons. After the IPv6 patch gets finalized we can ask
someone with native MS-Windows access to verify the behavior. Nice you have
found this difference.

Thanks,
Jan