Re: RFR: 7180557 - InetAddress.getLocalHost throws UnknownHostException on java7u5 on OSX
Rob,
See below -
ll. 210 have to be fixed, other comments is just an opinion.
Inet4AddressImpl.c:
ll.132 - it might be better to move initialization to a separate
function, the same way as in Inet6 - I really like this idea.
Inet6AddressImpl.c
ll. 126.
it's better to move static int initialized into initializeInetClasses()
and don't check it ll. 282.
ll. 170
it looks like rest of the code uses NI_MAXHOST also we have to check
results of JVM_GetHostName if it returns -1 it's probably better to
bailout immediately.
ll. 193 and below - wrong indent
4)
ll. 210
we can have more than one v4 address so
if (addrs4 >= 1)
have to be here.
*.
Your include ipv6 loopback in the list if interface has no ipv4
addresses, I'm not sure this logic is correct. On my opinion it's better
to never include ipv6 loopbacks.
*.
Is it better to rename:
includeLoopback => includeLoopbacks
ll. 218
It might be better to calculate arraySize under if at ll. 210 to make
code better readable
ll. 236
It might be better to split if statement to make it more readable at the
cost of duplicating iter = iter->ifa_next; line.
e.g.
while (iter != NULL) {
...
if (family != AF_INET6 and family != AF_INET) {
iter = iter->ifa_next;
continue;
}
if ((!includeV6 and family == AF_INET6)) {
iter = iter->ifa_next;
continue;
}
if (!includeLoopback and isLoopback) {
iter = iter->ifa_next;
continue;
}
if (iter->ifa_name[0] != '\0' && iter->ifa_addr) {
// Copy address to Java array
iter = iter->ifa_next;
continue; // redundant just for readability
}
}
ll.244
I'm not sure it's good to return partially complete array in case of
object allocation fail. Probably you should throw
JNU_ThrowOutOfMemoryError(env, "Object allocation failed");
-Dmitry
On 2013-09-20 18:58, Rob McKenna wrote:
> After a brief discussion with Chris, we decided to revert the position
> of the call to lookupIfLocalAddrs so as to give the local host primacy
> over DNS.
>
> Latest (and hopefully last) webrev here:
>
> http://cr.openjdk.java.net/~robm/7180557/webrev.03/
>
> -Rob
>
> On 14/09/13 00:00, Rob McKenna wrote:
>> Hi Bernd,
>>
>> I should have said in the context of this bug. What I meant was
>> removing AI_CANONNAME doesn't resolve the issue as far as Mac is
>> concerned. I.e. I still see the UnknownHostException. In this
>> particular case the hostname is not set via the hosts file.
>>
>> In the latest webrev the call to getifaddrs only occurs if getaddrinfo
>> fails.
>>
>> -Rob
>>
>> On 13/09/13 20:28, Bernd Eckenfels wrote:
>>> Am 13.09.2013, 19:32 Uhr, schrieb Rob McKenna :
>>>> W.r.t. the use of AI_CANONNAME, this doesn't actually make a
>>>> difference in the context of this fix, but is definitely something
>>>> that should be looked at. I'll put it on the todo list.
>>>
>>> I think it does make a difference: If you remove the CANON flag
>>> getaddrinfo() will not do DNS lookups when the host is configured to
>>> prefer the hosts file (which it should do on Linux and OSX). And so
>>> the platform specific lookupIfLocalhost() can be put after the
>>> getaddrinfo() (again).
>>>
>>> I actually think the bug "exists" on all platforms. If getaddrinfo()
>>> fails because neighter hosts nor DNS file finds the name this can
>>> happen on all platforms. I dont think it helps to add a fallback only
>>> on MACOSX (and there is certainly no need to prefer the fallback then).
>>>
>>> Gruss
>>> Bernd
>>
>
--
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.
Re: RFR : 8024952 : ClassCastException in PlainSocketImpl.accept() when using custom socketImpl
Sean,
The fix looks good for me but the code might be better readable if you
inverse the condition.
if (!(s instanceof PlainSocketImpl)) {
impl.accept(s)
return;
}
... rest of the code
-Dmitry
On 2013-10-01 18:54, Daniel Fuchs wrote:
> On 10/1/13 4:50 PM, Seán Coffey wrote:
>>
>> On 01/10/2013 14:51, Daniel Fuchs wrote:
>>> On 10/1/13 3:09 PM, Seán Coffey wrote:
>>>> Since I'm only creating a dummy socketImpl to test the
>>>> classcastexception, no real networking stack is in place here. I'm
>>>> catching the NPE that would be thrown from the native
>>>> Java_java_net_TwoStacksPlainSocketImpl_socketAccept function since the
>>>> underlying socket passed to it is null.
>>>>
>>>> C:\tmp>java CustomSocketImplFactory
>>>> Created Socket[addr=null,port=0,localport=0]
>>>> Exception in thread "main" java.lang.NullPointerException: socket is
>>>> null
>>>> at java.net.TwoStacksPlainSocketImpl.socketAccept(Native
>>>> Method)
>>>
>>> That's what I would have expected from your previous changeset.
>>>
>>> But you're no longer passing null - right? Now you're passing
>>> an instance of CustomSocketImpl.
>>>
>>> So where does the NPE come from? Could it be because you should
>>> be calling:
>>>ServerSocket.setSocketImplFactory(new CustomSocketImplFactory());
>>> and not:
>>>Socket.setSocketImplFactory(new CustomSocketImplFactory()); ?
>> The original bug stemmed from a custom socket Impl interacting with the
>> JDK ServerSocket Impl. If I move both Socket and ServerSocket factory
>> implementations over to use the custom Impl, the testcase doesn't get to
>> walk through the JDK serverSocket code and the ClassCastException is not
>> seen.
>>
>> The NPE seen is coming from down in the native stack when the JDK
>> ServerSocket is running through accept request (from our client socket).
>> I don't think it's an issue for this case.
>>
>> line 611 :
>> http://hg.openjdk.java.net/jdk8/tl/jdk/file/tip/src/windows/native/java/net/TwoStacksPlainSocketImpl.c
>>
>>
>>
>> regards,
>> Sean.
>
> Thanks Seán. You convinced me.
>
> -- daniel
>
>>> Or should you call both?
>>>
>>> best regards,
>>>
>>> -- daniel
>>>
>>>
>>>>
>>>> Regards,
>>>> Sean.
>>>>
>>>>>
>>>>> Or is that going to hide future bugs?
>>>>>
>>>>> best regards
>>>>>
>>>>> -- daniel (not a reviewer)
>>>>
>>>
>>
>
--
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.
Re: JDK 8 RFR 8010371: getaddrinfo can fail with EAI_SYSTEM/EAGAIN, causes UnknownHostException to be thrown
Alan, getaddrinfo is actually a shell around couple of different calls, so for getaddrinfo EAGAIN usually means that it can't rich nameserver, not the interrupted syscall. EAI_SYSTEM means that one of underlaying calls (e.g. gethostbyname) returns an error. Under Windows, getaddrinfo never returns EAI_SYSTEM, but can set WSA code other that one returned by getaddrinfo - i.e. it's recomended to use WSAGetLastError. So I'm OK with this fix. -Dmitry On 2013-10-01 22:50, Alan Bateman wrote: > On 01/10/2013 11:31, Brian Burkhalter wrote: >> Hello net-dev members, >> >> Please review this proposed fix at your convenience. >> >> Summary >> When looking up a host and an EAGAIN error is encountered, throw an >> instance of the new HostLookupException subclass of UnknownHostException. >> >> Issue >> https://bugs.openjdk.java.net/browse/JDK-8010371 >> >> Webrev >> http://cr.openjdk.java.net/~bpb/8010371 >> >> > So is getaddrinfo returning EAGAIN or is it failing with EAI_SYSTEM and > errno set to EAGAIN? I also wonder if the EAGAIN means the underlying > syscall has been interrupted, in which case the normal thing to do is to > retry. > > -Alan -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.
Re: JDK 8 RFR 8010371: getaddrinfo can fail with EAI_SYSTEM/EAGAIN, causes UnknownHostException to be thrown
Chris, I'm not sure immediate native retry make sence here because tipically EAGAIN of getaddrinfo caused by network failure, like unreachable nameserver. (see my previous letter) -Dmitry On 2013-10-02 23:53, Chris Hegarty wrote: > On 10/02/2013 08:40 PM, Brian Burkhalter wrote: >> >> So, how about this approach: >> >> 1) If the error is EAI_AGAIN / EIA_SYSTEM+EAGAIN / WSATRY_AGAIN then >> do one immediate native retry. >> 2) If the retry fails with the same error, then throw a UHE with a >> specific message or cause. > > Sounds good to me. > >> Questions: >> >> A) Would it be better to do the retry in the Java layer, perhaps with >> a very short wait? > > native, without any wait, should be sufficient. > >> B) Should the message or cause in #2 be explicitly document in the >> javadoc? > > I don't think it is necessary for this to be documented. It is more > informational. > > -Chris. -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.
Re: JDK 8 RFR 8010371: getaddrinfo can fail with EAI_SYSTEM/EAGAIN, causes UnknownHostException to be thrown
Chris, On my opinion, it's better to just return meaningful exception to customer and let them deal with network issue (as current webrev does). Typical network failure requires at least couple of milliseconds to recover so immediate retry most probably fails with the same error. >From the other side - cu has lots of possibility to defend against such failures on application level. E.g. popup a message box "please check your wiring and try again" In a future, it might be possible to add a timeout parameter to corresponding Java-level functions and keep trying on Java level until we get result or timeout, but it requires much more work and probably out of scope of this CR. -Dmitry On 2013-10-03 13:11, Chris Hegarty wrote: > On 10/02/2013 11:18 PM, Dmitry Samersoff wrote: >> Chris, >> >> I'm not sure immediate native retry make sence here because tipically >> EAGAIN of getaddrinfo caused by network failure, like unreachable >> nameserver. (see my previous letter) > > OK, while not ideal ( please don't shoot! ) what do others think of > Thread.yield() before retry. It is an attempt to allow other threads on > the system do some work before us, therefore potentially swapping out > our failed lookup thread until rescheduled. I'm just trying to avoid > baking in a hardcoded sleep/wait value ( since we don't know what that > should be ). > > The use of Thread.yield(), if acceptable, would of course most likely > make sense to push the retry logic back up into the Java level. > > -Chris. > >> >> -Dmitry >> >> On 2013-10-02 23:53, Chris Hegarty wrote: >>> On 10/02/2013 08:40 PM, Brian Burkhalter wrote: >>>> >>>> So, how about this approach: >>>> >>>> 1) If the error is EAI_AGAIN / EIA_SYSTEM+EAGAIN / WSATRY_AGAIN then >>>> do one immediate native retry. >>>> 2) If the retry fails with the same error, then throw a UHE with a >>>> specific message or cause. >>> >>> Sounds good to me. >>> >>>> Questions: >>>> >>>> A) Would it be better to do the retry in the Java layer, perhaps with >>>> a very short wait? >>> >>> native, without any wait, should be sufficient. >>> >>>> B) Should the message or cause in #2 be explicitly document in the >>>> javadoc? >>> >>> I don't think it is necessary for this to be documented. It is more >>> informational. >>> >>> -Chris. >> >> -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the source code.
Re: JDK 8 RFR 8010371: getaddrinfo can fail with EAI_SYSTEM/EAGAIN, causes UnknownHostException to be thrown
Chris, On 2013-10-03 14:10, Chris Hegarty wrote: > Thanks Dmitry, > > I think we have agreement that the cause of the UHE should contain an > Exception containing the gai_strerror string message. I can live without > adding retry logic. I'm ok with that. -Dmitry > > -Chris. > > On 10/03/2013 10:44 AM, Dmitry Samersoff wrote: >> Chris, >> >> On my opinion, it's better to just return meaningful exception to >> customer and let them deal with network issue (as current webrev does). >> >> Typical network failure requires at least couple of milliseconds to >> recover so immediate retry most probably fails with the same error. >> >> From the other side - cu has lots of possibility to defend against such >> failures on application level. E.g. popup a message box "please check >> your wiring and try again" >> >> In a future, >> >> it might be possible to add a timeout parameter to corresponding >> Java-level functions and keep trying on Java level until we get result >> or timeout, but it requires much more work and probably out of scope of >> this CR. >> >> -Dmitry >> >> >> On 2013-10-03 13:11, Chris Hegarty wrote: >>> On 10/02/2013 11:18 PM, Dmitry Samersoff wrote: >>>> Chris, >>>> >>>> I'm not sure immediate native retry make sence here because tipically >>>> EAGAIN of getaddrinfo caused by network failure, like unreachable >>>> nameserver. (see my previous letter) >>> >>> OK, while not ideal ( please don't shoot! ) what do others think of >>> Thread.yield() before retry. It is an attempt to allow other threads on >>> the system do some work before us, therefore potentially swapping out >>> our failed lookup thread until rescheduled. I'm just trying to avoid >>> baking in a hardcoded sleep/wait value ( since we don't know what that >>> should be ). >>> >>> The use of Thread.yield(), if acceptable, would of course most likely >>> make sense to push the retry logic back up into the Java level. >>> >>> -Chris. >>> >>>> >>>> -Dmitry >>>> >>>> On 2013-10-02 23:53, Chris Hegarty wrote: >>>>> On 10/02/2013 08:40 PM, Brian Burkhalter wrote: >>>>>> >>>>>> So, how about this approach: >>>>>> >>>>>> 1) If the error is EAI_AGAIN / EIA_SYSTEM+EAGAIN / WSATRY_AGAIN then >>>>>> do one immediate native retry. >>>>>> 2) If the retry fails with the same error, then throw a UHE with a >>>>>> specific message or cause. >>>>> >>>>> Sounds good to me. >>>>> >>>>>> Questions: >>>>>> >>>>>> A) Would it be better to do the retry in the Java layer, perhaps with >>>>>> a very short wait? >>>>> >>>>> native, without any wait, should be sufficient. >>>>> >>>>>> B) Should the message or cause in #2 be explicitly document in the >>>>>> javadoc? >>>>> >>>>> I don't think it is necessary for this to be documented. It is more >>>>> informational. >>>>> >>>>> -Chris. >>>> >>>> >> >> -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.
hg: jdk8/tl/jdk: 8009213: sun/management/jdp/JdpTest.sh fails with exit code 1
Changeset: 5d6dc0cba08f Author:dsamersoff Date: 2013-10-03 16:54 +0400 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/5d6dc0cba08f 8009213: sun/management/jdp/JdpTest.sh fails with exit code 1 Summary: There's no guarantee that the java process has executed far enough to be found by jps when we try to obtain it's pid. Reviewed-by: sla ! test/sun/management/jdp/JdpTest.sh
Re: RFR: 7180557 - InetAddress.getLocalHost throws UnknownHostException on java7u5 on OSX
Rob,
This version of your fix looks good for me.
Inet4AddressImpl.c:
Thumbs up.
Inet6AddressImpl.c:
Thumbs up.
173 if (JVM_GetHostName(myhostname, NI_MAXHOST)) {
Nitpicking, explicit == -1 would be better here.
> Actually, can you tell me why you'd rather not
> include ipv6 loopbacks at all?
If interface don't have ipv6 address but we have ipv6 loopback it most
likely means that ipv6 is not functioning properly.
-Dmitry
On 2013-10-04 19:03, Rob McKenna wrote:
> Hi Dmitry,
>
> Thanks a lot for the comprehensive review. W.r.t. line 210, I agree
> there is a problem with the logic, but I'd like to suggest an
> alternative solution:
>
> - If addrs4 >= 1, then we will always be including loopback addresses in
> the list. Since the idea of this extra condition is to exclude loopback
> interfaces from the list unless they're the only available addresses, I
> would suggest "(addrs4 == numV4Loopback && addrs6 == numV6Loopback)"
> instead.
>
> - On the very limited chance that a user has a machine with only an ipv6
> loopback configured (unlikely, I'd agree) it probably makes sense to
> leave it in there. Actually, can you tell me why you'd rather not
> include ipv6 loopbacks at all?
>
> New webrev at:
>
> http://cr.openjdk.java.net/~robm/7180557/webrev.04/
>
> -Rob
>
> On 21/09/13 12:20, Dmitry Samersoff wrote:
>> Rob,
>>
>> See below -
>> ll. 210 have to be fixed, other comments is just an opinion.
>>
>>
>> Inet4AddressImpl.c:
>>
>> ll.132 - it might be better to move initialization to a separate
>> function, the same way as in Inet6 - I really like this idea.
>>
>> Inet6AddressImpl.c
>>
>>
>> ll. 126.
>>
>> it's better to move static int initialized into initializeInetClasses()
>> and don't check it ll. 282.
>>
>>
>> ll. 170
>>
>> it looks like rest of the code uses NI_MAXHOST also we have to check
>> results of JVM_GetHostName if it returns -1 it's probably better to
>> bailout immediately.
>>
>>
>> ll. 193 and below - wrong indent
>>
>> 4)
>>
>> ll. 210
>>
>> we can have more than one v4 address so
>>
>> if (addrs4 >= 1)
>>
>> have to be here.
>>
>> *.
>>
>> Your include ipv6 loopback in the list if interface has no ipv4
>> addresses, I'm not sure this logic is correct. On my opinion it's better
>> to never include ipv6 loopbacks.
>>
>> *.
>>
>> Is it better to rename:
>> includeLoopback => includeLoopbacks
>>
>>
>> ll. 218
>>
>> It might be better to calculate arraySize under if at ll. 210 to make
>> code better readable
>>
>> ll. 236
>>
>> It might be better to split if statement to make it more readable at the
>> cost of duplicating iter = iter->ifa_next; line.
>>
>> e.g.
>>
>> while (iter != NULL) {
>> ...
>>
>>if (family != AF_INET6 and family != AF_INET) {
>> iter = iter->ifa_next;
>> continue;
>>}
>>
>>if ((!includeV6 and family == AF_INET6)) {
>> iter = iter->ifa_next;
>> continue;
>>}
>>
>>if (!includeLoopback and isLoopback) {
>> iter = iter->ifa_next;
>> continue;
>>}
>>
>>if (iter->ifa_name[0] != '\0' && iter->ifa_addr) {
>> // Copy address to Java array
>>
>> iter = iter->ifa_next;
>> continue; // redundant just for readability
>>}
>> }
>>
>> ll.244
>>
>> I'm not sure it's good to return partially complete array in case of
>> object allocation fail. Probably you should throw
>>
>> JNU_ThrowOutOfMemoryError(env, "Object allocation failed");
>>
>> -Dmitry
>>
>>
>> On 2013-09-20 18:58, Rob McKenna wrote:
>>> After a brief discussion with Chris, we decided to revert the position
>>> of the call to lookupIfLocalAddrs so as to give the local host primacy
>>> over DNS.
>>>
>>> Latest (and hopefully last) webrev here:
>>>
>>> http://cr.openjdk.java.net/~robm/7180557/webrev.03/
>>>
>>> -Rob
>>>
>>> On 14/09/13 00:00, Rob McKenna wrote:
>>>> Hi Bernd,
>>>>
>>>> I should have said in the context of this bug. What I meant was
>>>> removing AI_CANONNAME doesn't resolve the issue as far as Mac is
>>>> concerned. I.e. I s
Re: RFR: 7180557 - InetAddress.getLocalHost throws UnknownHostException on java7u5 on OSX
Rob,
Existing code uses if (JVM_GetHostName(myhostname, NI_MAXHOST)) so I
withdraw my last comments. Please, don't change it!!!
-Dmitry
On 2013-10-07 20:30, Rob McKenna wrote:
> Thanks Dmitry! I'll correct that nipick pre-push.
>
> -Rob
>
> On 07/10/13 16:47, Dmitry Samersoff wrote:
>> Rob,
>>
>> This version of your fix looks good for me.
>>
>>
>> Inet4AddressImpl.c:
>>
>>Thumbs up.
>>
>> Inet6AddressImpl.c:
>>
>>Thumbs up.
>>
>> 173 if (JVM_GetHostName(myhostname, NI_MAXHOST)) {
>>
>> Nitpicking, explicit == -1 would be better here.
>>
>>
>>> Actually, can you tell me why you'd rather not
>>> include ipv6 loopbacks at all?
>> If interface don't have ipv6 address but we have ipv6 loopback it most
>> likely means that ipv6 is not functioning properly.
>>
>> -Dmitry
>>
>>
>> On 2013-10-04 19:03, Rob McKenna wrote:
>>> Hi Dmitry,
>>>
>>> Thanks a lot for the comprehensive review. W.r.t. line 210, I agree
>>> there is a problem with the logic, but I'd like to suggest an
>>> alternative solution:
>>>
>>> - If addrs4 >= 1, then we will always be including loopback addresses in
>>> the list. Since the idea of this extra condition is to exclude loopback
>>> interfaces from the list unless they're the only available addresses, I
>>> would suggest "(addrs4 == numV4Loopback && addrs6 == numV6Loopback)"
>>> instead.
>>>
>>> - On the very limited chance that a user has a machine with only an ipv6
>>> loopback configured (unlikely, I'd agree) it probably makes sense to
>>> leave it in there. Actually, can you tell me why you'd rather not
>>> include ipv6 loopbacks at all?
>>>
>>> New webrev at:
>>>
>>> http://cr.openjdk.java.net/~robm/7180557/webrev.04/
>>>
>>> -Rob
>>>
>>> On 21/09/13 12:20, Dmitry Samersoff wrote:
>>>> Rob,
>>>>
>>>> See below -
>>>> ll. 210 have to be fixed, other comments is just an opinion.
>>>>
>>>>
>>>> Inet4AddressImpl.c:
>>>>
>>>> ll.132 - it might be better to move initialization to a separate
>>>> function, the same way as in Inet6 - I really like this idea.
>>>>
>>>> Inet6AddressImpl.c
>>>>
>>>>
>>>> ll. 126.
>>>>
>>>> it's better to move static int initialized into initializeInetClasses()
>>>> and don't check it ll. 282.
>>>>
>>>>
>>>> ll. 170
>>>>
>>>> it looks like rest of the code uses NI_MAXHOST also we have to check
>>>> results of JVM_GetHostName if it returns -1 it's probably better to
>>>> bailout immediately.
>>>>
>>>>
>>>> ll. 193 and below - wrong indent
>>>>
>>>> 4)
>>>>
>>>> ll. 210
>>>>
>>>> we can have more than one v4 address so
>>>>
>>>> if (addrs4 >= 1)
>>>>
>>>> have to be here.
>>>>
>>>> *.
>>>>
>>>> Your include ipv6 loopback in the list if interface has no ipv4
>>>> addresses, I'm not sure this logic is correct. On my opinion it's
>>>> better
>>>> to never include ipv6 loopbacks.
>>>>
>>>> *.
>>>>
>>>> Is it better to rename:
>>>> includeLoopback => includeLoopbacks
>>>>
>>>>
>>>> ll. 218
>>>>
>>>> It might be better to calculate arraySize under if at ll. 210 to make
>>>> code better readable
>>>>
>>>> ll. 236
>>>>
>>>> It might be better to split if statement to make it more readable at
>>>> the
>>>> cost of duplicating iter = iter->ifa_next; line.
>>>>
>>>> e.g.
>>>>
>>>> while (iter != NULL) {
>>>> ...
>>>>
>>>> if (family != AF_INET6 and family != AF_INET) {
>>>> iter = iter->ifa_next;
>>>> continue;
>>>> }
>>>>
>>>> if ((!includeV6 and family == AF_INET6)) {
>>>> iter = iter->ifa_next;
>>>> continue;
>>>> }
>>>>
>>>> if (!includeLoopback and isLoopback) {
>>>&
Re: JDK 8 RFR 8010371: getaddrinfo can fail with EAI_SYSTEM/EAGAIN, causes UnknownHostException to be thrown
Alan,
getaddrinfo could return EAI_AGAIN[1]
But recomended way is to just check return value of getaddrinfo and then
use WSA functions especially, FormatMessage[2] instead of gai_strerror.
Also we may consider rewriting this code using GetAddrInfoW sometimes in
a future to support IDN.
PS:
Brian, please notice mixing WSA and EAI at ll. 137.
[1]
http://msdn.microsoft.com/en-us/library/windows/desktop/ms738520(v=vs.85).aspx
[2]
http://msdn.microsoft.com/en-us/library/windows/desktop/ms679351(v=vs.85).aspx
On 2013-10-10 17:52, Alan Bateman wrote:
> On 09/10/2013 19:16, Brian Burkhalter wrote:
>> :
>> I have created a simple implementation for option B:
>>
>> http://cr.openjdk.java.net/~bpb/8010371/webrev.3/
>>
>> I should note that the Unix Inet6 implementation was already using the call
>> ThrowUnknownHostExceptionWithGaiError() to generate the UHE so in this case
>> the message should already have been useful. This call formats the UHE
>> message such as would:
>>
>> sprintf(error_message, "%s: %s", hostname, gai_strerror(error))
>>
>> I changed the Unix Inet4 implementation to do this as well instead of
>> calling JNU_ThrowByName().
> Thanks for persevering with this, we are on the right road now.
>
> I just looked at JDK-8010371 and the "OS" field is set to "linux". I
> don't know if this is right but as IPv6 is usually enabled by default
> these days then I have to guess that the person that submitted the bug
> has IPv6 disabled or is running with -Djava.net.preferIPv4Stack=true,
> otherwise it would be a non-issue (as you have discovered).
>
> The other thing about your observation is that
> ThrowUnknownHostExceptionWithGaiError is creating the UHE with the both
> the hostname and the error message. This resolves to the concern in one
> or two of the mails that the UHE names the exception message "host" and
> that someone might assume that the exception detail is the hostname.
>
> So thumbs up on the update to src/solaris/native/java/net/Inet4Address.c.
>
>>
>> For Windows I added a similar NET_ ThrowUnknownHostExceptionWithGaiError and
>> modified Inet{4,6} to mimic the Unix case. Note that the Windows code has
>> not yet been compiled pending comments.
>>
> Inet4AddressImpl.lookupAllHostAddr uses gethostbyname and when I look at
> the MSDN documentation then I don't see EAI_AGAIN as possible error. It
> does list WSATRY_AGAIN and I'm wondering if they have the same value and
> whether the WSA* error is only returned by WSAGetLastError?
>
> However I think we have a problem with using gai_strerror here as the
> MSDN documentation says that it is not thread safe. This means we may
> have to look for a WSA equivalent or maybe drop the Windows part of the
> fix (using a mutex of some synchronization might be possible but the
> scope would require research and of course it wouldn't work with 3rd
> party native code that is also using it).
>
> -Alan
>
>
>
>
--
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.
Re: JDK 8 RFR 8010371: getaddrinfo can fail with EAI_SYSTEM/EAGAIN, causes UnknownHostException to be thrown
Brian, You have to check error != 0 before call to WSAGetLastError() at ll. 134 Inet6AddressImpl.c Besides that - the fix looks good for me. -Dmitry On 2013-10-14 23:43, Brian Burkhalter wrote: > > On Oct 14, 2013, at 1:58 AM, Alan Bateman wrote: > >>> 2) In Inet4AddressImpl.c and Inet6AddressImpl.c replace >>> NET_ThrowUnknownHostExceptionWithGaiError with >>> NET_ThrowByNameWithLastError (see net_md_util.c). >>> >>> […] >>> >>> If the "con" of option 2 is acceptable then I think that would be the >>> best way to go, otherwise option 1. >>> >> Option #2 seems reasonable, the exception messages for similar network >> conditions are rarely the same on Windows and Unix anyway. > > Here's the patch updated for this option: > > http://cr.openjdk.java.net/~bpb/8010371/webrev.4/ > >> However I think it's important to have verified it with one or two >> errors to be confident that the errors translate as expected. > > I can do this if we are actually going with this change for JDK 8. > >> One other thing to add is that winsock_errors dates from early >> versions of Windows whether there wasn't a means to translate Windows >> Sockets errors. We should look at eliminating it (not for JDK 8 of >> course, it's too late) so that all errors are handle translated >> consistently. > > See https://bugs.openjdk.java.net/browse/JDK-4842142. > > Brian -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.
Re: JDK 8 RFR 8026806: Incomplete test of getaddrinfo() return value could lead to incorrect exception for Windows Inet 6
Brian, Looks good for me (not a reviewer). -Dmitry On 2013-10-17 20:46, Brian Burkhalter wrote: > Continuing the discussion from > http://mail.openjdk.java.net/pipermail/net-dev/2013-October/007574.html under > new issue ID: > > Issue:https://bugs.openjdk.java.net/browse/JDK-8026806 > Webrev: http://cr.openjdk.java.net/~bpb/8026806/webrev/ > > Thanks, > > Brian > > On Oct 17, 2013, at 8:06 AM, Brian Burkhalter wrote: > >> >> On Oct 17, 2013, at 5:37 AM, Alan Bateman wrote: >> >>> On 17/10/2013 00:21, Brian Burkhalter wrote: >>>> Dmitry, >>>> >>>> I think you are correct: that slipped by both me and the reviewers. I have >>>> reopened the issue and posted an amendment to the original webrev here: >>>> >>>> http://cr.openjdk.java.net/~bpb/8010371/webrev.4-amendment/ >>>> >>> I've restored the bug fields and I assume you'll create a new bug for the >>> follow-up. Sorry this was missed in the original review (probably because >>> it went through so many iterations). >>> >>> -Alan. >> >> Yes I will create a new one, thanks. >> >> Brian > -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.
hg: jdk8/tl/jdk: 8004213: JDP packet needs pid, broadcast interval and rmi server hostname fields
Changeset: 88436832cfd0 Author:dsamersoff Date: 2013-10-19 00:05 +0400 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/88436832cfd0 8004213: JDP packet needs pid, broadcast interval and rmi server hostname fields Summary: Add some extra fileds to jdp packet Reviewed-by: allwin, sla, hirt ! src/share/classes/sun/management/jdp/JdpController.java ! src/share/classes/sun/management/jdp/JdpJmxPacket.java ! test/sun/management/jdp/JdpClient.java ! test/sun/management/jdp/JdpDoSomething.java ! test/sun/management/jdp/JdpTest.sh
hg: jdk8/tl/jdk: 8024071: In ManagementAgent.start it should be possible to set the jdp.name parameter.
Changeset: 392acefef659 Author:dsamersoff Date: 2013-10-19 20:59 +0400 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/392acefef659 8024071: In ManagementAgent.start it should be possible to set the jdp.name parameter. Summary: Pass one more property from Agent to JdpController Reviewed-by: jbachorik, sla ! src/share/classes/sun/management/Agent.java ! test/sun/management/jdp/JdpTest.sh ! test/sun/management/jmxremote/startstop/JMXStartStopTest.sh
Re: RFR [9]: 8034174 Remove use of JVM_* functions from java.net code
Chris, Didn't look to windows part. Unix part looks good for me. See also below. I'm a bit concerned because of mixing NET_* abstractions and direct call to OS functions. It might be better to create NET_socket etc. We use NET_GetSockOpt/NET_SetSockOpt in one places and plain os functions in other ones it have to be unified. (not to your changes, but as far as you touched it) Doing socklen_t n = sizeof(m) it's better cast to socklen_t explicitly - on some platform socklen_t expands to int but sizeof to unsigned so it can cause a compiler warning. It's better to unify check of return value of os functions either as == -1 or as < 0 1. net_util.c Do we really need to check JNI_VERSION ? 2. Inet4AddressImpl.c 73, 335 it's better to use NI_MAXHOST in both places 784 optlen should be socklen_t 3. Inet6AddressImpl.c 73, 143 it's better to use NI_MAXHOST in both places 4. net_util_md.c 235 gettimeofday is obsoleted and might be not available on all platforms. So it's better to try clock_gettime first -Dmitry On 2014-02-22 12:29, Chris Hegarty wrote: > Interruptible I/O on Solaris has been highly problematic and the long > standing plan has been to remove it from the JDK. In JDK6 the VM option > UseVMInterruptibleIO was introduced to allow developers/customers experiment > with disabling it. In JDK7 the default value of UseVMInterruptibleIO was > changed to be "false" so that it is disabled by default. It is now finally > being removed. > > This bug tracks changing the native in src/share/native/java/net and > src/solaris/native/java/net so that the system calls are used directly rather > than going through the JVM_* functions. > > http://cr.openjdk.java.net/~chegar/8034174/webrev.00/webrev/ > > -Chris. > -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.
Re: RFR [9]: 8034174 Remove use of JVM_* functions from java.net code
Chris, You probably need to modify jdk/make/mapfiles/libnet/mapfile-vers -Dmitry On 2014-02-24 18:19, Chris Hegarty wrote: > On 24/02/14 14:12, Michael McMahon wrote: >> On 24/02/14 14:09, Chris Hegarty wrote: >>> On 24/02/14 10:42, Michael McMahon wrote: >>>> On 23/02/14 08:55, Chris Hegarty wrote: >>>>> On 22 Feb 2014, at 17:23, Dmitry Samersoff >>>>> wrote: >>>>> >>>>>> Chris, >>>>>> >>>>>> Didn't look to windows part. Unix part looks good for me. See also >>>>>> below. >>>>>> >>>>>> I'm a bit concerned because of mixing NET_* abstractions and direct >>>>>> call >>>>>> to OS functions. It might be better to create NET_socket etc. >>>>> Me too. It is already a mess. System calls should be used directly, >>>>> unless there is a reason not to do so. >>>>> >>>>>> We use NET_GetSockOpt/NET_SetSockOpt in one places and plain os >>>>>> functions in other ones it have to be unified. >>>>> If there is no reason to call the NET_ variant, then the system call >>>>> should be used. >>>> >>>> Seems like the big #ifdef in net_util_md.h on this is more or less >>>> redundant now >>>> since the #define of NET_xxx to JVM_xxx was its only purpose. >>> >>> The only difference between these now is that the bsd/linux variant >>> are defined in a separate file ( extern ), bsd_close/linux_close. I'm >>> not sure, but I think the use of extern is still required here. >>> >> >> I think extern would be okay in the other case though. All C functions >> are extern unless >> declared static afaik. > > Thanks. I'll include extern, and remove the other definitions. > > -Chris. > >> >>>> I wonder would it also be useful to expand the comment just above those >>>> definitions >>>> that currently just relates to AIX and say which other operating >>>> systems >>>> it applies to >>>> and if we could identify which system calls it affects, and which mean >>>> the NET_xx >>>> functions must be used. Or maybe this is going beyond what you >>>> wanted to >>>> do here? >>> >>> Beyond ;-) There is still a lot of cleanup that I want to make to this >>> code, but I'd like to do it incrementally, starting with breaking the >>> dependency on the VM interface. This makes it easier, certainly from a >>> review point of view. >>> >>> -Chris. >>> >>>> >>>> Michael >> -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.
Re: RFR [9] 8034181: SIGBUS at Java_sun_nio_ch_SctpChannelImpl_receive0
Chris, It's better to check that rv is less than NOTIFICATION_BUFFER_SIZE. -Dmitry On 2014-03-22 12:13, Chris Hegarty wrote: > The native SCTP implementation assumes that the given byte buffer ( buffer > address + position ) is memory aligned. It re-uses the buffer for handling > notifications from the SCTP Stack ( as well as for reading data off the > socket ). This can result in a SIBGUS on sparc(v9) if the address is not 4 > byte aligned [1]. > > The trivial solution is to copy the SCTP notification into a stack allocated > buffer, for handling, if the given address is not 4 byte aligned. > > http://cr.openjdk.java.net/~chegar/8034181/webev.00/webrev/ > > -Chris > > [1] https://bugs.openjdk.java.net/browse/JDK-8034181 > -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the source code.
Re: RFR: JDK-8054118 - java/net/ipv6tests/UdpTest.java failed intermittently
Mark, We can put nic.getDisplayName() under isWindows and save a bit of computer power. -Dmitry On 2014-08-07 22:15, Mark Sheppard wrote: > Hi, > please oblige and review the following fix > http://cr.openjdk.java.net/~msheppar/8054118/webrev/ > > to address the issue raised in > https://bugs.openjdk.java.net/browse/JDK-8054118 > > The Windows test environment has a Teredo interface. > This oscillates between being configured with IPv6 and not been configured. > The configured state is transient. When configured its IPv6 is retrieved as > first local ipv6 address. As such, it can happen that during the test the > interface is disabled, and the test will fail with the report Socket > receive timeout. > > This fix removes the Teredo interface from the testing scenario, so that > a more stable interface > will be used. > > regards > Mark -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.
Re: RFR: JDK-8058932 - java/net/InetAddress/IPv4Formats.java failed because hello.foo.bar does exist
Mark,
It probably should be some-name.invalid
IANA reserve .invalid TLD for tests like this one
see:
http://www.iana.org/assignments/special-use-domain-names/special-use-domain-names.xhtml
-Dmitry
On 2014-09-30 19:21, Mark Sheppard wrote:
> Hi
>
> Please oblige and review the following small change to test
> test/java/net/InetAddress/IPv4Formats.java
>
> --- a/test/java/net/InetAddress/IPv4Formats.javaTue Sep 30
> 13:25:04 2014 +0100
> +++ b/test/java/net/InetAddress/IPv4Formats.javaTue Sep 30
> 15:11:05 2014 +0100
> @@ -36,7 +36,7 @@
> {"126.1", "126.0.0.1"},
> {"128.50.65534", "128.50.255.254"},
> {"192.168.1.2", "192.168.1.2"},
> -{"hello.foo.bar", null},
> +{"somehost.some-domain", null},
> {"1024.1.2.3", null},
> {"128.14.66000", null }
>
> which addresses the issue
>
> https://bugs.openjdk.java.net/browse/JDK-8058932
>
> ping hello.foo.bar
>
> Pinging hello.foo.bar [127.0.53.53] with 32 bytes of data:
> Reply from 127.0.53.53: bytes=32 time<1ms TTL=128
> Reply from 127.0.53.53: bytes=32 time<1ms TTL=128
> Reply from 127.0.53.53: bytes=32 time<1ms TTL=128
> Reply from 127.0.53.53: bytes=32 time<1ms TTL=128
>
> this highlights a DNS configuration issue as indicated in
> https://www.icann.org/resources/pages/name-collision-2013-12-06-en
>
> so we remove foo.bar from the test and replace with somehost.some-domain
>
> regards
> Mark
--
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.
Re: RFR: JDK-8058932 - java/net/InetAddress/IPv4Formats.java failed because hello.foo.bar does exist
Looks good for me!
-Dmitry
On 2014-10-01 02:26, Mark Sheppard wrote:
> Thanks Tom and Dmitry
>
> last up best dressed ...
>
> .invalid as the test domain is a good recommendation
>
> change is now
>
> --- a/test/java/net/InetAddress/IPv4Formats.javaTue Sep 30
> 13:25:04 2014 +0100
> +++ b/test/java/net/InetAddress/IPv4Formats.javaTue Sep 30
> 23:23:46 2014 +0100
> @@ -36,7 +36,7 @@
> {"126.1", "126.0.0.1"},
> {"128.50.65534", "128.50.255.254"},
> {"192.168.1.2", "192.168.1.2"},
> -{"hello.foo.bar", null},
> +{"invalidhost.invalid", null},
> {"1024.1.2.3", null},
> {"128.14.66000", null }
>
>
> regards
> Mark
>
>
> On 30/09/2014 18:53, Dmitry Samersoff wrote:
>> Mark,
>>
>> It probably should be some-name.invalid
>>
>> IANA reserve .invalid TLD for tests like this one
>>
>> see:
>>
>> http://www.iana.org/assignments/special-use-domain-names/special-use-domain-names.xhtml
>>
>>
>> -Dmitry
>>
>> On 2014-09-30 19:21, Mark Sheppard wrote:
>>> Hi
>>>
>>> Please oblige and review the following small change to test
>>> test/java/net/InetAddress/IPv4Formats.java
>>>
>>> --- a/test/java/net/InetAddress/IPv4Formats.javaTue Sep 30
>>> 13:25:04 2014 +0100
>>> +++ b/test/java/net/InetAddress/IPv4Formats.javaTue Sep 30
>>> 15:11:05 2014 +0100
>>> @@ -36,7 +36,7 @@
>>> {"126.1", "126.0.0.1"},
>>> {"128.50.65534", "128.50.255.254"},
>>> {"192.168.1.2", "192.168.1.2"},
>>> -{"hello.foo.bar", null},
>>> +{"somehost.some-domain", null},
>>> {"1024.1.2.3", null},
>>> {"128.14.66000", null }
>>>
>>> which addresses the issue
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-8058932
>>>
>>> ping hello.foo.bar
>>>
>>> Pinging hello.foo.bar [127.0.53.53] with 32 bytes of data:
>>> Reply from 127.0.53.53: bytes=32 time<1ms TTL=128
>>> Reply from 127.0.53.53: bytes=32 time<1ms TTL=128
>>> Reply from 127.0.53.53: bytes=32 time<1ms TTL=128
>>> Reply from 127.0.53.53: bytes=32 time<1ms TTL=128
>>>
>>> this highlights a DNS configuration issue as indicated in
>>> https://www.icann.org/resources/pages/name-collision-2013-12-06-en
>>>
>>> so we remove foo.bar from the test and replace with somehost.some-domain
>>>
>>> regards
>>> Mark
>>
>
--
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.
Re: RFR 8067105: Socket returned by ServerSocket.accept() is inherited by child process on Windows
Chris, In a windows world it's rather common to rely on socket inheritance e.g. set socket to stdin(stdout) of child process and do CreateProcess. So please make sure your changes don't have side effects, also it might be better to control handler inheritance in one place only (at CreateProcess call) and don't manage it explicitly in other places. -Dmitry On 2014-12-17 18:47, Chris Hegarty wrote: > A socket connection which is returned by ServerSocket.accept() is > inherited by a child process. The expected behavior is that the socket > connection is not inherited by the child process. This is an oversight > in the original implementation, that only sets HANDLE_FLAG_INHERIT for > newly created sockets. > > The native socket returned by ServerSocket.accept() should be configured > so it will not be inherited by a child process, > SetHandleInformation(, HANDLE_FLAG_INHERIT, FALSE) . > > The change is in Java_java_net_DualStackPlainSocketImpl_accept0 > > diff --git > a/src/java.base/windows/native/libnet/DualStackPlainSocketImpl.c > b/src/java.base/windows/native/libnet/DualStackPlainSocketImpl.c > --- a/src/java.base/windows/native/libnet/DualStackPlainSocketImpl.c > +++ b/src/java.base/windows/native/libnet/DualStackPlainSocketImpl.c > @@ -294,6 +294,8 @@ > return -1; > } > > +SetHandleInformation((HANDLE)(UINT_PTR)newfd, HANDLE_FLAG_INHERIT, > FALSE); > + > ia = NET_SockaddrToInetAddress(env, (struct sockaddr *)&sa, &port); > isa = (*env)->NewObject(env, isa_class, isa_ctorID, ia, port); > (*env)->SetObjectArrayElement(env, isaa, 0, isa); > > -Chris. -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.
Re: RFR 8067105: Socket returned by ServerSocket.accept() is inherited by child process on Windows
Chris, > OK, but there can be no expectation that something like this can work > with java.net.Socket, or SocketChannel. I am only proposing to change > these two specific implementations. OK. Thank you for clarification. The fix looks good for me. -Dmitry On 2014-12-18 15:52, Chris Hegarty wrote: > On 18 Dec 2014, at 12:22, Dmitry Samersoff > wrote: > >> Chris, >> >> In a windows world it's rather common to rely on socket inheritance >> e.g. set socket to stdin(stdout) of child process and do CreateProcess. > > OK, but there can be no expectation that something like this can work with > java.net.Socket, or SocketChannel. I am only proposing to change these two > specific implementations. > >> So please make sure your changes don't have side effects, > > The changes will have an effect, accepted sockets/channels will no longer be > inherited. No other potential side-effect is possible. > >> also it might >> be better to control handler inheritance in one place only (at >> CreateProcess call) and don't manage it explicitly in other places. > > There is some handling of a small set of “special” handles in the native > process implementation. I don’t think we want to prevent all handles from > being inherited. Are you suggesting something different ? > > -Chris. > >> -Dmitry >> >> >> On 2014-12-17 18:47, Chris Hegarty wrote: >>> A socket connection which is returned by ServerSocket.accept() is >>> inherited by a child process. The expected behavior is that the socket >>> connection is not inherited by the child process. This is an oversight >>> in the original implementation, that only sets HANDLE_FLAG_INHERIT for >>> newly created sockets. >>> >>> The native socket returned by ServerSocket.accept() should be configured >>> so it will not be inherited by a child process, >>> SetHandleInformation(, HANDLE_FLAG_INHERIT, FALSE) . >>> >>> The change is in Java_java_net_DualStackPlainSocketImpl_accept0 >>> >>> diff --git >>> a/src/java.base/windows/native/libnet/DualStackPlainSocketImpl.c >>> b/src/java.base/windows/native/libnet/DualStackPlainSocketImpl.c >>> --- a/src/java.base/windows/native/libnet/DualStackPlainSocketImpl.c >>> +++ b/src/java.base/windows/native/libnet/DualStackPlainSocketImpl.c >>> @@ -294,6 +294,8 @@ >>> return -1; >>> } >>> >>> +SetHandleInformation((HANDLE)(UINT_PTR)newfd, HANDLE_FLAG_INHERIT, >>> FALSE); >>> + >>> ia = NET_SockaddrToInetAddress(env, (struct sockaddr *)&sa, &port); >>> isa = (*env)->NewObject(env, isa_class, isa_ctorID, ia, port); >>> (*env)->SetObjectArrayElement(env, isaa, 0, isa); >>> >>> -Chris. >> >> >> -- >> Dmitry Samersoff >> Oracle Java development team, Saint Petersburg, Russia >> * I would love to change the world, but they won't give me the sources. > -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.
Re: DNS resolution fails after resolv.conf update
Stanislav, I don't think we should fix it on JDK level. Workaround is obvious and actually recommended for production - install one of cashing nameservers and lock resolv.conf to localhost. -Dmitry On 2015-05-04 18:38, Stanislav Baiduzhyi wrote: > Hi All, > > We are facing an issue with DNS server caching on RHEL-based distros: after > the update of resolv.conf java application cannot resolve the hosts any more. > > Reproducer is very simple: > 1. Clean /etc/resolv.conf or connect to vpn and use vpn-only nameserver. > 2. Launch the minimal java app [1]. > 3. Restore the /etc/resolv.conf or disconnect from vpn (/etc/resolv.conf > should be updated with accessible nameserver at this moment). > 4. Notice that name resolution continues to fail. > > I've prepared a small webrev that fixes the issue [2], but would like to hear > some ideas/criticism first. If the patch looks ok to everyone, please create > an issue for this, and I will submit it for approval/sponsorship. > > [1]: > https://e61b615da46327c9050d624b0a3783ba21c6b125.googledrive.com/host/0B5Kp-cB1sXJrfnZ4NHZ0S1V3UTJZcDFra3RwUFZjQXY5WVZzUkwtTTd0Z1IyR1JmbDZPSVk/resolvconf-reinit/ResolvingHost.java > > [2]: > https://e61b615da46327c9050d624b0a3783ba21c6b125.googledrive.com/host/0B5Kp-cB1sXJrfnZ4NHZ0S1V3UTJZcDFra3RwUFZjQXY5WVZzUkwtTTd0Z1IyR1JmbDZPSVk/resolvconf-reinit/ > -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.
Re: RFR(S): 8156521: Minor Fixes and cleanups in NetworkInterface.c
Christoph, Looks good for me (Reviewed). Few more space nits (no need to re-review) 1141 Missed space around = in i=0 1890 extra space after ( 2068 wrong indent -Dmitry On 2016-05-10 12:54, Langer, Christoph wrote: > Hi all, > > > > can I please get a review for a change to NetworkInterface.c > > > > bugreport: https://bugs.openjdk.java.net/browse/JDK-8156521 > > webrev: http://cr.openjdk.java.net/~clanger/webrevs/8156521.0/ > > > > Apart from quite a few whitespace changes to clean up the coding, I went > through and replaced all occurences of strcpy with strncpy as this was a > finding of a code scanner that we used. Also in function > “enumIPv6Interfaces” for Linux the local variable plen was changed from > int to short. > > > > I ran builds on Linux, AIX, Solaris Sparc and Darwin to make sure > nothing broke. > > > > Thanks in advance > > Christoph > > > -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.
Re: RFR 8157811 [9] Additional minor fixes and cleanups in Networking native code
Chris,
Looks good for me!
On 2016-05-25 16:22, Chris Hegarty wrote:
> Thanks. I included it, though it may not show in the webrev ( white space
> change )
>
> I generalise the issue to cover a few other minor issues.
>
> Webrev updated in-place:
> http://cr.openjdk.java.net/~chegar/8157811/
>
> -Chris.
>
>> On 25 May 2016, at 13:59, Langer, Christoph wrote:
>>
>> Hi Chris,
>>
>> looks nice, I had seen some of these places, too.
>>
>> Here is another one which you could add:
>>
>> --- a/src/java.base/unix/native/libnet/NetworkInterface.c Tue May 24
>> 10:14:41 2016 -0700
>> +++ b/src/java.base/unix/native/libnet/NetworkInterface.c Wed May 25
>> 14:56:31 2016 +0200
>> @@ -1829,7 +1829,7 @@
>> strncpy(if2.lifr_name, ifname, sizeof(if2.lifr_name) - 1);
>>
>> if (ioctl(sock, SIOCGLIFFLAGS, (char *)&if2) < 0) {
>> - return -1;
>> +return -1;
>> }
>>
>> *flags = if2.lifr_flags;
>>
>> Best regards
>> Christoph
>>
>>> -Original Message-
>>> From: net-dev [mailto:net-dev-boun...@openjdk.java.net] On Behalf Of Chris
>>> Hegarty
>>> Sent: Mittwoch, 25. Mai 2016 11:50
>>> To: OpenJDK Network Dev list
>>> Subject: RFR 8157811 [9] Additional minor fixes and cleanups in
>>> NetworkInterface.c
>>>
>>> As a follow up to JDK-8156521, and when comparing against the
>>> version of this file in jdk8u-dev a few minor issues were noticed.
>>>
>>> There is a free of a memory structure that was missed somehow
>>> in the 9 version of this file ( it is already in the 8 version ). The
>>> remaining few changes are just some minor cleanup.
>>>
>>> http://cr.openjdk.java.net/~chegar/8157811/
>>> https://bugs.openjdk.java.net/browse/JDK-8157811
>>>
>>> -Chris.
>
--
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.
Re: 8158519: Incorrect network mask and broadcast address on Linux when there are multiple IPv4 addresses on an interface
Chris,
Looks good for me. Only minor nits (no need to re-review).
1040 missed space before {
1158 please, add comment and {} around continue.
1938 I'm not sure that check for IFF_POINTOPOINT is necessary (but
don't mid to leave it). Do you know the case when both flags are set?
-Dmitry
On 2016-06-02 15:06, Chris Hegarty wrote:
> Doychin, et al,
>
> I finally got time to look at the issue you reported and your suggested
> patch. I filed
> 8158519 [1] to track the issue. I think your suggested patch may be ok, but I
> just
> wanted to push on the ioctl approach. I found an alternative, and verified
> that it
> works as expected. Please take a look, and verify in your environment. Then
> we
> need to weigh up the two separate approaches.
>
> http://cr.openjdk.java.net/~chegar/8158519.00/
>
> For the record, I don’t have any specific issue with using getifaddrs, I just
> wanted to
> see if there was a less invasive change. That said, using getifaddrs could
> lead to
> cleaner code, but we would need to check the situation on AIX ( which I don’t
> have
> access to ).
>
> -Chris.
>
> [1] https://bugs.openjdk.java.net/browse/JDK-8158519
>
Re: Help needed: Java Socket and close detection
Christoph, My $0.2 Typically you see RST packet when the data come to a *closed* socket. You shouldn't get RST if client/server communication shutdown properly. Also balancer may take a care about connection shutdown (it needs to update internal tables) so it's possible that you never get RST after balancer. Moreover, there are RST based attack and scanning methods so firewall might be set to reject RST packets and doesn't pass it to inside network. I.e. IMHO, customer server should be updated to live without RST. -Dmitry On 2016-07-13 18:29, Langer, Christoph wrote: > Hi folks, > > > > I have a question to the experts - regarding an issue that was reported > to me by a customer. > > > > In the customer scenario they are running a Servlet engine and the > Servlet is constantly sending data to a browser client. When the browser > client is closed, the server does not get a notification of the other > end having been terminated and is constantly sending out data and > blocking an application thread. I’m under the assumption that the server > should get an RST packet from the network upon writing/flushing data to > the OutputStream as soon as the client is gone and hence an Exception > should pop up but this isn’t happening. > > > > There is a load balancer and maybe other network infrastructure involved > in between the Servlet JVM and the browser client. We did some TCPDUMP > tracing at the load balancer and we could not see an RST packet coming > in from the client side. But when I’m running the scenario without all > this network infrastructure involved, e.g. between servers and clients > in the same network, I would always observe an RST packet once I close > the browser. A FIN packet is received, too, but this does not lead to an > Exception and to all my knowledge this can’t be detected, not from the > java Socket API and even less from the Servlet API which is just dealing > with Streams. > > > > So my question to the experts is most of all: Would you agree that an > RST packet should be generated in the network and received by the > server? Or is it a normal behavior that servers must deal with not > receiving RSTs and hence needing to wait for a timeout until e.g. the > load balancer generates an RST? Also, is there any way to detect a FIN > in the JVM and react on it? > > > > Thanks in advance and best regards > > Christoph > > > -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.
Re: RFR 8075484:SocketInputStream.socketRead0 can hang even with soTimeout set
Vyom,
Did you consider to use select() to calculate timeout instead of
gettimeofday ?
gettimeofday is affected by system time changes, so running ntpd can
cause unpredictable behavior of this code. Also it's rather expensive
syscall.
-Dmitry
On 2016-09-01 19:03, Vyom Tewari wrote:
> please find the updated
> webrev(http://cr.openjdk.java.net/~vtewari/8075484/webrev0.1/index.html
> <http://cr.openjdk.java.net/%7Evtewari/8075484/webrev0.1/index.html>). I
> incorporated the review comments.
>
> Thanks,
>
> Vyom
>
>
> On Tuesday 30 August 2016 04:11 PM, Mark Sheppard wrote:
>>
>> Hi
>> perhaps there is an opportunity to do some refactoring here (...
>> for me a "goto " carries a code smell! )
>>
>> along the lines
>>
>> if (timeout) {
>> nread = NET_ReadWithTimeout(...);
>> } else {
>> nread = NET_Read(...);
>> }
>>
>>
>> the NET_ReadWithTimeout (...) function will contain a restructuring of
>> your goto loop
>> while (_timeout > 0) { nread = NET_Timeout(fd, _timeout);
>> if (nread <= 0) {
>> if (nread == 0) {
>> JNU_ThrowByName(env, JNU_JAVANETPKG
>> "SocketTimeoutException",
>> "Read timed out");
>> } else if (nread == -1) {
>> if (errno == EBADF) {
>>JNU_ThrowByName(env, JNU_JAVANETPKG
>> "SocketException", "Socket closed");
>> } else if (errno == ENOMEM) {
>> JNU_ThrowOutOfMemoryError(env, "NET_Timeout native
>> heap allocation failed");
>> } else {
>> JNU_ThrowByNameWithMessageAndLastError
>> (env, JNU_JAVANETPKG "SocketException",
>> "select/poll failed");
>> }
>> }
>> // release buffer in main call flow
>> // if (bufP != BUF) {
>> // free(bufP);
>> // }
>> nread = -1;
>> break;
>> } else {
>> nread = NET_NonBlockingRead(fd, bufP, len);
>> if (nread == -1 && ((errno == EAGAIN) || (errno == EWOULDBLOCK))) {
>> gettimeofday(&t, NULL);
>> newtime = t.tv_sec * 1000 + t.tv_usec / 1000;
>> _timeout -= newtime - prevtime;
>> if(_timeout > 0){
>> prevtime = newtime;
>> }
>> } else { break; } } } return nread;
>>
>> e&oe
>>
>> regards
>> Mark
>>
>>
>> On 29/08/2016 10:58, Vyom Tewari wrote:
>>> gentle reminder, please review the below code change.
>>>
>>> Vyom
>>>
>>>
>>> On Monday 22 August 2016 05:12 PM, Vyom Tewari wrote:
>>>> Hi All,
>>>>
>>>> Please review the code changes for below issue.
>>>>
>>>> Bug : https://bugs.openjdk.java.net/browse/JDK-8075484
>>>>
>>>> webrev:
>>>> http://cr.openjdk.java.net/~vtewari/8075484/webrev0.0/index.html
>>>> <http://cr.openjdk.java.net/%7Evtewari/8075484/webrev0.0/index.html>
>>>>
>>>> This issue is SocketInputStream.socketread0() hangs even with
>>>> "soTimeout" set.the implementation of
>>>> Java_java_net_SocketInputStream_socketRead0 assumes that read()
>>>> won't block after poll() reports that a read is possible.
>>>>
>>>> This assumption does not hold, as noted on the man page for select
>>>> (referenced by the man page for poll): Under Linux, select() may
>>>> report a socket file descriptor as "ready for reading", while
>>>> nevertheless a subsequent read blocks. This could for example happen
>>>> when data has arrived but upon examination has wrong checksum and is
>>>> discarded.
>>>>
>>>> Thanks,
>>>>
>>>> Vyom
>>>>
>>>>
>>>
>>
>
--
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.
Re: RFR 8075484:SocketInputStream.socketRead0 can hang even with soTimeout set
Vyom,
> thanks for review, I did consider to use a monotonically increasing
> clock like "clock_gettime" but existing nearby code("NET_Timeout")
> uses "gettimeofday" so i choose to be consistent with the existing
> code.
OK. The fix looks good for me.
-Dmitry
On 2016-09-02 06:39, Vyom Tewari wrote:
> hi Dimitry,
>
> thanks for review, I did consider to use a monotonically increasing
> clock like "clock_gettime" but existing nearby code("NET_Timeout") uses
> "gettimeofday" so i choose to be consistent with the existing code.
>
> Thanks,
>
> Vyom
>
>
> On Friday 02 September 2016 01:38 AM, Dmitry Samersoff wrote:
>> Vyom,
>>
>> Did you consider to use select() to calculate timeout instead of
>> gettimeofday ?
>>
>> gettimeofday is affected by system time changes, so running ntpd can
>> cause unpredictable behavior of this code. Also it's rather expensive
>> syscall.
>>
>> -Dmitry
>>
>> On 2016-09-01 19:03, Vyom Tewari wrote:
>>> please find the updated
>>> webrev(http://cr.openjdk.java.net/~vtewari/8075484/webrev0.1/index.html
>>> <http://cr.openjdk.java.net/%7Evtewari/8075484/webrev0.1/index.html>). I
>>> incorporated the review comments.
>>>
>>> Thanks,
>>>
>>> Vyom
>>>
>>>
>>> On Tuesday 30 August 2016 04:11 PM, Mark Sheppard wrote:
>>>> Hi
>>>>perhaps there is an opportunity to do some refactoring here (...
>>>> for me a "goto " carries a code smell! )
>>>>
>>>> along the lines
>>>>
>>>> if (timeout) {
>>>> nread = NET_ReadWithTimeout(...);
>>>> } else {
>>>> nread = NET_Read(...);
>>>> }
>>>>
>>>>
>>>> the NET_ReadWithTimeout (...) function will contain a restructuring of
>>>> your goto loop
>>>> while (_timeout > 0) { nread = NET_Timeout(fd, _timeout);
>>>>if (nread <= 0) {
>>>>if (nread == 0) {
>>>>JNU_ThrowByName(env, JNU_JAVANETPKG
>>>> "SocketTimeoutException",
>>>>"Read timed out");
>>>>} else if (nread == -1) {
>>>>if (errno == EBADF) {
>>>> JNU_ThrowByName(env, JNU_JAVANETPKG
>>>> "SocketException", "Socket closed");
>>>>} else if (errno == ENOMEM) {
>>>>JNU_ThrowOutOfMemoryError(env, "NET_Timeout
>>>> native heap allocation failed");
>>>>} else {
>>>>JNU_ThrowByNameWithMessageAndLastError
>>>>(env, JNU_JAVANETPKG "SocketException",
>>>> "select/poll failed");
>>>>}
>>>>}
>>>> // release buffer in main call flow
>>>> // if (bufP != BUF) {
>>>> // free(bufP);
>>>> // }
>>>> nread = -1;
>>>> break;
>>>>} else {
>>>> nread = NET_NonBlockingRead(fd, bufP, len);
>>>> if (nread == -1 && ((errno == EAGAIN) || (errno == EWOULDBLOCK))) {
>>>>gettimeofday(&t, NULL);
>>>> newtime = t.tv_sec * 1000 + t.tv_usec / 1000;
>>>> _timeout -= newtime - prevtime;
>>>> if(_timeout > 0){
>>>> prevtime = newtime;
>>>>}
>>>> } else { break; } } } return nread;
>>>>
>>>> e&oe
>>>>
>>>> regards
>>>> Mark
>>>>
>>>>
>>>> On 29/08/2016 10:58, Vyom Tewari wrote:
>>>>> gentle reminder, please review the below code change.
>>>>>
>>>>> Vyom
>>>>>
>>>>>
>>>>> On Monday 22 August 2016 05:12 PM, Vyom Tewari wrote:
>>>>>> Hi All,
>>>>>>
>>>>>> Please review the code changes for below issue.
>>>>>>
>>>>>> Bug : https://bugs.openjdk.java.net/browse/JDK-8075484
>>>>>>
>>>>>> webrev:
>>>>>> http://cr.openjdk.java.net/~vtewari/8075484/webrev0.0/index.html
>>>>>> <http://cr.openjdk.java.net/%7Evtewari/8075484/webrev0.0/index.html>
>>>>>>
>>>>>> This issue is SocketInputStream.socketread0() hangs even with
>>>>>> "soTimeout" set.the implementation of
>>>>>> Java_java_net_SocketInputStream_socketRead0 assumes that read()
>>>>>> won't block after poll() reports that a read is possible.
>>>>>>
>>>>>> This assumption does not hold, as noted on the man page for select
>>>>>> (referenced by the man page for poll): Under Linux, select() may
>>>>>> report a socket file descriptor as "ready for reading", while
>>>>>> nevertheless a subsequent read blocks. This could for example happen
>>>>>> when data has arrived but upon examination has wrong checksum and is
>>>>>> discarded.
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Vyom
>>>>>>
>>>>>>
>>
>
--
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.
Re: RFR 8075484:SocketInputStream.socketRead0 can hang even with soTimeout set
Vyom,
Looks good for me!
SocketInputStream.c:68
It's better to check for both EAGAIN and EINTR after poll()
(no need to re-review).
-Dmitry
On 2016-09-06 12:20, Vyom Tewari wrote:
> Hi,
>
> Please find the latest
> webrev(http://cr.openjdk.java.net/~vtewari/8075484/webrev0.2/index.html
> <http://cr.openjdk.java.net/%7Evtewari/8075484/webrev0.2/index.html>). I
> have incorporated the review comments.
>
> Thanks,
>
> Vyom
>
>
> On Monday 05 September 2016 08:37 PM, Chris Hegarty wrote:
>> On 05/09/16 15:37, Mark Sheppard wrote:
>>>
>>> if the desire is to avoid making double calls on gettimeofday in the
>>> NET_ReadWithTimeout's while loop for its main call flow,
>>
>> Yes, the desire is to make no more calls to gettimeofday, than are
>> already done.
>>
>>> then consider a modification to NET_Timeout and create a version
>>> int NET_TimeoutWithCurrentTime(int s, long timeout, stuct timeval *
>>> current_time)
>>>
>>> int NET_TimeoutWithCurrentTime(int s, long timeout, stuct timeval *
>>> current_time) {
>>> int result;
>>> struct timeval t;
>>> long prevtime, newtime;
>>> struct pollfd pfd;
>>> pfd.fd = s;
>>> pfd.events = POLLIN;
>>>
>>> if (timeout > 0) {
>>> prevtime = (current_time->tv_sec * 1000) +
>>> current_time->tv_usec / 1000;
>>> }
>>>
>>> for(;;) {
>>> result = poll(&pfd, 1, timeout);
>>> if (result < 0 && errno == EINTR) {
>>> if (timeout > 0) {
>>> gettimeofday(&t, NULL);
>>> newtime = (t.tv_sec * 1000) + t.tv_usec /1000;
>>> timeout -= newtime - prevtime;
>>> if (timeout <= 0)
>>> return 0;
>>> prevtime = newtime;
>>> }
>>> } else {
>>> return result;
>>> }
>>> }
>>> }
>>>
>>> the NET_ReadWithTimeout is modified with.
>>>
>>>while (timeout > 0) {
>>> result = NET_TimeoutWithCurrentTime(fd, timeout, &t);
>>>
>>> in any case there is the potential for multiple invocation of
>>> gettimeofday due to a need to make
>>> poll restartable,
>>
>> Yes, and that is fine. Just no more than is already there.
>>
>> and adjust the originally specified timeout
>>> accordingly, and for the edge case
>>> after the non blocking read.
>>
>> After an initial skim, your suggestion seems reasonable.
>>
>> -Chris.
>>
>>> regards
>>> Mark
>>>
>>>
>>>
>>> On 05/09/2016 12:02, Chris Hegarty wrote:
>>>> Vyom,
>>>>
>>>> >>>
>>>> webrev(http://cr.openjdk.java.net/~vtewari/8075484/webrev0.1/index.html
>>>>
>>>> There is some concern about the potential performance effect, etc,
>>>> of gettimeofday, maybe there is a way out of this. The reuse of
>>>> NET_Timeout is good, but it also calls gettimeofday. It seems that
>>>> a specific NET_ReadWithTimeout could be written to NOT reuse
>>>> NET_Timeout, but effectively inline its interruptible operation.
>>>> Or write a variant of NET_Timeout that takes a function to
>>>> execute. Rather than effectively two loops conditioned on the
>>>> timeout. Disclaimer: I have not actually tried to do this, but
>>>> it seems worth considering / evaluating.
>>>>
>>>> -Chris.
>>>>
>>>> On 02/09/16 04:39, Vyom Tewari wrote:
>>>>> hi Dimitry,
>>>>>
>>>>> thanks for review, I did consider to use a monotonically increasing
>>>>> clock like "clock_gettime" but existing nearby code("NET_Timeout")
>>>>> uses
>>>>> "gettimeofday" so i choose to be consistent with the existing code.
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Vyom
>>>>>
>>>>>
>>>>> On Friday 02 September 2016 01:38 AM, Dmitry Samersoff wrote:
>>>>>> Vyom,
>>>>>>
>>>>>> Did you consider to use select() to calculate timeout instead of
>>>>>> gettimeofday ?
>>>>>>
>>>>>> gettimeofday
Re: RFR 8169115, java/net/InetAddress/ptr/lookup.sh failed intermittently
Felix, 1. I'm not sure that javaweb.sfbay.sun.com is the best domain name for this test. Could we use different one (e.g. icann.org) 2. This test run JTREG -> Test VM -> Another VM. Could we optimize process usage? It might be better to create a jtreg "same vm" process that 1. launch another process with -Djava.net.preferIPv4Stack=true that do A and PRT lookup in one run. 2. Read results of process above, do PTR lookup with default settings and compare results. -Dmitry On 2016-12-06 12:06, Felix Yang wrote: > Hi, > >please review the following patch. It generally coverts codes from > shell to plain java. > > Bug: > > https://bugs.openjdk.java.net/browse/JDK-8169115 > > Webrev: > > http://cr.openjdk.java.net/~xiaofeya/8169115/webrev.00/ > > Thanks, > > Felix > -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.
Re: RFR 8169115, java/net/InetAddress/ptr/lookup.sh failed intermittently
Felix, Changes looks good to me. Thank you for doing it. -Dmitry On 2016-12-08 13:35, Felix Yang wrote: > Hi Dmitry, > >I tested your suggested "icann.org" and it indeed works well. Please > review the updated webrev: > > http://cr.openjdk.java.net/~xiaofeya/8169115/webrev.02/ > > Thanks, > Felix > On 2016/12/6 19:16, Dmitry Samersoff wrote: >> Felix, >> >> 1. I'm not sure that javaweb.sfbay.sun.com is the best domain name for >> this test. Could we use different one (e.g. icann.org) >> >> 2. This test run JTREG -> Test VM -> Another VM. Could we optimize >> process usage? >> >> It might be better to create a jtreg "same vm" process that >> >> 1. launch another process with -Djava.net.preferIPv4Stack=true >> that do A and PRT lookup in one run. >> >> 2. Read results of process above, do PTR lookup with default >> settings and compare results. >> >> -Dmitry >> >> >> On 2016-12-06 12:06, Felix Yang wrote: >>> Hi, >>> >>> please review the following patch. It generally coverts codes from >>> shell to plain java. >>> >>> Bug: >>> >>> https://bugs.openjdk.java.net/browse/JDK-8169115 >>> >>> Webrev: >>> >>> http://cr.openjdk.java.net/~xiaofeya/8169115/webrev.00/ >>> >>> Thanks, >>> >>> Felix >>> >> > -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.
hg: jdk7/tl/jdk: 6931566: NetworkInterface is not working when interface name is more than 15 characters long
Changeset: 887e525597f8 Author:dsamersoff Date: 2010-06-23 17:25 +0400 URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/887e525597f8 6931566: NetworkInterface is not working when interface name is more than 15 characters long Summary: Separate Linux and Solaris code, use lifreq under Solaris Reviewed-by: chegar ! src/solaris/native/java/net/NetworkInterface.c
Re: 6964714, Please review the fix (XS), round 3
Chris, Please review my changes. see: http://cr.openjdk.java.net/~dsamersoff/6964714/webrev.03/ -Dmitry -- Dmitry Samersoff J2SE Sustaining team, SPB04 * Give Rabbit time and he'll always get the answer ...
Re: Code Review 6967937: Scope id no longer being set after 6931566
On 2010-07-12 13:54, Chris Hegarty wrote: Alan, Dmitry, Michael, CR: Scope id no longer being set after 6931566 Looks good for me. Thank you for fixing it. -Dmitry CR 6931566 accidentally removes the setting of the scope id for IPv6 addresses before adding them to the interface list. This was missed during code review as there was a lot of refactoring to clean up the code. The solution is to simply set the scope id as was the case in the original code. Webrev: http://cr.openjdk.java.net/~chegar/6967937/webrev.00/webrev/ Thanks, -Chris. -- Dmitry Samersoff J2SE Sustaining team, SPB04 * Give Rabbit time and he'll always get the answer ...
Re: 6964714, Please review the fix (XS), round 3
Alan, Thank you for the comments. Fixed. http://cr.openjdk.java.net/~dsamersoff/6964714/webrev.04/ -Dmitry On 2010-07-12 12:00, Alan Bateman wrote: Dmitry Samersoff wrote: Chris, Please review my changes. see: http://cr.openjdk.java.net/~dsamersoff/6964714/webrev.03/ -Dmitry The fix to NetworkInterface.c looks good to me. I notice you've got the Sun copyright header in the test so that should be fixed. Also I agree with Chris's comment that the test can simply check if the Enumeration contains an element that is an instance of Inet6Address. Minor nit is that you seem to be intending by 8 instead of 4 spaces. Given that the test is in the NetworkInterface directory then an alternative name might be IPv4Only.java (keep the name short?). -Alan. -- Dmitry Samersoff J2SE Sustaining team, SPB04 * Give Rabbit time and he'll always get the answer ...
Re: 6964714, Please review the fix (XS), round 3
On 2010-07-12 22:30, Alan Bateman wrote: Dmitry Samersoff wrote: Alan, Thank you for the comments. Fixed. http://cr.openjdk.java.net/~dsamersoff/6964714/webrev.04/ -Dmitry The copyright date on the new test says 2008 :-) Copy-paste from a neighbor ;) Fixed? http://cr.openjdk.java.net/~dsamersoff/6964714/webrev.05/ -Dmitry -- Dmitry Samersoff J2SE Sustaining team, SPB04 * Give Rabbit time and he'll always get the answer ...
hg: jdk7/tl/jdk: 6964714: NetworkInterface getInetAddresses enumerates IPv6 addresses if java.net.preferIPvStack property set
Changeset: 25050030a320 Author:dsamersoff Date: 2010-07-13 15:32 +0400 URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/25050030a320 6964714: NetworkInterface getInetAddresses enumerates IPv6 addresses if java.net.preferIPvStack property set Summary: User can disable ipv6 explicitly, have to check it Reviewed-by: chegar, alanb ! src/solaris/native/java/net/NetworkInterface.c + test/java/net/NetworkInterface/IPv4Only.java
Re: Code Review 6970262: TEST_BUG: test/java/net/NetworkInterface/IPv4Only.java has wrong test name in @run tag
Chris, Thank you for digging in! Looks good for me. -Dmitry On 2010-07-20 13:03, Chris Hegarty wrote: Dmitry, Alan, Trivially update the @run tag to the correct class name. diff -r f3a4c1947fd1 test/java/net/NetworkInterface/IPv4Only.java --- a/test/java/net/NetworkInterface/IPv4Only.java Tue Jul 13 20:27:01 2010 +0800 +++ b/test/java/net/NetworkInterface/IPv4Only.java Tue Jul 20 10:06:05 2010 +0100 @@ -23,7 +23,7 @@ /* @test * @bug 6964714 - * @run main/othervm NetworkInterfaceV6List + * @run main/othervm IPv4Only * @summary Test the networkinterface listing with java.net.preferIPv4Stack=true. */ -Chris. -- Dmitry Samersoff J2SE Sustaining team, SPB04 * Give Rabbit time and he'll always get the answer ...
Re: Code Review 6972374: NetworkInterface.getNetworkInterfaces throws "java.net.SocketException" on Solaris zone
Chris, Looks good for me. -Dmitry On 2010-07-28 12:25, Chris Hegarty wrote: Dmitry, Alan, The Solaris version of getFlags sets an Exception if the ioctl fails. When used in addif getFlags will fail when access to the virtual interface's parent is forbidden, i.e. in a zone. addif is called when iterating over interfaces in enumIPvXInterfaces, if an exception occurs it simply cleans up and returns, propagating the exception. getFalgs should not set an exception. All other calls to it check the return value and set an exception if appropriate. Webrev: http://cr.openjdk.java.net/~chegar/6972374/webrev.00/webrev/ Thanks, -Chris. -- Dmitry Samersoff J2SE Sustaining team, SPB04 * Give Rabbit time and he'll always get the answer ...
Re: Suggestion of combining some macros of processing solaris, macosx with other UNIX
Chris, Sorry for being later at the party. 1. Could you explain why you are calling both mcast_set_if_by_addr_v4 mcast_set_if_by_addr_v6 for Linux and only mcast_set_if_by_addr_v6 for other OS-es. 2. If it's really necessary did you consider of writing special variant of mcast_set_if_by_addr_v6 combined both functionality for linux to reduce number of JNI (FindClass) calls and macros inside code ? -Dmitry On 2012-08-16 13:21, Frank Ding wrote: > On 8/14/2012 10:42 PM, Chris Hegarty wrote: >> On 14/08/12 13:11, Alan Bateman wrote: >>> On 14/08/2012 04:11, Frank Ding wrote: >>>> On 8/7/2012 1:46 PM, Frank Ding wrote: >>>>> : >>>>> >>>>> Could anybody take a look at my patch below and make comment? >>>>> http://cr.openjdk.java.net/~youdwei/ojdk-533/webrev.00/ >>>>> >>>>> Thanks & Best regards, >>>>> Frank >>>> >>>> Hi all, >>>> Is there anybody who is interested in the patch and who can take a >>>> look and comment? >>> It looks okay to me but I don't have time at the moment to sponsor it. >>> Can you confirm that you've run the tests with this change? >> >> I filed 7191275: "Cleanup OS specific vinblocks in >> PlainDatagramSocketImpl.c to support more unix-like platforms",for >> this issue. >> >> I can sponsor this patch and help get it in. Can answer Alan's >> question about testing? And confirm that it builds on all platforms? >> >> Thanks, >> -Chris. >> >>> >>> -Alan >> > Hi Chris and Alan, > Thank you for taking time to help this issue. I have built using > latest openjdk 8 repo on Windows 64 and Linux 32/64. Since it's a macro > change in path "src/solaris", I only did jtreg tests for Linux 32 and 64 > build. The jtreg tests I ran are restricted to package "java/net". > Please let me know if you need me to do more tests or on more platforms > (such as Solaris). > > Best regards, > Frank > -- Dmitry Samersoff Java Hotspot development team, SPB04 * There will come soft rains ...
Re: Suggestion of combining some macros of processing solaris, macosx with other UNIX
Chris, Yes, I'm asking more general question - it's why I address it to you but Frank. IMHO, logic cleanup would take almost the same time as just reformatting macros (in all cases testing is required) and create almost the same disturbance for people porting openjdk to other platforms. So (on my opinion) it's better to do it once. Also do we really supports linux 2.2 kernel in jdk8? I think not, and all related macros should be removed within a scope of any bug touching the file. -Dmitry On 2012-08-16 16:23, Chris Hegarty wrote: > You already know this, but to be clear, Frank's changes are simply > changing the definitions so that platforms like AIX will use the same > implementation as Solaris, Mac, etc. There will be no behavior changes > from this. You're just asking general questions, right? > > On 16/08/2012 12:25, Dmitry Samersoff wrote: >> Chris, >> >> Sorry for being later at the party. >> >> 1. >> >> Could you explain why you are calling both >> >> mcast_set_if_by_addr_v4 >> mcast_set_if_by_addr_v6 >> >> for Linux >> >> and only >> >> mcast_set_if_by_addr_v6 >> >> for other OS-es. > > There is a lot of history here and many bugs over the years. I'll try to > summarize. > > It was found that the dual stack on Linux requires that both the IPv4 > and IPv6 socket options (IP_MULTICAST_IF & IPV6_MULTICAST_IF) are > required to set the outgoing interface for sending multicast packets. > Similarly for other sockets options. > > This was introduced under CR 4742177 [1]. From 4742177: > "So with regard to ipv4 and ipv6 socket option, Solaris behaves this >way :- > - ipv4 options can be set for ipv4 socket fd, and ipv6 options for >ipv6 socket fd. Otherwise, an error will occur. > - effective options are those belong to the same family of socket >fd. > >Meanwhile, Linux behaves this way :- > - both ipv4 options and ipv6 options can be set for a socket fd. > - effective options are those belong to the same family of >destination address." > >> 2. If it's really necessary >> did you consider of writing special variant of mcast_set_if_by_addr_v6 >> combined both functionality for linux to reduce number of JNI >> (FindClass) calls and macros inside code ? > > Unfortunately, this behavior is necessary. I agree, the code is not all > that pretty, and there is a lot that could be done to clean it up, but I > would like to keep this issue confined to Franks's particular problem. > > -Chris. > > [1] http://bugs.sun.com/view_bug.do?bug_id=4742177 > >> >> >> -Dmitry >> >> >> On 2012-08-16 13:21, Frank Ding wrote: >>> On 8/14/2012 10:42 PM, Chris Hegarty wrote: >>>> On 14/08/12 13:11, Alan Bateman wrote: >>>>> On 14/08/2012 04:11, Frank Ding wrote: >>>>>> On 8/7/2012 1:46 PM, Frank Ding wrote: >>>>>>> : >>>>>>> >>>>>>> Could anybody take a look at my patch below and make comment? >>>>>>> http://cr.openjdk.java.net/~youdwei/ojdk-533/webrev.00/ >>>>>>> >>>>>>> Thanks& Best regards, >>>>>>> Frank >>>>>> >>>>>> Hi all, >>>>>>Is there anybody who is interested in the patch and who can take a >>>>>> look and comment? >>>>> It looks okay to me but I don't have time at the moment to sponsor it. >>>>> Can you confirm that you've run the tests with this change? >>>> >>>> I filed 7191275: "Cleanup OS specific vinblocks in >>>> PlainDatagramSocketImpl.c to support more unix-like platforms",for >>>> this issue. >>>> >>>> I can sponsor this patch and help get it in. Can answer Alan's >>>> question about testing? And confirm that it builds on all platforms? >>>> >>>> Thanks, >>>> -Chris. >>>> >>>>> >>>>> -Alan >>>> >>> Hi Chris and Alan, >>>Thank you for taking time to help this issue. I have built using >>> latest openjdk 8 repo on Windows 64 and Linux 32/64. Since it's a macro >>> change in path "src/solaris", I only did jtreg tests for Linux 32 and 64 >>> build. The jtreg tests I ran are restricted to package "java/net". >>> Please let me know if you need me to do more tests or on more platforms >>> (such as Solaris). >>> >>> Best regards, >>> Frank >>> >> >> -- Dmitry Samersoff Java Hotspot development team, SPB04 * There will come soft rains ...
hg: jdk8/tl/jdk: 7194597: Typeo in com.sun.management.VMOption.toString()
Changeset: 2598dad9 Author:dsamersoff Date: 2012-09-11 19:58 +0400 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/2598dad9 7194597: Typeo in com.sun.management.VMOption.toString() Summary: VMOption.toString() returns "...(read-only)" if writable "...(read-write)" otherwise. Reviewed-by: alanb, mchung Contributed-by: dmytro_she...@hotmail.com ! src/share/classes/com/sun/management/VMOption.java
Re: Indenting code?
Jim, IMHO, It's really boring task to format big peace of code manually. The worst scenario I could imagine - autoformat code by netbeans and then manually adjust it to some coding standard. Is it possible to: 1. Rich some common point between netbeans and old sun coding standard. 2. Provide a config for one of available opensource beautifiers (jalopy, uncrustify etc.) for people who don't use netbeans. 3. Write results as coding standard. -Dmitry On 2012-09-14 21:23, Jim Gish wrote: > While it is true that NB and Eclipse and other IDEs offer auto > formatting and that will suit some us, I also no that there are some > amongst us who still use emacs and vi and possibly other non-IDE > editors. The first thing to agree on is what standard are we coding > to. I had assumed it was the old Sun Java coding standards ( > http://www.oracle.com/technetwork/java/codeconv-138413.html) > > Is that the case? > > If not, I suggest that we /don't /open this up to a full-fledged > discussion of what the standard should be. I've been involved in far > too many such religious debates over the years that end up reminding me > of the famous Belushi-esque food fight scene from Animal House. > Instead, if any question on any one individual point comes up, we look > at the predominate approach in the existing code and use that. > > As Alan points out, local consistency is important to maintain. In the > unlikely event that an entire piece of code is rewritten, then it's ok > to bring it up to the current standard, otherwise don't mess with it. > In other words, there are more important things to consider than whether > any one piece of code meets the standard. Although that would be ideal, > we do have to consider the consequences of major formatting changes, > since those will impact the ease of interpreting diffs, and far more > significant, ability to manage merging. > > If we agree that the old Sun Java coding standards are what we /are > mostly/ using, then we can identify formatting templates for the major > IDEs, and other tools as needed. > > Jim > > Also, this is broader than net-dev, so I'm moving the discussion to > disc...@openjdk.java.net. Please respond there. > On 09/14/2012 12:27 PM, Chris Hegarty wrote: >> On 14/09/12 12:20, Alan Bateman wrote: >>> On 14/09/2012 01:21, Brad Wetmore wrote: >>>> Netbean's automatic formatting does a pretty good job with new code. >>>> However, I think the general advice is to not change existing code >>>> just because. When you're dealing with multiple release families, it >>>> makes the merges much more difficult. >>>> >>>> Brad >>> One think that Paul Sandoz suggested recently is that we should have a >>> NB template that folks can use to avoid some discussions/debates on >>> styles. It would be great for someone to run with that, the hard part is >>> of course that it will be impossible to get agreement. Personally I find >>> NB's defaults okay but there are several cases where its indenting is >>> horrible. >> >> I did play with NB somewhat trying to get it follow, exactly, the >> preferred style in some areas of the JDK code. I was able to get it >> close, or at least better than the default, but I don't believe it is >> possible to get it to do exactly what we want. >> >> -Chris. >> >>> Anyway, the main advice I think is to keep things locally consistent >>> where possible. Also major refactoring or formatting in a bug fix is a >>> royal pain for reviewers. >>> >>> -Alan > > -- > Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304 > Oracle Java Platform Group | Core Libraries Team > 35 Network Drive > Burlington, MA 01803 > jim.g...@oracle.com > -- Dmitry Samersoff Java Hotspot development team, SPB04 * There will come soft rains ...
Re: Proposed changes for Bug 7193520
John, Changes look good for me. Few nits below. PlainDatagramSocketImpl.c 318 brackets is not necessary anymore 1644 whole #ifdef could be removed struct ip_mreqn mreqn; is not necessary anymore, 2283 the same 2294 #ifdef is not necessary anymore -Dmitry On 2012-09-14 22:22, John Zavgren wrote: > Greetings: > > > This bug (7193520) was filed because there are obsolete checks in the openJDK > native code that implements datagram sockets, e.g., > src/solaris/native/java/net/PlainDatagramSocketImpl.c > and it's Java counterpart: > src/share/classes/java/net/AbstractPlainDatagramSocketImpl.java > > The native code (PlainDatagramSocketImpl.c and "friends") runs real time > checks for the Linux kernel version number. If the most significant two > fields of the version number is "2.2" on the host platform, then these checks > cause the socket to be created, used, and managed differently than if the > Linux kernel version were newer. (These behavior changes were necessary > because Linux kernel 2.2.X > IP networking was was implemented differently and lacked features of the > newer kernels.) > > However, the run time logic isn't actually needed anymore because openJDK > doesn't support Linux kernel 2.2.X, and consequently one cannot run openJDK > on these older OSes. The run time checks are never used. > > The proposed changes to the code > (http://cr.openjdk.java.net/~chegar/7193520/webrev.00/) eliminate distracting > dead wood, and it makes it run (slightly) faster, because the run time checks > are eliminated. > > > Thanks! > and > RSVP > John Zavgren > john.zavg...@oracle.com > > > > -- Dmitry Samersoff Java Hotspot development team, SPB04 * There will come soft rains ...
Re: Proposed changes for Bug 7193520
John, linux has both ip_mreq and ip_mreqn structs, but after better consideration, I think it's better to *leave this code as is* because ip_mreq is obsoleted and general direction should be to use ip_mreqn for all OS that have it but these changes clearly out of scope of this CR, -Dmitry On 2012-09-15 00:06, John Zavgren wrote: > I agree with all the changes you recommend except the last one... see below. > - Original Message - > From: dmitry.samers...@oracle.com > To: john.zavg...@oracle.com > Cc: net-dev@openjdk.java.net > Sent: Friday, September 14, 2012 3:08:57 PM GMT -05:00 US/Canada Eastern > Subject: Re: Proposed changes for Bug 7193520 > > John, > > Changes look good for me. > Few nits below. > > > PlainDatagramSocketImpl.c > > > 318 brackets is not necessary anymore > > 1644 whole #ifdef could be removed > struct ip_mreqn mreqn; > is not necessary anymore, > > 2283 the same > > 2294 #ifdef is not necessary anymore > --- This is the original code near line number 2294 > #ifdef __linux__ > mname.imr_address.s_addr = > (isOldKernel ? mreqn.imr_address.s_addr : in.s_addr); > > #else > mname.imr_interface.s_addr = in.s_addr; > #endif > --- > When Linux is the OS, the structure field name to be set is "imr_address", > whereas when other OSes are used, the field name is: imr_interface. > Am I understanding your suggestion correctly? > > Thanks! > John > > > -Dmitry > > > > On 2012-09-14 22:22, John Zavgren wrote: >> Greetings: >> >> >> This bug (7193520) was filed because there are obsolete checks in the >> openJDK native code that implements datagram sockets, e.g., >> src/solaris/native/java/net/PlainDatagramSocketImpl.c >> and it's Java counterpart: >> src/share/classes/java/net/AbstractPlainDatagramSocketImpl.java >> >> The native code (PlainDatagramSocketImpl.c and "friends") runs real time >> checks for the Linux kernel version number. If the most significant two >> fields of the version number is "2.2" on the host platform, then these >> checks cause the socket to be created, used, and managed differently than if >> the Linux kernel version were newer. (These behavior changes were necessary >> because Linux kernel 2.2.X >> IP networking was was implemented differently and lacked features of the >> newer kernels.) >> >> However, the run time logic isn't actually needed anymore because openJDK >> doesn't support Linux kernel 2.2.X, and consequently one cannot run openJDK >> on these older OSes. The run time checks are never used. >> >> The proposed changes to the code >> (http://cr.openjdk.java.net/~chegar/7193520/webrev.00/) eliminate >> distracting dead wood, and it makes it run (slightly) faster, because the >> run time checks are eliminated. >> >> >> Thanks! >> and >> RSVP >> John Zavgren >> john.zavg...@oracle.com >> >> >> >> > > -- Dmitry Samersoff Java Hotspot development team, SPB04 * There will come soft rains ...
Re: Indenting code?
Paul, On 2012-09-17 17:52, Paul Sandoz wrote: > The solution to that is to re-format all JDK source code using a tool > and commit independent of any fixes, i bet the NBs formatter could be > extracted for command line operation. Netbeans formatter is OK to do simple job why editing, but it couldn't be used as a standalone tool. There are plenty of separate tools, so someone have to invest time to evaluate some of these tools, choose one and write config that mimic netbeans settings. > FWIW i really don't care about what the style would be as long as it > is not equivalent to a Huggy Bear sofa so garish as to require one to > wear peril sensitive sunglasses. Personally, I also don't think that the coding style is important. It's important to have the style common one over all code - which one doesn't really matter. -Dmitry -- Dmitry Samersoff Java Hotspot development team, SPB04 * There will come soft rains ...
Re: Two Review requests
John,
> File Descriptor Leak:
> http://cr.openjdk.java.net/~chegar/8000203/webrev.00/
> (Jira bug ID number: 8000203)
(*it's not to your changes but as far as you touch this code *)
607 else is not needed here
609 realloc should not touch original pointer in case of fail,
so this code leads to hidden memmory leak
its better to do:
newLoRoutes = realloc(
if (newLoRoutes == NULL){
// What you do here depends to whether you
// plan to keep incomplete table or not.
free(loRoutes);
break;
}
-Dmitry
On 2012-09-28 18:11, John Zavgren wrote:
> Greetings:
>
> I just posted the webrev images for two networking code bugs:
>
> File Descriptor Leak:
> http://cr.openjdk.java.net/~chegar/8000203/webrev.00/
> (Jira bug ID number: 8000203)
>
> Uninitialized memory:
> http://cr.openjdk.java.net/~chegar/8000206/webrev.00/
> (Jira bug ID: 8000206)
> This change doesn't actually fix a bug... the original code initialized
> "optlen" before it was referenced, however, parfait (static code analysis)
> believes "optlen" MAY be used before initialization. I added the assignment
> statement to "spoof" parfait, and it no longer flags a bug. I assume it's
> better to put minor harmless tweaks in our code than to add state information
> to parfait, that would cause it to ignore certain "situations". That option
> seems complicated and dangerous.
>
> Thanks!
> John Zavgren
> john.zavg...@oracle.com
>
--
Dmitry Samersoff
Java Hotspot development team, SPB04
* There will come soft rains ...
hg: jdk8/tl/jdk: 7186723: TEST_BUG: Race condition in sun/management/jmxremote/startstop/JMXStartStopTest.sh
Changeset: 0c1c4b185451 Author:dsamersoff Date: 2012-09-29 15:44 +0400 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/0c1c4b185451 7186723: TEST_BUG: Race condition in sun/management/jmxremote/startstop/JMXStartStopTest.sh Summary: Make test self-terminating and independent to cygwin/mks kill behaviour Reviewed-by: sspitsyn, alanb ! test/ProblemList.txt ! test/sun/management/jmxremote/startstop/JMXStartStopDoSomething.java ! test/sun/management/jmxremote/startstop/JMXStartStopTest.java ! test/sun/management/jmxremote/startstop/JMXStartStopTest.sh ! test/sun/management/jmxremote/startstop/REMOTE_TESTING.txt
Re: URL guarantees that the ftp protocol handler is present, worth re-considering this?
Alan, Looks ok for me. -Dmitry On 2012-10-15 20:14, Alan Bateman wrote: > > java.net.URL guarantees that 5 protocol handlers are present: http, > https, ftp, file and jar. > > ftp a legacy protocol and I'm wondering whether it's time to consider > removing it from the list. I'm not suggesting we don't continue to > include it, rather just removing the guarantee that it is always > present. The motivation for bringing this up is modules and compact > profiles where it might be desirable to not include the ftp protocol > handler for footprint reasons. > > -Alan -- Dmitry Samersoff Java Hotspot development team, SPB04 * There will come soft rains ...
hg: jdk8/tl/jdk: 6809322: javax.management.timer.Timer does not fire all notifications
Changeset: b2d8a99a049e Author:dsamersoff Date: 2012-10-17 18:34 +0400 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/b2d8a99a049e 6809322: javax.management.timer.Timer does not fire all notifications Summary: Some notifications get dropped due to ConcurrentModificationException thrown in Timer.notifyAlarmClock() method. Reviewed-by: dholmes, rbackman Contributed-by: jaroslav.bacho...@oracle.com ! src/share/classes/javax/management/timer/Timer.java + test/javax/management/timer/MissingNotificationTest.java
Re: Change to: ./src/solaris/native/java/net/PlainDatagramSocketImpl.c
John, Looks good for me. -Dmitry On 2012-10-19 00:59, John Zavgren wrote: > Greetings: > I'm proposing the following change: > > http://cr.openjdk.java.net/~khazra/john/8000206/webrev.00/ > > because it simplifies the code by eliminating an unnecessary union data > structure. This change eliminates > a false positive from our static code analyzer: parfait. This modification > doesn't change any of the externally-visible behavior in the procedure. > > I look forward to your comments and suggestions. > > Thanks! > John Zavgren > john.zavg...@oracle.com > -- Dmitry Samersoff Java Hotspot development team, SPB04 * There will come soft rains ...
Re: RFR 8000203: file descriptor leak, src/solaris/native/java/net/net_util_md.c ... AND a potential realloc()-related memory leak.
John, Looks good for me! -Dmitry On 2012-10-24 17:16, John Zavgren wrote: > > Greetings: > > I'm requesting a review of a software change that fixes a file descriptor > leak AND a potential memory leak that involves memory reallocation > (realloc()). The webrev image is in the following location: > > http://cr.openjdk.java.net/~chegar/8000203/webrev.01/ > > Thanks! > John Zavgren > john.zavg...@oracle.com > -- Dmitry Samersoff Java Hotspot development team, SPB04 * There will come soft rains ...
Re: RFR 8000203: file descriptor leak, src/solaris/native/java/net/net_util_md.c ... AND a potential realloc()-related memory leak.
Kurchi. On 2012-10-24 21:31, Kurchi Hazra wrote: > Just for the sake of understanding the fix better, loRoutesTemp will > point to 0 if the realloc() request fails, > and we still need a reference to the older allocated memory (loRoutes in > this case) in order to free it. Hence > the need for a temporary variable here? Correct. realloc doesn't modify original pointer in case of file. -Dmitry > > - Kurchi > > > On 24.10.2012 06:27, Chris Hegarty wrote: >> Looks good to me. Thanks for going the extra mile here. >> >> -Chris. >> >> On 24/10/2012 14:16, John Zavgren wrote: >>> >>> Greetings: >>> >>> I'm requesting a review of a software change that fixes a file >>> descriptor leak AND a potential memory leak that involves memory >>> reallocation (realloc()). The webrev image is in the following location: >>> >>> http://cr.openjdk.java.net/~chegar/8000203/webrev.01/ >>> >>> Thanks! >>> John Zavgren >>> john.zavg...@oracle.com > -- Dmitry Samersoff Java Hotspot development team, SPB04 * There will come soft rains ...
Re: RFR 8002297: sun/net/www/protocol/http/StackTraceTest.java fails intermittently
Chris, If you need definitely refusing port the best option is use port 0 reserved by IANA for this purpose. Other option is use port 1, it reserved for tcp multiplexor service and nowdays is always closed for obvious security reason. -Dmitry On 2012-11-06 15:08, Chris Hegarty wrote: > Trivial test issue where the test will fail if run on a machine that has > a process listening on port 8080 ( test expects this port to refuse the > connection ). The solution is to simply open and close a listening > socket and use its ephemeral port. Should be good enough. > > http://cr.openjdk.java.net/~chegar/8002297/webrev.00/webrev/ > > -Chris -- Dmitry Samersoff Java development team, SPB04 * There will come soft rains ...
Re: RFR 8002297: sun/net/www/protocol/http/StackTraceTest.java fails intermittently
On 2012-11-06 15:41, Chris Hegarty wrote: > On 11/06/2012 11:30 AM, Dmitry Samersoff wrote: >> Chris, >> >> If you need definitely refusing port the best option is use port 0 >> reserved by IANA for this purpose. > > Do you have a link specifying this? I don't see this behavior on my > Solaris box. http://www.iana.org/assignments/service-names-port-numbers/service-names-port-numbers.xml 0 marked as reserved. -Dmitry > > -Chris. > >> >> Other option is use port 1, it reserved for tcp multiplexor service and >> nowdays is always closed for obvious security reason. >> >> -Dmitry >> >> >> On 2012-11-06 15:08, Chris Hegarty wrote: >>> Trivial test issue where the test will fail if run on a machine that has >>> a process listening on port 8080 ( test expects this port to refuse the >>> connection ). The solution is to simply open and close a listening >>> socket and use its ephemeral port. Should be good enough. >>> >>> http://cr.openjdk.java.net/~chegar/8002297/webrev.00/webrev/ >>> >>> -Chris >> >> -- Dmitry Samersoff Java development team, SPB04 * There will come soft rains ...
Re: RFR 8002297: sun/net/www/protocol/http/StackTraceTest.java fails intermittently
Chris, On 2012-11-06 15:50, Chris Hegarty wrote: > If it's ok with you I'd like to keep > what is in the webrev. It should be good enough for this test. Yes sure. -Dmitry > > -Chris. > >> >> -Dmitry >> >> >>> >>> -Chris. >>> >>>> >>>> Other option is use port 1, it reserved for tcp multiplexor service and >>>> nowdays is always closed for obvious security reason. >>>> >>>> -Dmitry >>>> >>>> >>>> On 2012-11-06 15:08, Chris Hegarty wrote: >>>>> Trivial test issue where the test will fail if run on a machine >>>>> that has >>>>> a process listening on port 8080 ( test expects this port to refuse >>>>> the >>>>> connection ). The solution is to simply open and close a listening >>>>> socket and use its ephemeral port. Should be good enough. >>>>> >>>>> http://cr.openjdk.java.net/~chegar/8002297/webrev.00/webrev/ >>>>> >>>>> -Chris >>>> >>>> >> >> -- Dmitry Samersoff Java development team, SPB04 * There will come soft rains ...
Re: Request for code review 6512101: Incorrect encoding in NetworkInterface.getDisplayName()
Frank, Changes look good for me. But it might be better to fall back to original behavior if MultiByteToWideChar return error, rather than abort. -Dmitry On 2012-11-07 13:08, Frank Ding wrote: > Hi guys, > Could you please take a look at patch below aimed to resolve existing > bug http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6512101 ? > http://cr.openjdk.java.net/~dingxmin/6512101/webrev.01/ > > I happen to have a Chinese Win 7 environment. "buggy.png" is current > output of test case described in bug system whereas "fixed.png" is the > output after the my patch is applied. > > http://cr.openjdk.java.net/~dingxmin/6512101/buggy.png > http://cr.openjdk.java.net/~dingxmin/6512101/fixed.png > > The patch simply converts to wide chars encoded in CP_OEMCP by calling > MultiByteToWideChar. We have confirmed a guy from Microsoft who said that > > BEGIN QUOTE > I'm not sure how common it is to call the Java code that results in > calling the GetIfTable API but I would guess it does not happen that > often. Additionally, if it's rare that the adapter contains the accented > characters, it would definitely be quite easy to miss in testing. > > I have not found any documentation about the encoding of the bDescr > string unfortunately. I did, however, debug through the API and located > the place where it is generated. It is getting converted from a UTF-16 > string to a single-byte string using a conversion like this: > > WideCharToMultiByte( > CP_OEMCP, > WC_NO_BEST_FIT_CHARS, > , > -1, > IfRow->bDescr, > , > NULL, > NULL); > > I have checked the source for Windows Vista, 2008, Windows 7, and > Windows 2008 R2. It is using CP_OEMCP in all of them. So using the > reverse conversion in your code using CP_OEMCP should be safe. > Alternatively, you can use the GetIfTable2 function > (http://msdn.microsoft.com/en-us/library/windows/desktop/aa365945^(v=vs.85).aspx > ) which returns the same information in the original UTF-16 encoding. > END QUOTE > > The link below may be helpful to the second param of > WideCharToMultiByte. > http://blogs.msdn.com/b/oldnewthing/archive/2012/05/04/10300670.aspx > > You comments are appreciated. > Best regards, > Frank > -- Dmitry Samersoff Saint Petersburg, Russia, http://devnull.samersoff.net * There will come soft rains ...
Re: getportbyname in Java?
John,
The goal of getportbyname() is transfer responsibility of finding right
ports for running service from software developer to system
administrator, and especially useful if we have one /etc/services for
whole network (e.g. distributed by NIS)
e.g instead of constantly fixing port numbers in jdk tests we can
a) require QA to have a name "test_program_port" defined on
all machines
b) Use getportbyname() to get appropriate port.
or
if we have kerberos running on non-standard port, in ideal world we can
just change /etc/services:kerberos entry out from 88 instead of fixing
zillion client configs.
So I'm surprised we don't have this API in java.
-Dmitry
On 2012-11-14 07:08, John Zavgren wrote:
> Max:
> I've never seen a procedure that binds a protocol to a port number. (The http
> protocol you mention usually uses TCP port number 80, but the port choice for
> this protocol is up to the discretion of the programmer. The file
> /etc/services binds names to numbers so that tools like tcpdump and netstat
> can convert port numbers to names and that can be useful sometimes. But, you
> can violate these conventions without any consequences. If you use TCP port
> number 79 to carry http traffic, my netstat program will think it's the
> finger protocol. You can event set the port number that an http server uses
> for receiving connections if you want. It's completely legal to run a WWW
> server on, say, TCP port 666.)
>
> On the other hand, there are procedures for getting an IP protocol by name...
> they will convert the character string "UDP" into a structure called a
> protoent i.e., getprotobyname("UDP"); The protoent structure in this example
> will contain the number 17 in host byte order as the member p_proto. But,
> that's a different kind of protocol than what you are considering. The
> bindings between IP protocol names and numbers are "written in stone",
> because every OS (windows, Mac, Linux, etc.) needs to know which protocol
> handler to invoke whenever an IP datagram arrives. If this wasn't true then
> interoperability would suffer. Maybe this is why getprotobyname() exists in
> the C runtime libraries? I've never used it. And it doesn't seem necessary.
>
> Have you considered using an Enum? That could bind the string "http" to the
> port number 80.
> John
>
> - Original Message -
> From: kurchi.subhra.ha...@oracle.com
> To: weijun.w...@oracle.com
> Cc: net-dev@openjdk.java.net
> Sent: Tuesday, November 13, 2012 9:16:28 PM GMT -05:00 US/Canada Eastern
> Subject: Re: getportbyname in Java?
>
> I don't think so...
>
> Thanks,
> - Kurchi
>
> On 13.11.2012 16:40, Weijun Wang wrote:
>> Is there a Java API I can translate "http" to 80?
>>
>> Thanks
>> Max
>
--
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* Give Rabbit time, and he'll always get the answer
Re: RFR8003991 (take two)
John, I guess free(loRoutes) should be here. -Dmitry On 2012-11-27 03:19, John Zavgren wrote: > Greetings: > > I'm posting the webrev image of a change that I made that eliminates a > potential file descriptor leak. (The previous RFR referenced a webrev image > with the right file name but the wrong content. 'sorry about that.) > > http://cr.openjdk.java.net/~mullan/webrevs/8003991/webrev.00/ > > Thanks! > John Zavgren > -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * Give Rabbit time, and he'll always get the answer
Re: RFR 8003833: Spurious NPE from Socket.getIn/OutputStream
Looks good for me. -Dmitry On 2012-11-27 15:44, Chris Hegarty wrote: > This is a longstanding bug in the Socket code that was only noticed > recently as a result of some test changes that Daniel pushed in the nio > area. > > There is a very small window in > AbstractPlainSocketImpl.getIn/OutputStream where isClosedOrPending() > grabs the fdLock to check if the socket is closed, or not, and the > construction of the in/output stream, where another thread may > asynchronously close the socket. > > http://cr.openjdk.java.net/~chegar/8003833/webrev.00/webrev/ > > -Chris. -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * Give Rabbit time, and he'll always get the answer
Re: Code review: 7200720: crash in net.dll during NTLM authentication
Michael, NTLMAuthentication.java: 1. Copyright year should be either fixed or not changed 2. I think it's better to remove bug id from comments and replace comment like // MMM 7200720 ?? to something better. NTLMAuthSequence.c 1. Copyright ... Otherwise looks good for me. -Dmitry On 2012-11-28 17:47, Michael McMahon wrote: > On 23/11/12 16:16, Alan Bateman wrote: >> On 23/11/2012 15:59, Michael McMahon wrote: >>> Could I get the following change reviewed please? >>> There was a crash caused by accessing freed memory when >>> authentication was repeated in the same HttpURLConnection instance/ >>> >>> http://cr.openjdk.java.net/~michaelm/7200720/webrev.1/ >> I haven't reviewed the changes but I notice this changes the build >> slightly so I just wonder if you've checked the new build. >> >> -Alan > > just checked the new build on Linux and it's fine. > There are a couple of other fixes that will follow this one > related to this problem. But, I'd like to go ahead and push this one first > if possible > > - Michael -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * Give Rabbit time, and he'll always get the answer
Re: Request for code review 6512101: Incorrect encoding in NetworkInterface.getDisplayName()
Frank, Looks good for me. May be "Can't get WIDE string" should be changed to something more verbose. -Dmitry On 2012-12-10 11:40, Frank Ding wrote: > Hi Dmitry and Chris, > Could you please review the second revision again? > > Thanks and Best regards, > Frank > On 11/29/2012 1:08 PM, Frank Ding wrote: >> >> Hi Dmitry and Chris, >> Thanks for your comments. With your comments incorporated, I've >> prepared v2 @ >> http://cr.openjdk.java.net/~dingxmin/6512101/webrev.02/. Could you >> please review it again? >> >> Best regards, >> Frank >> >> On 11/14/2012 12:12 AM, Chris Hegarty wrote: >>> On 11/11/2012 07:03 PM, Dmitry Samersoff wrote: >>>> Frank, >>>> >>>> Changes look good for me. >>> >>> I admit that I am not an expert in this area, but given the >>> information you provided, and I guess you verified this in your >>> environment, the conversion would appear reasonable. >>> >>>> But it might be better to fall back to original behavior if >>>> MultiByteToWideChar return error, rather than abort. >>> >>> I agree with Dmitry, fall back would be preferable. Can you make the >>> changes and post an updated webrev. >>> >>> -Chris. >>> >>>> >>>> -Dmitry >>>> >>>> On 2012-11-07 13:08, Frank Ding wrote: >>>>> Hi guys, >>>>>Could you please take a look at patch below aimed to resolve >>>>> existing >>>>> bug http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6512101 ? >>>>>http://cr.openjdk.java.net/~dingxmin/6512101/webrev.01/ >>>>> >>>>>I happen to have a Chinese Win 7 environment. "buggy.png" is >>>>> current >>>>> output of test case described in bug system whereas "fixed.png" is the >>>>> output after the my patch is applied. >>>>> >>>>> http://cr.openjdk.java.net/~dingxmin/6512101/buggy.png >>>>> http://cr.openjdk.java.net/~dingxmin/6512101/fixed.png >>>>> >>>>>The patch simply converts to wide chars encoded in CP_OEMCP by >>>>> calling >>>>> MultiByteToWideChar. We have confirmed a guy from Microsoft who >>>>> said that >>>>> >>>>> BEGIN QUOTE >>>>> I'm not sure how common it is to call the Java code that results in >>>>> calling the GetIfTable API but I would guess it does not happen that >>>>> often. Additionally, if it's rare that the adapter contains the >>>>> accented >>>>> characters, it would definitely be quite easy to miss in testing. >>>>> >>>>> I have not found any documentation about the encoding of the bDescr >>>>> string unfortunately. I did, however, debug through the API and >>>>> located >>>>> the place where it is generated. It is getting converted from a UTF-16 >>>>> string to a single-byte string using a conversion like this: >>>>> >>>>> WideCharToMultiByte( >>>>> CP_OEMCP, >>>>> WC_NO_BEST_FIT_CHARS, >>>>> , >>>>> -1, >>>>> IfRow->bDescr, >>>>> , >>>>> NULL, >>>>> NULL); >>>>> >>>>> I have checked the source for Windows Vista, 2008, Windows 7, and >>>>> Windows 2008 R2. It is using CP_OEMCP in all of them. So using the >>>>> reverse conversion in your code using CP_OEMCP should be safe. >>>>> Alternatively, you can use the GetIfTable2 function >>>>> (http://msdn.microsoft.com/en-us/library/windows/desktop/aa365945^(v=vs.85).aspx >>>>> >>>>> ) which returns the same information in the original UTF-16 encoding. >>>>> END QUOTE >>>>> >>>>>The link below may be helpful to the second param of >>>>> WideCharToMultiByte. >>>>> http://blogs.msdn.com/b/oldnewthing/archive/2012/05/04/10300670.aspx >>>>> >>>>> You comments are appreciated. >>>>> Best regards, >>>>> Frank >>>>> >>>> >>>> >>> >> > -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * Give Rabbit time, and he'll always get the answer
Re: Request for code review 6512101: Incorrect encoding in NetworkInterface.getDisplayName()
Frank, Excellent! Thank you for doing it. -Dmitry On 2012-12-10 12:00, Frank Ding wrote: > Hi Dmitry, > I updated wording accordingly @ > http://cr.openjdk.java.net/~dingxmin/6512101/webrev.03. It is now > changed to "Cannot get multibyte char for interface display name". What > about that? > > Best regards, > Frank > > On 12/10/2012 3:43 PM, Dmitry Samersoff wrote: >> Frank, >> >> Looks good for me. >> >> May be "Can't get WIDE string" should be changed to something more >> verbose. >> >> -Dmitry >> >> >> On 2012-12-10 11:40, Frank Ding wrote: >>> Hi Dmitry and Chris, >>>Could you please review the second revision again? >>> >>> Thanks and Best regards, >>> Frank >>> On 11/29/2012 1:08 PM, Frank Ding wrote: >>>> Hi Dmitry and Chris, >>>>Thanks for your comments. With your comments incorporated, I've >>>> prepared v2 @ >>>> http://cr.openjdk.java.net/~dingxmin/6512101/webrev.02/. Could you >>>> please review it again? >>>> >>>> Best regards, >>>> Frank >>>> >>>> On 11/14/2012 12:12 AM, Chris Hegarty wrote: >>>>> On 11/11/2012 07:03 PM, Dmitry Samersoff wrote: >>>>>> Frank, >>>>>> >>>>>> Changes look good for me. >>>>> I admit that I am not an expert in this area, but given the >>>>> information you provided, and I guess you verified this in your >>>>> environment, the conversion would appear reasonable. >>>>> >>>>>> But it might be better to fall back to original behavior if >>>>>> MultiByteToWideChar return error, rather than abort. >>>>> I agree with Dmitry, fall back would be preferable. Can you make the >>>>> changes and post an updated webrev. >>>>> >>>>> -Chris. >>>>> >>>>>> -Dmitry >>>>>> >>>>>> On 2012-11-07 13:08, Frank Ding wrote: >>>>>>> Hi guys, >>>>>>> Could you please take a look at patch below aimed to resolve >>>>>>> existing >>>>>>> bug http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6512101 ? >>>>>>> http://cr.openjdk.java.net/~dingxmin/6512101/webrev.01/ >>>>>>> >>>>>>> I happen to have a Chinese Win 7 environment. "buggy.png" is >>>>>>> current >>>>>>> output of test case described in bug system whereas "fixed.png" >>>>>>> is the >>>>>>> output after the my patch is applied. >>>>>>> >>>>>>> http://cr.openjdk.java.net/~dingxmin/6512101/buggy.png >>>>>>> http://cr.openjdk.java.net/~dingxmin/6512101/fixed.png >>>>>>> >>>>>>> The patch simply converts to wide chars encoded in CP_OEMCP by >>>>>>> calling >>>>>>> MultiByteToWideChar. We have confirmed a guy from Microsoft who >>>>>>> said that >>>>>>> >>>>>>> BEGIN QUOTE >>>>>>> I'm not sure how common it is to call the Java code that results in >>>>>>> calling the GetIfTable API but I would guess it does not happen that >>>>>>> often. Additionally, if it's rare that the adapter contains the >>>>>>> accented >>>>>>> characters, it would definitely be quite easy to miss in testing. >>>>>>> >>>>>>> I have not found any documentation about the encoding of the bDescr >>>>>>> string unfortunately. I did, however, debug through the API and >>>>>>> located >>>>>>> the place where it is generated. It is getting converted from a >>>>>>> UTF-16 >>>>>>> string to a single-byte string using a conversion like this: >>>>>>> >>>>>>> WideCharToMultiByte( >>>>>>> CP_OEMCP, >>>>>>> WC_NO_BEST_FIT_CHARS, >>>>>>> , >>>>>>> -1, >>>>>>> IfRow->bDescr, >>>>>>> , >>>>>>> NULL, >>>>>>> NULL); >>>>>>> >>>>>>> I have checked the source for Windows Vista, 2008, Windows 7, and >>>>>>> Windows 2008 R2. It is using CP_OEMCP in all of them. So using the >>>>>>> reverse conversion in your code using CP_OEMCP should be safe. >>>>>>> Alternatively, you can use the GetIfTable2 function >>>>>>> (http://msdn.microsoft.com/en-us/library/windows/desktop/aa365945^(v=vs.85).aspx >>>>>>> >>>>>>> >>>>>>> ) which returns the same information in the original UTF-16 >>>>>>> encoding. >>>>>>> END QUOTE >>>>>>> >>>>>>> The link below may be helpful to the second param of >>>>>>> WideCharToMultiByte. >>>>>>> http://blogs.msdn.com/b/oldnewthing/archive/2012/05/04/10300670.aspx >>>>>>> >>>>>>> You comments are appreciated. >>>>>>> Best regards, >>>>>>> Frank >>>>>>> >>>>>> >> > -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * Give Rabbit time, and he'll always get the answer
Re: Code review: 8003948 (jdk8)
Michael,
150 It might be better to use regionMatches here
162 Is it intentional to always don't copy keys[0]/value[0] ?
Actually I would prefer to have this code better readable,
something like -
157 if ( ! ){
keys[j]=keys[j];
value[j]=value[j];
++j
}
-Dmitry
On 2012-12-10 18:30, Michael McMahon wrote:
> Could I get the following change reviewed please?
>
> http://cr.openjdk.java.net/~michaelm/8003948/webrev.1/
>
> We need to filter out extraneous authentication headers when doing
> ntlm authentication with MS web servers and proxies.
>
> Thanks
> Michael
--
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* Give Rabbit time, and he'll always get the answer
Re: reviews for 7200720 and 8003948 [7u-dev]
Michael, On 2012-12-10 23:35, Michael McMahon wrote: > Could I get the following webrevs reviewed please? > They are identical changes (except for one small change suggested by > Dmitry) > to what was done in 8 for the same issues > > http://cr.openjdk.java.net/~michaelm/7200720.7u-dev/webrev.1/ I see a comment // MMM 7200720 ?? at ll: 202 of NTLMAuthentication.java - Does it make sense to change it to something more verbose? Otherwise looks good for me. > http://cr.openjdk.java.net/~michaelm/8003948.7u-dev/webrev.1/ Looks good for me. -Dmitry -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * Give Rabbit time, and he'll always get the answer
Re: RFR 8004675: Inet6Address.getHostAddress should use string scope identifier where available
Chris, Looks good for me. PS: Inet6Address.java: It's not necessary to remove explicit initializations - compiler do it perfectly for you. -Dmitry On 2012-12-10 20:01, Chris Hegarty wrote: > > Inet6Address.getHostAddress() is specified to return the IP address > string in textual presentation, followed by a '%' character and the > scope identifier. This scope identifier can be either a numeric value or > a string, depending on how the instance was created (if it was created > with a scoped interface). > > This change proposes to remove the boolean field, 'scope_ifname_set', > since it is not always correctly set when the instance contains a valid > scoped interface. For example, when iterating over the > NetworkInterface's on the system. 'scope_ifname_set' was never accessed > from native code, so it can simply be removed. > > http://cr.openjdk.java.net/~chegar/8004675/webrev.00/webrev/ > > -Chris. -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * Give Rabbit time, and he'll always get the answer
Re: RFR 8004925: java/net/Socks/SocksV4Test.java failing on all platforms
Chris, According to rfc2606 TLD .invalid is reserved for cases like this one, So, it seems to me domainame.invalid is the best approach. -Dmitry On 2012-12-12 20:15, Chris Hegarty wrote: > > On 12/12/2012 14:14, Alan Bateman wrote: >> >>> -Chris. >> Would it be better if the test SocksServer had a list of knows that it >> always rejects? That might speed up the test too as it would avoid is >> trying to resolve host names or connect to hosts that don't exist. > > The UHE is thrown from the client socket connect(). The Server in this > case doesn't ever receive the destination address or host name. It is > simply replying to the initial/opening SOCKS handshake. > > The updated host name is still brittle ( if a .t TLD is ever registered! > ). I don't have a better alternative. > > -Chris. > >> >> -Alan >> -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * Give Rabbit time, and he'll always get the answer
Re: RFR 8004925: java/net/Socks/SocksV4Test.java failing on all platforms
On 2012-12-12 22:29, Chris Hegarty wrote: > On 12/12/2012 18:15, Dmitry Samersoff wrote: >> Chris, >> >> According to rfc2606 TLD .invalid is reserved for cases like this one, > > Yes, I came across this, but there is nothing to stop an internal DNS > server from resolving .invalid domains. Anyway, may > "doesnot.exist.invalid" would be sufficient. You can't prevent internal DNS from resolving anything without doing some heavy tricks, so I guess doesnot.exist.invalid and error message that clear states that DNS setup violates rfc2606 is sufficient. -Dmitry > > -Chris. > > >> >> So, it seems to me >> >> domainame.invalid >> >> is the best approach. >> >> -Dmitry >> >> >> On 2012-12-12 20:15, Chris Hegarty wrote: >>> >>> On 12/12/2012 14:14, Alan Bateman wrote: >>>> >>>>> -Chris. >>>> Would it be better if the test SocksServer had a list of knows that it >>>> always rejects? That might speed up the test too as it would avoid is >>>> trying to resolve host names or connect to hosts that don't exist. >>> >>> The UHE is thrown from the client socket connect(). The Server in this >>> case doesn't ever receive the destination address or host name. It is >>> simply replying to the initial/opening SOCKS handshake. >>> >>> The updated host name is still brittle ( if a .t TLD is ever registered! >>> ). I don't have a better alternative. >>> >>> -Chris. >>> >>>> >>>> -Alan >>>> >> >> -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * Give Rabbit time, and he'll always get the answer
Re: RFR JDK-8005120
John, Did you consider using socklen_t instead of uint32_t and unsigned int (for namelen etc) ? -Dmitry On 2012-12-19 19:36, John Zavgren wrote: > Greetings: > Please consider the following change to the two files: > src/share/transport/socket/sysSocket.h > src/solaris/transport/socket/socket_md.c > that eliminate compiler warnings that stem from the fact that the variables > that the native code passes to various system calls were not declared > correctly. They were declared as integers, but they must be "unsigned" > integers because they are used to define buffer lengths. Were one to supply a > negative value as an argument, it would be cast into an unsigned "Martian" > value and there'd be (hopefully) a system call error. > > Thanks! > John Zavgren > > http://cr.openjdk.java.net/~mullan/webrevs/jzavgren/8005120/ > -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * Give Rabbit time, and he'll always get the answer
Re: RFR JDK-8005120
John,
On 2012-12-19 22:25, John Zavgren wrote:
> Yes... I did consider that, but I didn't see any POSIX data types near the
> code I was changing, so I decided to use a "brute force" data type instead.
connect, recvfrom etc almost always uses socklen_t, but socklen_t could
expands either to int or to unsigned it on different OS
so as soon as the code pass a parameter directly to one of such
functions like e. g.
52 dbgsysConnect(int fd, struct sockaddr *name, uint32_t namelen) {
53 int rv = connect(fd, name, namelen);
on my opinion, we should use socklen_t
-Dmitry
>
> Shall I make that change?
>
> John
> - Original Message -
> From: dmitry.samers...@oracle.com
> To: john.zavg...@oracle.com
> Cc: net-dev@openjdk.java.net
> Sent: Wednesday, December 19, 2012 1:06:52 PM GMT -05:00 US/Canada Eastern
> Subject: Re: RFR JDK-8005120
>
> John,
>
> Did you consider using socklen_t instead of uint32_t and unsigned int
> (for namelen etc) ?
>
> -Dmitry
>
>
> On 2012-12-19 19:36, John Zavgren wrote:
>> Greetings:
>> Please consider the following change to the two files:
>> src/share/transport/socket/sysSocket.h
>> src/solaris/transport/socket/socket_md.c
>> that eliminate compiler warnings that stem from the fact that the variables
>> that the native code passes to various system calls were not declared
>> correctly. They were declared as integers, but they must be "unsigned"
>> integers because they are used to define buffer lengths. Were one to supply
>> a negative value as an argument, it would be cast into an unsigned "Martian"
>> value and there'd be (hopefully) a system call error.
>>
>> Thanks!
>> John Zavgren
>>
>> http://cr.openjdk.java.net/~mullan/webrevs/jzavgren/8005120/
>>
>
>
--
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* Give Rabbit time, and he'll always get the answer
Re: RFR JDK-8005120
John,
On 2012-12-19 23:33, John Zavgren wrote:
> Dmitry:
> Are you suggesting that in the definition of, for example, dbgsysConnect(),
> we convert the namelen argument to be a socklen_t?
The best approach IMHO, is
dbgsysConnect(int fd, struct sockaddr *name, socklen_t namelen)
and change
share/transport/socket/socketTransport.c:462
err = dbgsysConnect(socketFD, (struct sockaddr *)&sa, sizeof(sa));
to
err = dbgsysConnect(socketFD, (struct sockaddr *)&sa,
(socklen_t) sizeof(sa));
-Dmitry
>
>
> int
> dbgsysConnect(int fd, struct sockaddr *name, uint32_t namelen) {
> int rv = connect(fd, name, (socklen_t)nameLength);<--- NEW
> if (rv < 0 && (errno == EINPROGRESS || errno == EINTR)) {
> return DBG_EINPROGRESS;
> } else {
> return rv;
> }
> }
> Thanks!
> John
> - Original Message -
> From: dmitry.samers...@oracle.com
> To: john.zavg...@oracle.com
> Cc: net-dev@openjdk.java.net
> Sent: Wednesday, December 19, 2012 1:40:11 PM GMT -05:00 US/Canada Eastern
> Subject: Re: RFR JDK-8005120
>
> John,
>
> On 2012-12-19 22:25, John Zavgren wrote:
>> Yes... I did consider that, but I didn't see any POSIX data types near the
>> code I was changing, so I decided to use a "brute force" data type instead.
>
> connect, recvfrom etc almost always uses socklen_t, but socklen_t could
> expands either to int or to unsigned it on different OS
>
> so as soon as the code pass a parameter directly to one of such
> functions like e. g.
>
> 52 dbgsysConnect(int fd, struct sockaddr *name, uint32_t namelen) {
> 53 int rv = connect(fd, name, namelen);
>
> on my opinion, we should use socklen_t
>
> -Dmitry
>
>>
>> Shall I make that change?
>>
>> John
>> - Original Message -
>> From: dmitry.samers...@oracle.com
>> To: john.zavg...@oracle.com
>> Cc: net-dev@openjdk.java.net
>> Sent: Wednesday, December 19, 2012 1:06:52 PM GMT -05:00 US/Canada Eastern
>> Subject: Re: RFR JDK-8005120
>>
>> John,
>>
>> Did you consider using socklen_t instead of uint32_t and unsigned int
>> (for namelen etc) ?
>>
>> -Dmitry
>>
>>
>> On 2012-12-19 19:36, John Zavgren wrote:
>>> Greetings:
>>> Please consider the following change to the two files:
>>> src/share/transport/socket/sysSocket.h
>>> src/solaris/transport/socket/socket_md.c
>>> that eliminate compiler warnings that stem from the fact that the variables
>>> that the native code passes to various system calls were not declared
>>> correctly. They were declared as integers, but they must be "unsigned"
>>> integers because they are used to define buffer lengths. Were one to supply
>>> a negative value as an argument, it would be cast into an unsigned
>>> "Martian" value and there'd be (hopefully) a system call error.
>>>
>>> Thanks!
>>> John Zavgren
>>>
>>> http://cr.openjdk.java.net/~mullan/webrevs/jzavgren/8005120/
>>>
>>
>>
>
>
--
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* Give Rabbit time, and he'll always get the answer
hg: jdk8/tl/jdk: 6783290: MBeanInfo/MBeanFeatureInfo has inconsistent readObject/writeObject
Changeset: b600d490dc57 Author:dsamersoff Date: 2012-12-20 16:02 +0400 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/b600d490dc57 6783290: MBeanInfo/MBeanFeatureInfo has inconsistent readObject/writeObject Summary: call readObject in all cases Reviewed-by: emcmanus Contributed-by: jaroslav.bacho...@oracle.com ! src/share/classes/javax/management/MBeanFeatureInfo.java ! src/share/classes/javax/management/MBeanInfo.java
hg: jdk8/tl/jdk: 6937053: RMI unmarshalling errors in ClientNotifForwarder cause silent failure
Changeset: e43f90d5af11 Author:dsamersoff Date: 2012-12-20 16:56 +0400 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/e43f90d5af11 6937053: RMI unmarshalling errors in ClientNotifForwarder cause silent failure Summary: the catch block in the fetchNotifs() method is extended to expect UnmarshalException Reviewed-by: emcmanus Contributed-by: jaroslav.bacho...@oracle.com ! src/share/classes/com/sun/jmx/remote/internal/ClientNotifForwarder.java
hg: jdk8/tl/jdk: 7009998: JMX synchronization during connection restart is faulty
Changeset: 3f014bc09297 Author:dsamersoff Date: 2012-12-20 17:24 +0400 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/3f014bc09297 7009998: JMX synchronization during connection restart is faulty Summary: add a return statement after the re-connecting has finished and the state is CONNECTED Reviewed-by: sjiang Contributed-by: jaroslav.bacho...@oracle.com ! make/netbeans/jmx/build.properties ! src/share/classes/com/sun/jmx/remote/internal/ClientCommunicatorAdmin.java
Re: RFR JDK-8005120
John, On windows socklen_t defined in WS2TCPIP.H make sure it's included when necessary. -Dmitry On 2012-12-20 17:49, John Zavgren wrote: > Greetings: > > I agree that the "correct" way to fix this problem is to use POSIX data > types, e.g., socklen_t. However, when I switch to the doctrinaire data type, > the build fails on windows machines: > - build monologue - > c:\jprt\t\p1\032220.jzavgren\s\jdk\src\share\transport\socket\sysSocket.h(39) > : error C2146: syntax error : missing ')' before identifier 'len' > c:\jprt\t\p1\032220.jzavgren\s\jdk\src\share\transport\socket\sysSocket.h(39) > : error C2081: 'socklen_t' : name in formal parameter list illegal > c:\jprt\t\p1\032220.jzavgren\s\jdk\src\share\transport\socket\sysSocket.h(39) > : error C2061: syntax error : identifier 'len' > c:\jprt\t\p1\032220.jzavgren\s\jdk\src\share\transport\socket\sysSocket.h(39) > : error C2059: syntax error : ';' > c:\jprt\t\p1\032220.jzavgren\s\jdk\src\share\transport\socket\sysSocket.h(39) > : error C2059: syntax error : ')' > > - build monologue - > > I used alternative types, e.g., uint32_t, etc. as a way to avoid the > limitations of windows. > What is the recommended way to accommodate this windows limitation? Shall I > use a typedef statement to define "socklen_t"? > > Thanks! > > > - Original Message - > From: chris.hega...@oracle.com > To: david.hol...@oracle.com > Cc: alan.bate...@oracle.com, serviceability-...@openjdk.java.net, > john.zavg...@oracle.com, net-dev@openjdk.java.net > Sent: Thursday, December 20, 2012 7:41:07 AM GMT -05:00 US/Canada Eastern > Subject: Re: RFR JDK-8005120 > > On 19/12/2012 20:52, David Holmes wrote: >> Real sense of deja-vu here. Didn't we go through this same thing with >> the HPI socket routines? > > Yes, and the networking native code too. > > I think it is best to use socklen_t for the unix code. From what I can > see making these changes, to use socklen_t, should be relatively localized. > > -Chris. > >> >> Depending on the OS (and version?) we should be using socklen_t not int >> and not uint32_t. >> >> David >> >> On 20/12/2012 2:35 AM, Chris Hegarty wrote: >>> John, >>> >>> I grabbed your patch, and with it I now see different warnings. >>> >>> ../../../../src/share/transport/socket/socketTransport.c: In function >>> 'socketTransport_startListening': >>> ../../../../src/share/transport/socket/socketTransport.c:310:40: >>> warning: pointer targets in passing argument 3 of 'dbgsysGetSocketName' >>> differ in signedness [-Wpointer-sign] >>> ../../../../src/share/transport/socket/sysSocket.h:58:5: note: expected >>> 'uint32_t *' but argument is of type 'int *' >>> ../../../../src/share/transport/socket/socketTransport.c: In function >>> 'socketTransport_accept': >>> ../../../../src/share/transport/socket/socketTransport.c:371:33: >>> warning: pointer targets in passing argument 3 of 'dbgsysAccept' differ >>> in signedness [-Wpointer-sign] >>> ../../../../src/share/transport/socket/sysSocket.h:41:5: note: expected >>> 'uint32_t *' but argument is of type 'int *' >>> >>> Do you see these in your build? >>> >>> -Chris. >>> >>> On 12/19/2012 03:42 PM, Alan Bateman wrote: >>>> >>>> John - this is the debugger socket transport so cc'ing the >>>> serviceability-dev list as that is where this code is maintained. >>>> >>>> On 19/12/2012 15:36, John Zavgren wrote: >>>>> Greetings: >>>>> Please consider the following change to the two files: >>>>> src/share/transport/socket/sysSocket.h >>>>> src/solaris/transport/socket/socket_md.c >>>>> that eliminate compiler warnings that stem from the fact that the >>>>> variables that the native code passes to various system calls were not >>>>> declared correctly. They were declared as integers, but they must be >>>>> "unsigned" integers because they are used to define buffer lengths. >>>>> Were one to supply a negative value as an argument, it would be cast >>>>> into an unsigned "Martian" value and there'd be (hopefully) a system >>>>> call error. >>>>> >>>>> Thanks! >>>>> John Zavgren >>>>> >>>>> http://cr.openjdk.java.net/~mullan/webrevs/jzavgren/8005120/ >>>> -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * Give Rabbit time, and he'll always get the answer
hg: jdk8/tl/jdk: 8005309: Missed tests for 6783290,6937053,7009998
Changeset: c1a55ee9618e Author:dsamersoff Date: 2012-12-20 20:12 +0400 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/c1a55ee9618e 8005309: Missed tests for 6783290,6937053,7009998 Summary: Missed tests for 6783290,6937053,7009998 Reviewed-by: sjiang, emcmanus Contributed-by: jaroslav.bacho...@oracle.com + test/com/sun/jmx/remote/CCAdminReconnectTest.java + test/com/sun/jmx/remote/NotificationMarshalVersions/Client/Client.java + test/com/sun/jmx/remote/NotificationMarshalVersions/Client/ConfigKey.java + test/com/sun/jmx/remote/NotificationMarshalVersions/Client/TestNotification.java + test/com/sun/jmx/remote/NotificationMarshalVersions/Server/ConfigKey.java + test/com/sun/jmx/remote/NotificationMarshalVersions/Server/Server.java + test/com/sun/jmx/remote/NotificationMarshalVersions/Server/Ste.java + test/com/sun/jmx/remote/NotificationMarshalVersions/Server/SteMBean.java + test/com/sun/jmx/remote/NotificationMarshalVersions/Server/TestNotification.java + test/com/sun/jmx/remote/NotificationMarshalVersions/TestSerializationMismatch.sh + test/javax/management/MBeanInfo/SerializationTest1.java
Re: RFR JDK-8005120
uild? >>>> >>>> -Chris. >>>> >>>> On 12/19/2012 03:42 PM, Alan Bateman wrote: >>>>> >>>>> John - this is the debugger socket transport so cc'ing the >>>>> serviceability-dev list as that is where this code is maintained. >>>>> >>>>> On 19/12/2012 15:36, John Zavgren wrote: >>>>>> Greetings: >>>>>> Please consider the following change to the two files: >>>>>> src/share/transport/socket/sysSocket.h >>>>>> src/solaris/transport/socket/socket_md.c >>>>>> that eliminate compiler warnings that stem from the fact that the >>>>>> variables that the native code passes to various system calls were not >>>>>> declared correctly. They were declared as integers, but they must be >>>>>> "unsigned" integers because they are used to define buffer lengths. >>>>>> Were one to supply a negative value as an argument, it would be cast >>>>>> into an unsigned "Martian" value and there'd be (hopefully) a system >>>>>> call error. >>>>>> >>>>>> Thanks! >>>>>> John Zavgren >>>>>> >>>>>> http://cr.openjdk.java.net/~mullan/webrevs/jzavgren/8005120/ >>>>> -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * Give Rabbit time, and he'll always get the answer
Re: RFR JDK-8005120
;>>>> ../../../../src/share/transport/socket/sysSocket.h:58:5: note: >>>>> expected >>>>> 'uint32_t *' but argument is of type 'int *' >>>>> ../../../../src/share/transport/socket/socketTransport.c: In function >>>>> 'socketTransport_accept': >>>>> ../../../../src/share/transport/socket/socketTransport.c:371:33: >>>>> warning: pointer targets in passing argument 3 of 'dbgsysAccept' >>>>> differ >>>>> in signedness [-Wpointer-sign] >>>>> ../../../../src/share/transport/socket/sysSocket.h:41:5: note: >>>>> expected >>>>> 'uint32_t *' but argument is of type 'int *' >>>>> >>>>> Do you see these in your build? >>>>> >>>>> -Chris. >>>>> >>>>> On 12/19/2012 03:42 PM, Alan Bateman wrote: >>>>>> John - this is the debugger socket transport so cc'ing the >>>>>> serviceability-dev list as that is where this code is maintained. >>>>>> >>>>>> On 19/12/2012 15:36, John Zavgren wrote: >>>>>>> Greetings: >>>>>>> Please consider the following change to the two files: >>>>>>> src/share/transport/socket/sysSocket.h >>>>>>> src/solaris/transport/socket/socket_md.c >>>>>>> that eliminate compiler warnings that stem from the fact that the >>>>>>> variables that the native code passes to various system calls >>>>>>> were not >>>>>>> declared correctly. They were declared as integers, but they must be >>>>>>> "unsigned" integers because they are used to define buffer lengths. >>>>>>> Were one to supply a negative value as an argument, it would be cast >>>>>>> into an unsigned "Martian" value and there'd be (hopefully) a system >>>>>>> call error. >>>>>>> >>>>>>> Thanks! >>>>>>> John Zavgren >>>>>>> >>>>>>> http://cr.openjdk.java.net/~mullan/webrevs/jzavgren/8005120/ > > -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * Give Rabbit time, and he'll always get the answer
Re: RFR 8006395: Race in async socket close on Linux
Chris, Looks good for me. -Dmitry On 2013-01-31 01:36, Chris Hegarty wrote: > There is a very small, and very old, window for a race in the socket > asynchronous close mechanism on Linux. This can lead to blocking socket > operations never returning even after the socket has been closed. > > This issue would appear to exist since its (linux_close.c) creation back > in 1.4, but since the window for the race is tiny, it seems to have gone > unnoticed until now. It was originally diagnosed through code > inspection, but since then I have created and added a small test that > reproduce the issue about one in every 10 - 20 runs, with jdk8, on > Ubuntu 12.04, with 2x 2.33GHz Intel Xeon E5345 (2x quad-core, 1 thread > per core => 8 threads). > > closefd first interrupts (sends wakeup signal to) all the threads > blocked on the fd, then it closes/dup2's the fd. However, the signal may > arrive at its target thread before that thread has entered the blocking > system call, and before close/dup2. In this case, the target thread will > simple enter the blocking system call and never return. > > Solution > - > If it was to close/dup2 the fd before issuing the wake up, then any > thread not yet blocked in a system call should see that the fd is closed > on entry, otherwise it will be woken up by the signal. > > While there is an equivalent closefd in bsd_close.c ( mac/bsd specific > code), I have not been able to reproduce this issue after many test runs > on mac. Also, making similar changes to closefd in bsd_close runs into a > problem with dup2; dup2 will hang if another thread is doing a blocking > operation. I believe this issue is similar to 7133499. So as far as this > issue is concerned changes will only be make to the Linux version of > closefd. > > Webrev > --- > > http://cr.openjdk.java.net/~chegar/8006395/webrev.00/webrev/ > > -Chris. -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * Give Rabbit time, and he'll always get the answer
hg: jdk8/tl/jdk: 8002048: Protocol to discovery of manageable Java processes on a network
Changeset: 962d6612cace Author:dsamersoff Date: 2013-02-03 21:39 +0400 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/962d6612cace 8002048: Protocol to discovery of manageable Java processes on a network Summary: Introduce a protocol to discover manageble Java instances across a network subnet, JDP Reviewed-by: sla, dfuchs ! src/share/classes/sun/management/Agent.java + src/share/classes/sun/management/jdp/JdpBroadcaster.java + src/share/classes/sun/management/jdp/JdpController.java + src/share/classes/sun/management/jdp/JdpException.java + src/share/classes/sun/management/jdp/JdpGenericPacket.java + src/share/classes/sun/management/jdp/JdpJmxPacket.java + src/share/classes/sun/management/jdp/JdpPacket.java + src/share/classes/sun/management/jdp/JdpPacketReader.java + src/share/classes/sun/management/jdp/JdpPacketWriter.java + src/share/classes/sun/management/jdp/package-info.java + test/sun/management/jdp/JdpClient.java + test/sun/management/jdp/JdpDoSomething.java + test/sun/management/jdp/JdpTest.sh + test/sun/management/jdp/JdpUnitTest.java
hg: jdk8/tl/jdk: 8007277: JDK-8002048 testcase fails to compile
Changeset: 0e7d5dd84fdf Author:dsamersoff Date: 2013-02-06 16:53 +0400 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/0e7d5dd84fdf 8007277: JDK-8002048 testcase fails to compile Summary: sun.* classes is not included to ct.sym file and symbol file have to be ignored Reviewed-by: alanb ! test/sun/management/jdp/JdpTest.sh
hg: jdk8/tl/jdk: 8007536: Incorrect copyright header in JDP files
Changeset: 1df991184045 Author:dsamersoff Date: 2013-02-11 18:44 +0400 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/1df991184045 8007536: Incorrect copyright header in JDP files Summary: Copyright header in JDP files missed the "classpath exception" rule. Reviewed-by: mikael ! src/share/classes/sun/management/jdp/JdpBroadcaster.java ! src/share/classes/sun/management/jdp/JdpController.java ! src/share/classes/sun/management/jdp/JdpException.java ! src/share/classes/sun/management/jdp/JdpGenericPacket.java ! src/share/classes/sun/management/jdp/JdpJmxPacket.java ! src/share/classes/sun/management/jdp/JdpPacket.java ! src/share/classes/sun/management/jdp/JdpPacketReader.java ! src/share/classes/sun/management/jdp/JdpPacketWriter.java ! src/share/classes/sun/management/jdp/package-info.java
hg: jdk8/tl/jdk: 8007786: JDK-8002048 testcase doesn't work on Solaris
Changeset: f7fb173ac833 Author:dsamersoff Date: 2013-02-12 16:02 +0400 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/f7fb173ac833 8007786: JDK-8002048 testcase doesn't work on Solaris Summary: test built in into Solaris shell doesn't have -e operator Reviewed-by: sla, sspitsyn ! test/sun/management/jdp/JdpTest.sh
hg: jdk8/tl/jdk: 8008095: TEST_BUG: JDK-8002048 one more testcase failure on Solaris
Changeset: 8181be9a3538 Author:dsamersoff Date: 2013-02-13 21:06 +0400 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/8181be9a3538 8008095: TEST_BUG: JDK-8002048 one more testcase failure on Solaris Summary: fixed couple of more Solaris shell incompatibilities Reviewed-by: chegar ! test/sun/management/jdp/JdpTest.sh
Re: RFR JDK-8008804
John, closesocket call should be here. -Dmitry On 2013-03-04 20:12, John Zavgren wrote: > Greetings: > > I posted a webrev image for a modification that eliminates a file descriptor > leak. > > http://cr.openjdk.java.net/~jzavgren/8008804/webrev.01/ > > Thanks! > John > -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * Give Rabbit time, and he'll always get the answer
Re: RFR JDK-8008804
John, Why socketclose? Is it intentional? -Dmitry On 2013-03-04 23:37, John Zavgren wrote: > Thanks for catching that Dmitry. I just posted a new webrev image for a > change that uses the correct procedure: > http://cr.openjdk.java.net/~jzavgren/8008804/webrev.02/ > > John > - Original Message - > From: dmitry.samers...@oracle.com > To: john.zavg...@oracle.com > Cc: net-dev@openjdk.java.net > Sent: Monday, March 4, 2013 12:59:16 PM GMT -05:00 US/Canada Eastern > Subject: Re: RFR JDK-8008804 > > John, > > closesocket call should be here. > > -Dmitry > > On 2013-03-04 20:12, John Zavgren wrote: >> Greetings: >> >> I posted a webrev image for a modification that eliminates a file descriptor >> leak. >> >> http://cr.openjdk.java.net/~jzavgren/8008804/webrev.01/ >> >> Thanks! >> John >> > > -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * Give Rabbit time, and he'll always get the answer
Re: Request for review: 8009650 - HttpClient available() check throws SocketException when connection has been closed
Rob,
Is it possible to avoid code duplication?
i.e. do something like this:
int r;
try {
...
} catch (SocketException e) {
// Comments goes here
r = -1
}
if (r == -1){
if (logger. ...
available = false;
}
return available;
-Dmitry
On 2013-03-07 20:18, Rob McKenna wrote:
> Hi folks,
>
> This is a slight alteration of the fix contributed by Stuart Douglas.
> This fix deals with a SocketException caused by getSoTimeout() on a
> closed connection.
>
> http://cr.openjdk.java.net/~robm/8009650/webrev.01/
>
> -Rob
--
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* Give Rabbit time, and he'll always get the answer
Re: Request for review: 8009650 - HttpClient available() check throws SocketException when connection has been closed
Rob,
Sorry for not being clean enough. We have repeated pattern:
if (logger.isLoggable(PlatformLogger.FINEST)) {
logger.finest("HttpClient.available(): " + msg
}
so it makes code better readable if we can put it to some common place.
-Dmitry
On 2013-03-08 02:31, Rob McKenna wrote:
> Hi Dmitry,
>
> I'm not 100% sure what you mean by duplication, the exceptions and their
> messages are distinct. I think it would be best to keep it that way.
>
> -Rob
>
> On 07/03/13 22:00, Dmitry Samersoff wrote:
>> Rob,
>>
>> Is it possible to avoid code duplication?
>>
>> i.e. do something like this:
>>
>> int r;
>>
>> try {
>>...
>> } catch (SocketException e) {
>> // Comments goes here
>> r = -1
>> }
>>
>> if (r == -1){
>>if (logger. ...
>>available = false;
>> }
>>
>>return available;
>>
>> -Dmitry
>>
>>
>> On 2013-03-07 20:18, Rob McKenna wrote:
>>> Hi folks,
>>>
>>> This is a slight alteration of the fix contributed by Stuart Douglas.
>>> This fix deals with a SocketException caused by getSoTimeout() on a
>>> closed connection.
>>>
>>> http://cr.openjdk.java.net/~robm/8009650/webrev.01/
>>>
>>> -Rob
>>
>
--
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* Give Rabbit time, and he'll always get the answer
Re: Request for review: 8009650 - HttpClient available() check throws SocketException when connection has been closed
Rob,
Looks good for me.
-Dmitry
On 2013-03-13 04:35, Rob McKenna wrote:
> Hi folks,
>
> New webrev at:
>
> http://cr.openjdk.java.net/~robm/8009650/webrev.02/
>
> Apologies for the delay.
>
> -Rob
>
> On 07/03/13 23:19, Rob McKenna wrote:
>> Ah, I see what you mean. Can do.
>>
>> -Rob
>>
>> On 07/03/13 23:13, Dmitry Samersoff wrote:
>>> Rob,
>>>
>>> Sorry for not being clean enough. We have repeated pattern:
>>>
>>> if (logger.isLoggable(PlatformLogger.FINEST)) {
>>> logger.finest("HttpClient.available(): " + msg
>>> }
>>>
>>> so it makes code better readable if we can put it to some common place.
>>>
>>> -Dmitry
>>>
>>> On 2013-03-08 02:31, Rob McKenna wrote:
>>>> Hi Dmitry,
>>>>
>>>> I'm not 100% sure what you mean by duplication, the exceptions and
>>>> their
>>>> messages are distinct. I think it would be best to keep it that way.
>>>>
>>>> -Rob
>>>>
>>>> On 07/03/13 22:00, Dmitry Samersoff wrote:
>>>>> Rob,
>>>>>
>>>>> Is it possible to avoid code duplication?
>>>>>
>>>>> i.e. do something like this:
>>>>>
>>>>> int r;
>>>>>
>>>>> try {
>>>>> ...
>>>>> } catch (SocketException e) {
>>>>> // Comments goes here
>>>>> r = -1
>>>>> }
>>>>>
>>>>> if (r == -1){
>>>>> if (logger. ...
>>>>> available = false;
>>>>> }
>>>>>
>>>>> return available;
>>>>>
>>>>> -Dmitry
>>>>>
>>>>>
>>>>> On 2013-03-07 20:18, Rob McKenna wrote:
>>>>>> Hi folks,
>>>>>>
>>>>>> This is a slight alteration of the fix contributed by Stuart Douglas.
>>>>>> This fix deals with a SocketException caused by getSoTimeout() on a
>>>>>> closed connection.
>>>>>>
>>>>>> http://cr.openjdk.java.net/~robm/8009650/webrev.01/
>>>>>>
>>>>>> -Rob
>>>
>>
>
--
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* Give Rabbit time, and he'll always get the answer
Re: RFR 8012244: java/net/Socket/asyncClose/Race.java fails intermittently on Windows
Chris, src/windows/native/java/net/SocketInputStream.c:137 May be we can just check for WSA_INVALID_HANDLE here -Dmitry On 2013-04-15 21:08, Chris Hegarty wrote: > Test issue where the test fails on some Windows versions, and minor > implementation clean up. > > http://cr.openjdk.java.net/~chegar/8012244/webrev.00/webrev/ > > -Chris. -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * Give Rabbit time, and he'll always get the answer
Re: RFR-JDK8012108
John, I see bad realloc pattern here. Could you fix it as well? e.g. 93 adapterInfo = (IP_ADAPTER_ADDRESSES *) realloc (adapterInfo, len); -Dmitry On 2013-04-19 00:56, John Zavgren wrote: > Greetings: > > I fixed a case in the windows native code where calloc() was being used > without checking it's returned value. > > http://cr.openjdk.java.net/~jzavgren/8012108/webrev.01/ > > Thanks! > John Zavgren -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * Give Rabbit time, and he'll always get the answer
Re: RFR-JDK8012108
John, 102, 145 else is redundant. 438 - we will crash here if start is null -Dmitry On 2013-04-20 01:33, John Zavgren wrote: > Greetings: > > I fixed the bad realloc pattern. Please let me know what you think. > http://cr.openjdk.java.net/~jzavgren/8012108/webrev.02/ > > Thanks! > John Z > > > - Original Message - > From: chris.hega...@oracle.com > To: net-dev@openjdk.java.net, john.zavg...@oracle.com > Cc: dmitry.samers...@oracle.com > Sent: Friday, April 19, 2013 8:59:25 AM GMT -05:00 US/Canada Eastern > Subject: Re: RFR-JDK8012108 > > On 18/04/2013 22:11, Dmitry Samersoff wrote: >> John, >> >> I see bad realloc pattern here. Could you fix it as well? > > Yes, please. Otherwise the changes look fine. > > -Chris. > >> >> e.g. >> >> 93 adapterInfo = (IP_ADAPTER_ADDRESSES *) realloc (adapterInfo, len); >> >> -Dmitry >> >> On 2013-04-19 00:56, John Zavgren wrote: >>> Greetings: >>> >>> I fixed a case in the windows native code where calloc() was being used >>> without checking it's returned value. >>> >>> http://cr.openjdk.java.net/~jzavgren/8012108/webrev.01/ >>> >>> Thanks! >>> John Zavgren >> >> -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * Give Rabbit time, and he'll always get the answer
Re: RFR-JDK8012108
Kurchi, if *netaddrPP == NULL at 367 and calloc fails at 391 we would jump to freeAllocatedMemory with start == NULL I have no ideas whether this code path is possible in reality or not, but it stops my eyes. -Dmitry On 2013-04-20 14:55, Kurchi Subhra Hazra wrote: > > > On Apr 20, 2013, at 3:16 AM, Dmitry Samersoff > wrote: > >> John, >> >> 102, 145 else is redundant. >> >> 438 - we will crash here if start is null > > > Maybe it is good to have an additional check for start != null, but from what > I see, start will not be null if *netaddrPP is not null. > > > > > >> >> -Dmitry >> >> >> On 2013-04-20 01:33, John Zavgren wrote: >>> Greetings: >>> >>> I fixed the bad realloc pattern. Please let me know what you think. >>> http://cr.openjdk.java.net/~jzavgren/8012108/webrev.02/ >>> >>> Thanks! >>> John Z >>> >>> >>> - Original Message - >>> From: chris.hega...@oracle.com >>> To: net-dev@openjdk.java.net, john.zavg...@oracle.com >>> Cc: dmitry.samers...@oracle.com >>> Sent: Friday, April 19, 2013 8:59:25 AM GMT -05:00 US/Canada Eastern >>> Subject: Re: RFR-JDK8012108 >>> >>> On 18/04/2013 22:11, Dmitry Samersoff wrote: >>>> John, >>>> >>>> I see bad realloc pattern here. Could you fix it as well? >>> >>> Yes, please. Otherwise the changes look fine. >>> >>> -Chris. >>> >>>> >>>> e.g. >>>> >>>> 93 adapterInfo = (IP_ADAPTER_ADDRESSES *) realloc (adapterInfo, len); >>>> >>>> -Dmitry >>>> >>>> On 2013-04-19 00:56, John Zavgren wrote: >>>>> Greetings: >>>>> >>>>> I fixed a case in the windows native code where calloc() was being used >>>>> without checking it's returned value. >>>>> >>>>> http://cr.openjdk.java.net/~jzavgren/8012108/webrev.01/ >>>>> >>>>> Thanks! >>>>> John Zavgren >> >> >> -- >> Dmitry Samersoff >> Oracle Java development team, Saint Petersburg, Russia >> * Give Rabbit time, and he'll always get the answer -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * Give Rabbit time, and he'll always get the answer
Re: RFR-JDK8012108
John,
OK. Thank you for the explanation.
It looks like i misread the code.
-Dmitry
On 2013-04-24 19:35, John Zavgren wrote:
> Dmitry:
> Thanks for your comments
>
> From my reading of the code for the procedure: getAddrsFromAdapter(...):
>
> If *netaddrPP == NULL at line 369, and a jump is made to
> "freeAllocatedMemory" because of a calloc() failure, then obviously the
> assignment operation on line 429 (*netaddrPP = start) is skipped, and
> *netaddrPP remains NULL... and consequently the "block":
> if (*netaddrPP != NULL) {
> // We started with an existing list
> curr=start->next;
> start->next = NULL;
> start = curr;
> }
> is not executed (and no references are made to start->next).
>
> On the other hand, if *netaddrPP == NULL at line 369 and no errors
> occur, we make the assignment:
> *netaddrPP = start at line 429 and return immediately (without
> considering start->next).
>
> If you see any errors in my thinking, please let me know.
> Thanks!
> John
>
>
> On 04/20/2013 07:40 AM, Dmitry Samersoff wrote:
>> Kurchi,
>>
>> if *netaddrPP == NULL at 367 and calloc fails at 391 we would jump
>> to freeAllocatedMemory with start == NULL
>>
>>
>> I have no ideas whether this code path is possible in reality or not,
>> but it stops my eyes.
>>
>> -Dmitry
>>
>>
>> On 2013-04-20 14:55, Kurchi Subhra Hazra wrote:
>>>
>>> On Apr 20, 2013, at 3:16 AM, Dmitry Samersoff
>>> wrote:
>>>
>>>> John,
>>>>
>>>> 102, 145 else is redundant.
>>>>
>>>> 438 - we will crash here if start is null
>>>
>>> Maybe it is good to have an additional check for start != null, but
>>> from what I see, start will not be null if *netaddrPP is not null.
>>>
>>>
>>>
>>>
>>>
>>>> -Dmitry
>>>>
>>>>
>>>> On 2013-04-20 01:33, John Zavgren wrote:
>>>>> Greetings:
>>>>>
>>>>> I fixed the bad realloc pattern. Please let me know what you think.
>>>>> http://cr.openjdk.java.net/~jzavgren/8012108/webrev.02/
>>>>>
>>>>> Thanks!
>>>>> John Z
>>>>>
>>>>>
>>>>> - Original Message -
>>>>> From: chris.hega...@oracle.com
>>>>> To: net-dev@openjdk.java.net, john.zavg...@oracle.com
>>>>> Cc: dmitry.samers...@oracle.com
>>>>> Sent: Friday, April 19, 2013 8:59:25 AM GMT -05:00 US/Canada Eastern
>>>>> Subject: Re: RFR-JDK8012108
>>>>>
>>>>> On 18/04/2013 22:11, Dmitry Samersoff wrote:
>>>>>> John,
>>>>>>
>>>>>> I see bad realloc pattern here. Could you fix it as well?
>>>>> Yes, please. Otherwise the changes look fine.
>>>>>
>>>>> -Chris.
>>>>>
>>>>>> e.g.
>>>>>>
>>>>>> 93 adapterInfo = (IP_ADAPTER_ADDRESSES *) realloc
>>>>>> (adapterInfo, len);
>>>>>>
>>>>>> -Dmitry
>>>>>>
>>>>>> On 2013-04-19 00:56, John Zavgren wrote:
>>>>>>> Greetings:
>>>>>>>
>>>>>>> I fixed a case in the windows native code where calloc() was
>>>>>>> being used
>>>>>>> without checking it's returned value.
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~jzavgren/8012108/webrev.01/
>>>>>>>
>>>>>>> Thanks!
>>>>>>> John Zavgren
>>>>
>>>> --
>>>> Dmitry Samersoff
>>>> Oracle Java development team, Saint Petersburg, Russia
>>>> * Give Rabbit time, and he'll always get the answer
>>
>
>
--
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* Give Rabbit time, and he'll always get the answer
Re: RFR-JDK8012108
John,
Looks good for me besides some nits:
NetworkInterface.c:
132,208,405: better rearrange if and get rid of extra else
151: occasional space changes
162: //else comment seems redundant for me
408: missed free(tableP)
NetworkInterface_winXP.c:
103: else is redundant
141: { bracket misplaced
146: //else comment seems redundant for me
ResolverConfigurationImpl.c:
130: better rearrange if and get rid of extra else
free(adapterP) missed
-Dmitry
On 2013-04-25 05:10, John Zavgren wrote:
> All:
>
> I expanded the scope of the work for this bug and cleaned up other
> realloc() errors in the windows native code. I believe I've identified
> all unsafe calls to realloc() in this corner of the native code.
>
> Two additional files were affected.
>
> Please let me know what you think:
> http://cr.openjdk.java.net/~jzavgren/8012108/webrev.03/
> <http://cr.openjdk.java.net/%7Ejzavgren/8012108/webrev.03/>
>
> Thanks!
> John
>
> On 04/20/2013 10:36 AM, Kurchi Subhra Hazra wrote:
>> On Apr 20, 2013, at 4:40 AM, Dmitry Samersoff
>> wrote:
>>
>>> Kurchi,
>>>
>>> if *netaddrPP == NULL at 367 and calloc fails at 391 we would jump
>>> to freeAllocatedMemory with start == NULL
>>>
>> True, but then we skip to line 444 since *netaddrPP == NULL, so we don't get
>> to line 438.
>>
>> I am just saying it is not strictly necessary to check if start is null
>> before entering the first if block. We might want to guard against how the
>> code changes in future and put in the check though.
>>
>>
>>> I have no ideas whether this code path is possible in reality or not,
>>> but it stops my eyes.
>>>
>>> -Dmitry
>>>
>>>
>>> On 2013-04-20 14:55, Kurchi Subhra Hazra wrote:
>>>> On Apr 20, 2013, at 3:16 AM, Dmitry Samersoff
>>>> wrote:
>>>>
>>>>> John,
>>>>>
>>>>> 102, 145 else is redundant.
>>>>>
>>>>> 438 - we will crash here if start is null
>>>> Maybe it is good to have an additional check for start != null, but from
>>>> what I see, start will not be null if *netaddrPP is not null.
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>> -Dmitry
>>>>>
>>>>>
>>>>> On 2013-04-20 01:33, John Zavgren wrote:
>>>>>> Greetings:
>>>>>>
>>>>>> I fixed the bad realloc pattern. Please let me know what you think.
>>>>>> http://cr.openjdk.java.net/~jzavgren/8012108/webrev.02/
>>>>>>
>>>>>> Thanks!
>>>>>> John Z
>>>>>>
>>>>>>
>>>>>> - Original Message -
>>>>>> From: chris.hega...@oracle.com
>>>>>> To: net-dev@openjdk.java.net, john.zavg...@oracle.com
>>>>>> Cc: dmitry.samers...@oracle.com
>>>>>> Sent: Friday, April 19, 2013 8:59:25 AM GMT -05:00 US/Canada Eastern
>>>>>> Subject: Re: RFR-JDK8012108
>>>>>>
>>>>>> On 18/04/2013 22:11, Dmitry Samersoff wrote:
>>>>>>> John,
>>>>>>>
>>>>>>> I see bad realloc pattern here. Could you fix it as well?
>>>>>> Yes, please. Otherwise the changes look fine.
>>>>>>
>>>>>> -Chris.
>>>>>>
>>>>>>> e.g.
>>>>>>>
>>>>>>> 93 adapterInfo = (IP_ADAPTER_ADDRESSES *) realloc (adapterInfo,
>>>>>>> len);
>>>>>>>
>>>>>>> -Dmitry
>>>>>>>
>>>>>>> On 2013-04-19 00:56, John Zavgren wrote:
>>>>>>>> Greetings:
>>>>>>>>
>>>>>>>> I fixed a case in the windows native code where calloc() was being used
>>>>>>>> without checking it's returned value.
>>>>>>>>
>>>>>>>> http://cr.openjdk.java.net/~jzavgren/8012108/webrev.01/
>>>>>>>>
>>>>>>>> Thanks!
>>>>>>>> John Zavgren
>>>>> --
>>>>> Dmitry Samersoff
>>>>> Oracle Java development team, Saint Petersburg, Russia
>>>>> * Give Rabbit time, and he'll always get the answer
>>> --
>>> Dmitry Samersoff
>>> Oracle Java development team, Saint Petersburg, Russia
>>> * Give Rabbit time, and he'll always get the answer
>
>
> --
> John Zavgren
> john.zavg...@oracle.com
> 603-821-0904
> US-Burlington-MA
>
--
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* Give Rabbit time, and he'll always get the answer
Re: API change for 8010464: Evolve java networking same origin policy
Michael, > "GET,POST:Header1,Header2" Colon is a delimiter between http header and it's value. With this syntax we might have problems in a future if sometimes we will support different headers for different methods or add an ability to check header value as well. -Dmitry On 2013-04-30 14:30, Michael McMahon wrote: > Hi Kurchi, > > I can include such an example easily. Eg: > > "GET,POST:Header1,Header2" > > means one permission that permits either GET or POST with either or both > of the two headers. If you wanted to restrict one set of headers to GET > and another set to POST, then that would require two different > permissions. > > - Michael > > On 30/04/13 00:40, Kurchi Hazra wrote: >> Hi Michael, >> >> From the documentation, it is not clear to me how to represent >> both request-headers and method list together in an actions string for >> two or more methods. (Say I have two methods GET and POST and I want >> to specify a request-headers list for each, how do I do it?) Maybe >> another example will help. >> >> Thanks, >> Kurchi >> >> On 4/29/2013 3:53 AM, Michael McMahon wrote: >>> On 28/04/13 09:01, Chris Hegarty wrote: >>>> In the main I link the new HttpURLPermission class. >>>> >>>> When reading the docs I found the references to "the URL" and "URL >>>> string" confusing ( it could be just me ). When I see capital 'URL' >>>> my mind instantly, and incorrectly, goes to java.net.URL. In all >>>> cases you mean the URL string given when constructing the >>>> HttpURLPermission, right? >>>> >>> >>> Yes, that is what is meant. The class does not use j.n.URL at all, as >>> that would bring us back >>> to the old (undesirable) behavior with DNS lookups required for basic >>> operations like equals() and hashCode() >>> >>>> Another example is the equals method >>>> "Returns true if, this.getActions().equals(p.getActions()) and p's >>>>URL equals this's URL. Returns false otherwise." >>>> >>>> this is referring so a simple string comparison of the given URL >>>> string, right? This should be case insensitive too. Does it take >>>> into account default protocol ports, e.g. http://foo.com/ equal >>>> http://foo.com:80/ >>>> >>> >>> The implementation uses a java.net.URI internally. So URI takes care >>> of that. >>> >>>> implies() makes reference to the URL scheme, and other specific >>>> parts of the URL. Also, the constructors throw IAE 'if url is not a >>>> valid URL', but what does this mean. Should we just bite the bullet >>>> and just say that URI is used to parse the given string into its >>>> specific parts? Otherwise, how can this be validated. >>>> >>> >>> I originally didn't want to mention URI in the apidoc due to >>> potential confusion surrounding the use of URL in the permission >>> class name. But, maybe it would be clearer to be explicit about it, >>> particularly for the equals() behavior. >>> Otherwise we have to specify all of it in this class. >>> >>>> As for the additions to HttpURLConnection, what are the implications >>>> on proxies? Permissions, etc... >>>> >>> >>> There's no change in behavior with respect to proxies. Permission is >>> given to connect to proxies implicitly >>> except in cases where the caller specifies the proxy through the >>> URL.openConnection(Proxy) api. >>> There are other unusual cases like the Http "Use Proxy" response. >>> Explicit permission is required >>> for that case also. >>> >>> Thanks! >>> >>> Michael >>> >>>> -Chris. >>>> >>>> On 04/26/2013 03:36 PM, Michael McMahon wrote: >>>>> Hi, >>>>> >>>>> The is the suggested API for one of the two new JEPs recently >>>>> submitted. >>>>> >>>>> This is for JEP 184: HTTP URL Permissions >>>>> >>>>> The idea here is to define a higher level http permission class >>>>> which "knows about" URLs, HTTP request methods and headers. >>>>> So, it is no longer necessary to grant blanket permission for any kind >>>>> of TCP connection to a host/port. Instead a HttpURLPermission >>>>> restricts >>>>> access to only the Http protocol itself. Restrictions can also be >>>>> imposed >>>>> based on URL paths, specific request methods and request headers. >>>>> >>>>> The API change can be seen at the URL below: >>>>> >>>>> http://cr.openjdk.java.net/~michaelm/8010464/api/ >>>>> >>>>> In addition to defining a new permission class, HttpURLConnection >>>>> is modified to make use of it and the documentation change >>>>> describing this >>>>> can be seen at the link below: >>>>> >>>>> http://cr.openjdk.java.net/~michaelm/8010464/api/blender.html >>>>> >>>>> All comments welcome. >>>>> >>>>> Thanks >>>>> >>>>> Michael. >>> >> > -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * Give Rabbit time, and he'll always get the answer
Re: API change for 8010464: Evolve java networking same origin policy
Michael,
I'm just asking about replacing : (colon) to another character to be
able to write something like:
permission
java.net.HttpURLPermission "http://www.foo.com/-";,
"GET Location: http://www.foo.com/*, Content-type: image/jpeg";
in a future
-Dmitry.
On 2013-05-01 15:04, Michael McMahon wrote:
> On 01/05/13 11:09, Dmitry Samersoff wrote:
>> Michael,
>>
>>> "GET,POST:Header1,Header2"
>> Colon is a delimiter between http header and it's value.
>>
>> With this syntax we might have problems in a future if sometimes we will
>> support different headers for different methods or add an ability to
>> check header value as well.
>>
>> -Dmitry
>
> Dmitry,
>
> It would complicate the syntax a lot if you wanted to support
> different headers for different methods. Would be a lot simpler
> to just grant separate permissions for the two cases. Eg.
>
> grant {
> permission java.net.HttpURLPermission "http://www.foo.com/-";,
> "GET:Header1,Header2";
> permission java.net.HttpURLPermission "http://www.foo.com/-";,
> "POST:Header3,Header4";
> };
>
> Michael
>
>> On 2013-04-30 14:30, Michael McMahon wrote:
>>> Hi Kurchi,
>>>
>>> I can include such an example easily. Eg:
>>>
>>> "GET,POST:Header1,Header2"
>>>
>>> means one permission that permits either GET or POST with either or both
>>> of the two headers. If you wanted to restrict one set of headers to GET
>>> and another set to POST, then that would require two different
>>> permissions.
>>>
>>> - Michael
>>>
>>> On 30/04/13 00:40, Kurchi Hazra wrote:
>>>> Hi Michael,
>>>>
>>>> From the documentation, it is not clear to me how to represent
>>>> both request-headers and method list together in an actions string for
>>>> two or more methods. (Say I have two methods GET and POST and I want
>>>> to specify a request-headers list for each, how do I do it?) Maybe
>>>> another example will help.
>>>>
>>>> Thanks,
>>>> Kurchi
>>>>
>>>> On 4/29/2013 3:53 AM, Michael McMahon wrote:
>>>>> On 28/04/13 09:01, Chris Hegarty wrote:
>>>>>> In the main I link the new HttpURLPermission class.
>>>>>>
>>>>>> When reading the docs I found the references to "the URL" and "URL
>>>>>> string" confusing ( it could be just me ). When I see capital 'URL'
>>>>>> my mind instantly, and incorrectly, goes to java.net.URL. In all
>>>>>> cases you mean the URL string given when constructing the
>>>>>> HttpURLPermission, right?
>>>>>>
>>>>> Yes, that is what is meant. The class does not use j.n.URL at all, as
>>>>> that would bring us back
>>>>> to the old (undesirable) behavior with DNS lookups required for basic
>>>>> operations like equals() and hashCode()
>>>>>
>>>>>> Another example is the equals method
>>>>>>"Returns true if, this.getActions().equals(p.getActions()) and p's
>>>>>> URL equals this's URL. Returns false otherwise."
>>>>>>
>>>>>> this is referring so a simple string comparison of the given URL
>>>>>> string, right? This should be case insensitive too. Does it take
>>>>>> into account default protocol ports, e.g. http://foo.com/ equal
>>>>>> http://foo.com:80/
>>>>>>
>>>>> The implementation uses a java.net.URI internally. So URI takes care
>>>>> of that.
>>>>>
>>>>>> implies() makes reference to the URL scheme, and other specific
>>>>>> parts of the URL. Also, the constructors throw IAE 'if url is not a
>>>>>> valid URL', but what does this mean. Should we just bite the bullet
>>>>>> and just say that URI is used to parse the given string into its
>>>>>> specific parts? Otherwise, how can this be validated.
>>>>>>
>>>>> I originally didn't want to mention URI in the apidoc due to
>>>>> potential confusion surrounding the use of URL in the permission
>>>>> class name. But, maybe it would be clearer to be explicit about it,
>>>>> particularly for the equals() behavior.
>>>>> Otherwise we have to
Re: API change for 8010464: Evolve java networking same origin policy
Michael,
Sorry for not being clean enough.
On my opinion an ability to check header value as well as a header name
is quite useful future for the real world.
e.g. to being able to prevent redirection to other domain or limit
certain content type etc.
I understand, that these changes is out of scope of today work, but it
quite possible that we implement it sometimes in a future.
For (header-name: header-value) pair : (colon) is a native delimiter,
so it's better not to use it to separate methods list and headers list.
On my opinion, (space) is enough for this case and better reflect HTTP
header i.e.
"GET,POST Header1,Header2"
-Dmitry
On 2013-05-01 15:16, Michael McMahon wrote:
> Ah right. The permission only contains header names.
> It never contains header values. And header names are "tokens"
> in the Http spec that cannot contain a colon character.
>
> Michael
>
> On 01/05/13 12:11, Dmitry Samersoff wrote:
>> Michael,
>>
>> I'm just asking about replacing : (colon) to another character to be
>> able to write something like:
>>
>> permission
>> java.net.HttpURLPermission "http://www.foo.com/-";,
>> "GET Location: http://www.foo.com/*, Content-type: image/jpeg";
>>
>> in a future
>>
>> -Dmitry.
>>
>> On 2013-05-01 15:04, Michael McMahon wrote:
>>> On 01/05/13 11:09, Dmitry Samersoff wrote:
>>>> Michael,
>>>>
>>>>> "GET,POST:Header1,Header2"
>>>> Colon is a delimiter between http header and it's value.
>>>>
>>>> With this syntax we might have problems in a future if sometimes we
>>>> will
>>>> support different headers for different methods or add an ability to
>>>> check header value as well.
>>>>
>>>> -Dmitry
>>> Dmitry,
>>>
>>> It would complicate the syntax a lot if you wanted to support
>>> different headers for different methods. Would be a lot simpler
>>> to just grant separate permissions for the two cases. Eg.
>>>
>>> grant {
>>> permission java.net.HttpURLPermission "http://www.foo.com/-";,
>>> "GET:Header1,Header2";
>>> permission java.net.HttpURLPermission "http://www.foo.com/-";,
>>> "POST:Header3,Header4";
>>> };
>>>
>>> Michael
>>>
>>>> On 2013-04-30 14:30, Michael McMahon wrote:
>>>>> Hi Kurchi,
>>>>>
>>>>> I can include such an example easily. Eg:
>>>>>
>>>>> "GET,POST:Header1,Header2"
>>>>>
>>>>> means one permission that permits either GET or POST with either or
>>>>> both
>>>>> of the two headers. If you wanted to restrict one set of headers to
>>>>> GET
>>>>> and another set to POST, then that would require two different
>>>>> permissions.
>>>>>
>>>>> - Michael
>>>>>
>>>>> On 30/04/13 00:40, Kurchi Hazra wrote:
>>>>>> Hi Michael,
>>>>>>
>>>>>>From the documentation, it is not clear to me how to represent
>>>>>> both request-headers and method list together in an actions string
>>>>>> for
>>>>>> two or more methods. (Say I have two methods GET and POST and I want
>>>>>> to specify a request-headers list for each, how do I do it?) Maybe
>>>>>> another example will help.
>>>>>>
>>>>>> Thanks,
>>>>>> Kurchi
>>>>>>
>>>>>> On 4/29/2013 3:53 AM, Michael McMahon wrote:
>>>>>>> On 28/04/13 09:01, Chris Hegarty wrote:
>>>>>>>> In the main I link the new HttpURLPermission class.
>>>>>>>>
>>>>>>>> When reading the docs I found the references to "the URL" and "URL
>>>>>>>> string" confusing ( it could be just me ). When I see capital 'URL'
>>>>>>>> my mind instantly, and incorrectly, goes to java.net.URL. In all
>>>>>>>> cases you mean the URL string given when constructing the
>>>>>>>> HttpURLPermission, right?
>>>>>>>>
>>>>>>> Yes, that is what is meant. The class does not use j.n.URL at
>>>>>>> all, as
>>>>>>> that would bring us back
>>>>>>> to the old (undesirable)
Re: API change for 8010464: Evolve java networking same origin policy
Michael,
On 2013-05-01 16:40, Michael McMahon wrote:
> Another possibility if we were to do that in the future, could be:
>
> "GET,POST:Header1=value 1,Header2=value 2"
Variant with space looks more natural for me as it follows HTTP header
syntax.
But it's up to you to make a decision.
-Dmitry
>
> - Michael
>
> On 01/05/13 12:38, Dmitry Samersoff wrote:
>> Michael,
>>
>> Sorry for not being clean enough.
>>
>> On my opinion an ability to check header value as well as a header name
>> is quite useful future for the real world.
>>
>> e.g. to being able to prevent redirection to other domain or limit
>> certain content type etc.
>>
>> I understand, that these changes is out of scope of today work, but it
>> quite possible that we implement it sometimes in a future.
>>
>> For (header-name: header-value) pair : (colon) is a native delimiter,
>> so it's better not to use it to separate methods list and headers list.
>>
>> On my opinion, (space) is enough for this case and better reflect HTTP
>> header i.e.
>>
>> "GET,POST Header1,Header2"
>>
>> -Dmitry
>>
>>
>>
>> On 2013-05-01 15:16, Michael McMahon wrote:
>>> Ah right. The permission only contains header names.
>>> It never contains header values. And header names are "tokens"
>>> in the Http spec that cannot contain a colon character.
>>>
>>> Michael
>>>
>>> On 01/05/13 12:11, Dmitry Samersoff wrote:
>>>> Michael,
>>>>
>>>> I'm just asking about replacing : (colon) to another character to be
>>>> able to write something like:
>>>>
>>>>permission
>>>>java.net.HttpURLPermission "http://www.foo.com/-";,
>>>>"GET Location: http://www.foo.com/*, Content-type: image/jpeg";
>>>>
>>>> in a future
>>>>
>>>> -Dmitry.
>>>>
>>>> On 2013-05-01 15:04, Michael McMahon wrote:
>>>>> On 01/05/13 11:09, Dmitry Samersoff wrote:
>>>>>> Michael,
>>>>>>
>>>>>>> "GET,POST:Header1,Header2"
>>>>>> Colon is a delimiter between http header and it's value.
>>>>>>
>>>>>> With this syntax we might have problems in a future if sometimes we
>>>>>> will
>>>>>> support different headers for different methods or add an ability to
>>>>>> check header value as well.
>>>>>>
>>>>>> -Dmitry
>>>>> Dmitry,
>>>>>
>>>>> It would complicate the syntax a lot if you wanted to support
>>>>> different headers for different methods. Would be a lot simpler
>>>>> to just grant separate permissions for the two cases. Eg.
>>>>>
>>>>> grant {
>>>>> permission java.net.HttpURLPermission "http://www.foo.com/-";,
>>>>> "GET:Header1,Header2";
>>>>> permission java.net.HttpURLPermission "http://www.foo.com/-";,
>>>>> "POST:Header3,Header4";
>>>>> };
>>>>>
>>>>> Michael
>>>>>
>>>>>> On 2013-04-30 14:30, Michael McMahon wrote:
>>>>>>> Hi Kurchi,
>>>>>>>
>>>>>>> I can include such an example easily. Eg:
>>>>>>>
>>>>>>> "GET,POST:Header1,Header2"
>>>>>>>
>>>>>>> means one permission that permits either GET or POST with either or
>>>>>>> both
>>>>>>> of the two headers. If you wanted to restrict one set of headers to
>>>>>>> GET
>>>>>>> and another set to POST, then that would require two different
>>>>>>> permissions.
>>>>>>>
>>>>>>> - Michael
>>>>>>>
>>>>>>> On 30/04/13 00:40, Kurchi Hazra wrote:
>>>>>>>> Hi Michael,
>>>>>>>>
>>>>>>>> From the documentation, it is not clear to me how to
>>>>>>>> represent
>>>>>>>> both request-headers and method list together in an actions string
>>>>>>>> for
>>>>>>>> two or more methods. (Say I have two methods GET and POST and I
>>>>>>>> wan
