Can someone from the build group please give the build part of this the once over.
Thanks, -Chris. > On 31 Jan 2017, at 19:14, Zeller, Arno <arno.zel...@sap.com> wrote: > > Hi Chris, > > thanks for all the improvements. I imported your webrev and prepared another > webrev: > http://cr.openjdk.java.net/~clanger/webrevs/8170868.6/ > > I have seen the multiple DIRECTs myself during testing and I think filtering > on the java side is a very elegant solution. > Thanks for this! > > Best regards, > Arno > >> -----Original Message----- >> From: Chris Hegarty [mailto:chris.hega...@oracle.com] >> Sent: Dienstag, 31. Januar 2017 12:47 >> To: Zeller, Arno <arno.zel...@sap.com> >> Cc: net-...@openjdk.java.net >> Subject: Re: RFR:8170868: DefaultProxySelector should use system defaults >> on Windows, MacOS and Gnome >> >> Arno, >> >> Thanks for doing this, and your patience so far. One more final round of >> comments from me. Mostly minor. I’ve put them in the form of a webrev so >> you can easily view and import them, if you agree. >> >> http://cr.openjdk.java.net/~chegar/8170868_additional/ >> >> 1) A few comment rewordings and formatting suggestions. >> >> 2) I see duplicate DIRECT, and a few specific proxies, in my >> testing. Maybe my environment or PAC file is returning >> duplicate entries. I suggest that, at the java-level, we filter >> out duplicates while preserving ordering. ( see above webrev ) >> >> 3) We do not use asserts in core libraries. So I replaced the usage >> with an early return NULL ( no proxy ). At a later stage we could >> throw an exception or something, but probably not all that >> interesting. >> >> I have run this change ( along with my additional suggestions ) through our >> internal build and test system. No surprises. >> >> -Chris. >> >>> On 31 Jan 2017, at 09:14, Zeller, Arno <arno.zel...@sap.com> wrote: >>> >>> Hi Chris, >>> >>> thanks a lot - I did not see this case in my tests. I added your change to >>> my >> new webrev: >>> http://cr.openjdk.java.net/~clanger/webrevs/8170868.5/ >>> >>> >>> Best regards, >>> Arno >>> >>>> -----Original Message----- >>>> From: Chris Hegarty [mailto:chris.hega...@oracle.com] >>>> Sent: Montag, 30. Januar 2017 17:14 >>>> To: Zeller, Arno <arno.zel...@sap.com> >>>> Cc: net-...@openjdk.java.net >>>> Subject: Re: RFR:8170868: DefaultProxySelector should use system >>>> defaults on Windows, MacOS and Gnome >>>> >>>> Arno, >>>> >>>> I found an issue on Windows when proxies are specified per-protocol, i.e. >>>> they are returned with their optional scheme set. I believe that the >>>> scheme should be checked, at least without this change FTP proxies >>>> were being returned for HTTP URL's, on my machine. >>>> >>>> $ hg -R jdk diff >>>> diff -r 07e07fecf383 >>>> src/java.base/windows/native/libnet/DefaultProxySelector.c >>>> --- a/src/java.base/windows/native/libnet/DefaultProxySelector.c >>>> Mon Jan 30 14:09:14 2017 +0000 >>>> +++ b/src/java.base/windows/native/libnet/DefaultProxySelector.c >>>> Mon Jan 30 16:09:23 2017 +0000 >>>> @@ -99,6 +99,7 @@ >>>> * Returns the size of the array as int. >>>> */ >>>> static int createProxyList(LPWSTR win_proxy, const WCHAR *pproto, >>>> list_item **head) { >>>> + static const wchar_t separators[] = L"\t\r\n ;"; >>>> list_item *current = NULL; >>>> int nr_elems = 0; >>>> wchar_t *context = NULL; >>>> @@ -109,13 +110,26 @@ >>>> * The proxy server list contains one or more of the following >>>> strings separated by semicolons or whitespace. >>>> * ([<scheme>=][<scheme>"://"]<server>[":"<port>]) >>>> */ >>>> - current_proxy = wcstok_s(win_proxy, L"; ", &context); >>>> - while (current_proxy != NULL) { >>>> + current_proxy = wcstok_s(win_proxy, separators, &context); >>>> + while (current_proxy != NULL) { >>>> LPWSTR pport; >>>> LPWSTR phost; >>>> int portVal = 0; >>>> wchar_t *next_proxy = NULL; >>>> list_item *proxy = NULL; >>>> + wchar_t* pos = NULL; >>>> + >>>> + /* Filter based on the scheme, if there is one */ >>>> + pos = wcschr(current_proxy, L'='); >>>> + if (pos) { >>>> + *pos = L'\0'; >>>> + if (wcscmp(current_proxy, pproto) != 0) { >>>> + current_proxy = wcstok_s(NULL, separators, &context); >>>> + continue; >>>> + } >>>> + current_proxy = pos + 1; >>>> + } >>>> >>>> /* Let's check for a scheme and ignore it. */ >>>> if ((phost = wcsstr(current_proxy, L"://")) != NULL) { @@ >>>> -152,7 +166,7 @@ >>>> } >>>> } >>>> /* goto next proxy if available... */ >>>> - current_proxy = wcstok_s(NULL, L"; ", &context); >>>> + current_proxy = wcstok_s(NULL, separators, &context); >>>> } >>>> return nr_elems; >>>> } >>>> @@ -268,7 +282,6 @@ >>>> if (win_proxy != NULL) { >>>> wchar_t *context = NULL; >>>> int defport = 0; >>>> - WCHAR pproto[MAX_STR_LEN]; >>>> int nr_elems = 0; >>>> >>>> /** >>>> @@ -290,10 +303,7 @@ >>>> goto noproxy; >>>> } >>>> >>>> - /* Prepare protocol string to compare with. */ >>>> - _snwprintf(pproto, sizeof(pproto), L"%s=", lpProto); >>>> - >>>> - nr_elems = createProxyList(win_proxy, pproto, &head); >>>> + nr_elems = createProxyList(win_proxy, lpProto, &head); >>>> if (nr_elems != 0 && head != NULL) { >>>> int index = 0; >>>> proxy_array = (*env)->NewObjectArray(env, nr_elems, >>>> proxy_class, NULL); >>>> >>>> -Chris. >>>> >>>> P.S. I will take a look at the latest webrev. >>>> >
