Bug #100499 for POE: [PATCH] Fix for swapped IPv6 addr / port + get rid of needless blocking host resolve

Fri Nov 21 13:08:31 2014 madis555 [...] hot.ee - Ticket created

Subject:	[PATCH] Fix for swapped IPv6 addr / port + get rid of needless blocking host resolve
Date:	Fri, 21 Nov 2014 20:08:06 +0200
To:	bug-POE [...] rt.cpan.org
From:	"Sulev-Madis Silber (ketas)" <madis555 [...] hot.ee>

Hello. I have fixes for two issues. If you use IPv6 (which you *SHOULD* in 2014!), POE::Wheel::SocketFactory swaps peer addr and port (in _define_accept_state). In another place (_define_connect_state), it uses getnameinfo to return hostname, which is completely unnecessary (addr is needed and it blocks too). Also I've updated POD to suggest using inet_ntop in Socket to get human-readable addresses for IPv4 and IPv6. Attached patch file is also available at http://ketas.si.pri.ee/POE-Wheel-SocketFactory-unpack_sockaddr_in6.1416593081.diff (2.1K) Thanks.

Message body is not shown because sender requested not to inline it.

Mon Nov 24 03:10:40 2014 RCAPUTO [...] cpan.org - Correspondence added

On Fri Nov 21 13:08:31 2014, madis555@hot.ee wrote: Show quoted text

> Hello. > > I have fixes for two issues.

I'm not sure why getnameinfo() is in there, specifically. There seems to be a few changes that add/remove it over time. I assume while it's not very performant, it exists in the code to cover some not entirely obvious condition or portability issue. While I can certainly revert it again, it's probably best to have a clear reason why. Otherwise it will just get changed back when the next person runs into a problem... and so on. If you could send this as two separate patches, one to fix each feature, I can apply the uncontroversial one without waiting for getnameinfo() to be [takes off sunglasses] resolved.

Mon Nov 24 03:10:41 2014 The RT System itself - Status changed from 'new' to 'open'

Mon Nov 24 19:13:27 2014 madis555 [...] hot.ee - Correspondence added

Subject:	Re: [rt.cpan.org #100499] [PATCH] Fix for swapped IPv6 addr / port + get rid of needless blocking host resolve
Date:	Tue, 25 Nov 2014 02:13:11 +0200
To:	bug-POE [...] rt.cpan.org
From:	"Sulev-Madis Silber (ketas)" <madis555 [...] hot.ee>

On 2014-11-24 10:10, Rocco Caputo via RT wrote: Show quoted text

> <URL: https://rt.cpan.org/Ticket/Display.html?id=100499 > > > I'm not sure why getnameinfo() is in there, specifically. There > seems to be a few changes that add/remove it over time. I assume > while it's not very performant, it exists in the code to cover some > not entirely obvious condition or portability issue. > > While I can certainly revert it again, it's probably best to have a > clear reason why. Otherwise it will just get changed back when the > next person runs into a problem... and so on.

I don't really understand that problem. Even if you use NI_NUMERICHOST | NI_NUMERICSERV with getnameinfo(), it still returns address as human-readable string, while docs mention it should always return address binary format. I think changing that address format would be quite impossible. Imagine the cursing... Though, I cursed as well... because in place of binary address of ip, there was hostname! No idea if getnameinfo() really returned that binary address somewhere sometime many decades ago or so, I have no idea why would anybody want to have wrong data there. There must be real reason. I wonder if automatic compatibility check can be created there, but it just seems like wrong random data to me. That's usually bug. Show quoted text

> If you could send this as two separate patches, one to fix each > feature, I can apply the uncontroversial one without waiting for > getnameinfo() to be [takes off sunglasses] resolved.

Ok, if you really wish, here's separate microscopic patch for fixing addr <-> port issue. And one for getnameinfo(). http://ketas.si.pri.ee/POE-Wheel-SocketFactory-inet6-swapped-port-addr.1416825472.diff http://ketas.si.pri.ee/POE-Wheel-SocketFactory-getnameinfo.1416826409.diff

Message body is not shown because sender requested not to inline it.

Tue Jun 02 14:04:12 2015 RCAPUTO [...] cpan.org - Correspondence added

Thank you for the separate patches. I've applied them, and they'll be in the next release, which I hope to upload today.

Tue Jun 02 14:04:18 2015 RCAPUTO [...] cpan.org - Status changed from 'open' to 'resolved'