Thanks, Chris. I've removed the dependency on the internal HttpServer. Here's another webrev https://googledrive.com/host/0B2CI6Ih--1t5bVVwbVlBRmpVMDg/2/index.html.
Best, Dimitar On Mon, Jan 6, 2014 at 4:18 PM, Chris Hegarty <chris.hega...@oracle.com>wrote: > The source changes look good to me too. > > I see Alan has commented on the test, and I agree. Trivially, can you also > remove the dependency on the old internal HttpServer, and use the newer > com.sun.net.httpserver API. It is easier to user, and more robust. You can > see an example here [1]. > > -Chris. > > [1] http://hg.openjdk.java.net/jdk9/dev/jdk/file/tip/test/ > sun/net/www/protocol/http/StreamingOutputStream.java. > > > > On 05/01/14 11:09, Alan Bateman wrote: > >> On 03/01/2014 11:04, Dimitar Mavrodiev wrote: >> >>> Greetings all, >>> >>> I've fixed this and created a test to cover it, is there a sponsor who >>> could push this through? Here's a link to the webrev >>> https://googledrive.com/host/0B2CI6Ih--1t5bVVwbVlBRmpVMDg/index.html. >>> >>> It's a simple fix that correctly consumes the bytes from a SOCKS reply >>> which represent an IPv6 address or a domain name. I also had to fix >>> SocksServer as it was not correctly constructing a String >>> representation of an IPv6 address from byte[]. >>> >>> I didn't find it necessary to cover the case of DOMAIN_NAME in the >>> test as name resolution happens locally and not on the SOCKS server, >>> which is perhaps worth another fix. >>> >>> Thanks for the patch and I see that your OCA has been processed. >> >> I checked section 5 of RFC 1928 and it does indeed appear that the >> DOMAINNAME (0x03) and IP V6 address (0x04) cases were not implemented >> correctly. Your patch looks right. In passing, I see that the constants >> for SOCKS are defined in an interface (which is an anti-pattern) and we >> should clean that up at some point (not necessary for this patch of >> course). >> >> On the test then I think it will need to check that IPv6 is enabled as >> part of the setup, otherwise it looks like it will fail. I realize that >> IPv6 is enabled by default everywhere these days but we regularly test >> on machine that don't have it configured. One other thing about the test >> is that it will require a GPL header. Would you have cycles to expand >> the SOCKS test infrastructure to cover the DOMAIN_NAME case? I ask about >> that case because it was the lack of test coverage that meant this >> mis-handling slipped through (although I don't think it is actually used >> and doesn't appear to have been noticed before). >> >> Otherwise, I think this is a good (and I would be happy to sponsor it). >> >> -Alan. >> >>
