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-dev@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. >>
