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

[Jan Kratochvil <jan.kratochvil@redhat.com>]

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