Created attachment 72234 [details] [review]
for .111

I'm really liking your patch.  I think this should go in the next version.

I have just two suggestions.

One is that I don't think it should be NNTP's job to create the socket.
IMO that complicates NNTP's responsibilities and also confuses the flow
between NNTP_Pool, NNTP, and socket creation.

So here's a revison that, to my mind, is a little cleaner.
- We add a new Socket::Creator::Listener class containing "on_socket_created".
- Socket::Creator now returns void and takes a new argument, a Listener pointer.
- NNTP_Pool calls create_socket(), listens for a response, /then/ calls handshake
- GIOChannelSocket::Creator uses threads + idle funcs as before.

The other point is just a platform issue.  Your create_channel() didn't compile
on Linux because of closesocket and one of the *addr=&something calls.
So the second patch's create_channel() is unchanged from 0.111.

If you'd like to supersede this with a third patch that adds your
create_channel() improvements in a way that works on Windows and Linux,
that would be great.

Charles

Created attachment 72280 [details] [review]
0.111 patch.  revision of previous patch.

Created attachment 72288 [details] [review]
rev 3 for .111

I've fixed the problems in the create_socket function (hopefully).  The closesocket issue was ficed with a define.

The big change I made was to ensure that pan tried all addresses for a server instead of just the first one the it could get a socket for.  A server could have both v4 & v6 addresses but only have NNTP on v4.  If the original code could get a v6 socket it would only try the first v6 address.

Created attachment 72289 [details] [review]
oops

The g_thread_init call got left out.

Actually I think my 0.111 code is too convoluted here.
What would you think about this kind of a loop instead?
It includes your improvements and
is easier to read than my original code.


  // try to open a socket on any ipv4 or ipv6 addresses we found
  sockfd = -1;
  for (struct addrinfo * walk(ans); walk && sockfd<0; walk=walk->ai_next)
  {
    // only use ipv4 or ipv6 addresses
    if ((walk->ai_family!=PF_INET) && (walk->ai_family!=PF_INET6))
      continue;

    // try to create a socket...
    sockfd = ::socket (walk->ai_family, walk->ai_socktype, walk->ai_protocol);
    if (sockfd < 0)
      continue;

    // and make a connection
    if (::connect (sockfd, ans->ai_addr, ans->ai_addrlen) < 0) {
      g_message ("Connect failed: %s", g_strerror(errno));
      closesocket (sockfd);
      sockfd = -1;
      continue;
    }
  }
  ::freeaddrinfo (ans);

That looks a lot better.  So you decided not to have debug output of the addresses?

Nah, it's so much cleaner without it.

Does the version in 0.112 look right to you?

Yep, this version looks good.

Created attachment 72527 [details] [review]
for .112

Ok, support for ipv6 should now be dynamic on windows.  At least this patch works on XP.  I've also moved the ensure_module_inited function calls to the main thread to avoid possible races.

Applied for 0.113.

Created attachment 73005 [details] [review]
0.112 patch

Your patch definitely has the right idea, but some of the Windows-specific
calls need to be wrapped in an #ifdef block in order to get the code to 
compile on Linux.

This new patch is a revision that compiles and works right on Linux;
I'd like you to try it out on Windows and make sure I haven't broken
anything there too. :)

Also, the ensure_inited() is now /only/ called before invoking the
worker thread, just in case there's a race condition there.  I missed
that possibility last time.

Since you've rolled out a Windows build for 0.113, I take it
that the latest patch worked.  :)

Thanks for your work on this ticket!