Re: 8251155: HostIdentifier fails to canonicalize hostnames starting with digits(Internet mail)
Thanks Claes for your review.
Pushed.
Best regards,
Jie
On 2020/8/25, 7:26 PM, "Claes Redestad" wrote:
Hi Jie,
fix looks good to me!
/Claes
On 2020-08-25 04:12, jiefu(傅杰) wrote:
> Thanks Serguei for your review.
>
> Claes, are you okay with this change:
> http://cr.openjdk.java.net/~jiefu/8251155/webrev.00/
>
> Thanks.
> Best regards,
> Jie
>
>
>
> *From:* serguei.spit...@oracle.com
> *Sent:* Tuesday, August 25, 2020 8:38 AM
> *To:* jiefu(傅杰); serviceability-dev@openjdk.java.net; Claes Redestad
> *Subject:* Re: 8251155: HostIdentifier fails to canonicalize hostnames
> starting with digits(Internet mail)
> Hi Jie,
>
> I'm okay with the fix.
>
> Thanks,
> Serguei
>
>
> On 8/24/20 09:21, jiefu(傅杰) wrote:
>>
>> Hi Serguei and Claes,
>>
>> I forget to mention that you can also verify this fix using the
>> following tests:
>>
>> --
>>
>> test/jdk/sun/tools/jstatd/TestJstatdExternalRegistry.java
>>
>> test/jdk/sun/tools/jstatd/TestJstatdPort.java
>>
>> test/jdk/sun/tools/jstatd/TestJstatdPortAndServer.java
>>
>> test/jdk/sun/tools/jstatd/TestJstatdRmiPort.java
>>
>> --
>>
>> Without the patch, All of them will fail if the hostname starting from
>> digits.
>>
>> We've found that it seems very common that the hostname will start
>> with digits in dockers.
>>
>> So it would be better to fix it.
>>
>> What do you think?
>>
>> Thanks.
>>
>> Best regards,
>>
>> Jie
>>
>> *From: *"jiefu(傅杰)"
>> *Date: *Wednesday, August 19, 2020 at 4:05 PM
>> *To: *"serguei.spit...@oracle.com" ,
>> "serviceability-dev@openjdk.java.net"
>> , Claes Redestad
>>
>> *Subject: *Re: 8251155: HostIdentifier fails to canonicalize hostnames
>> starting with digits(Internet mail)
>>
>> Hi Serguei,
>>
>> Thanks for your review and help.
>>
>> Please see comments inline.
>>
>>
>>
>> *From:*serguei.spit...@oracle.com
>> *Sent:* Wednesday, August 19, 2020 4:03 AM
>> *To:* jiefu(傅杰); serviceability-dev@openjdk.java.net; Claes Redestad
>> *Subject:* Re: 8251155: HostIdentifier fails to canonicalize hostnames
>> starting with digits(Internet mail)
>>
>> 83 *
>> 84 * {@code } - transformed into "//localhost"
>> 85 * localhost - transformed into "//localhost"
>> 86 * hostname - transformed into "//hostname"
>> 87 * hostname:port - transformed into "//hostname:port"
>> 88 * proto:hostname - transformed into "proto://hostname"
>> 89 * proto:hostname:port - transformed into
>> 90 * "proto://hostname:port"
>> 91 * proto://hostname:port
>> 92 *
>>
>> >> Is it worth to add an example to the list above?
>>
>> Yes. It's really helpful for the review process. Thanks.
>>
>>
>>
>> >> I wander if this fix needs a CSR.
>>
>> I don't think so.
>>
>> This is just a bug fix which doesn't add/remove/change any feature of
>> the tools.
>>
>> The original design has claimed to support hostname and hostname:port
>> cases.
>>
>> But it fails to do so when the hostname starts with digits.
>>
>> It seems to be very common that the hostname will be started with
>> digits in dockers.
>>
>> So I think it's worth to fix this bug.
>>
>>
>> >> How did you check this fix does not introduce any regressions?
>>
>> In fact, Claes had helped me to answer this question here:
>>
https://mail.openjdk.java.net/pipermail/serviceability-dev/2020-August/032691.html.
>>
>> Also, I've tested this patch on Linux/x64 with
>> tier1 ~ tier3 (no regression).
>>
>> Thanks a lot.
>>
>> Best regards,
>>
>> Jie
>>
>
Re: 8251155: HostIdentifier fails to canonicalize hostnames starting with digits(Internet mail)
Thanks Serguei for your review.
Claes, are you okay with this change:
http://cr.openjdk.java.net/~jiefu/8251155/webrev.00/
Thanks.
Best regards,
Jie
From: serguei.spit...@oracle.com
Sent: Tuesday, August 25, 2020 8:38 AM
To: jiefu(傅杰); serviceability-dev@openjdk.java.net; Claes Redestad
Subject: Re: 8251155: HostIdentifier fails to canonicalize hostnames starting
with digits(Internet mail)
Hi Jie,
I'm okay with the fix.
Thanks,
Serguei
On 8/24/20 09:21, jiefu(傅杰) wrote:
Hi Serguei and Claes,
I forget to mention that you can also verify this fix using the following tests:
--
test/jdk/sun/tools/jstatd/TestJstatdExternalRegistry.java
test/jdk/sun/tools/jstatd/TestJstatdPort.java
test/jdk/sun/tools/jstatd/TestJstatdPortAndServer.java
test/jdk/sun/tools/jstatd/TestJstatdRmiPort.java
--
Without the patch, All of them will fail if the hostname starting from digits.
We've found that it seems very common that the hostname will start with digits
in dockers.
So it would be better to fix it.
What do you think?
Thanks.
Best regards,
Jie
From: "jiefu(傅杰)" <mailto:ji...@tencent.com>
Date: Wednesday, August 19, 2020 at 4:05 PM
To: "serguei.spit...@oracle.com"<mailto:serguei.spit...@oracle.com>
<mailto:serguei.spit...@oracle.com>,
"serviceability-dev@openjdk.java.net"<mailto:serviceability-dev@openjdk.java.net>
<mailto:serviceability-dev@openjdk.java.net>,
Claes Redestad <mailto:claes.redes...@oracle.com>
Subject: Re: 8251155: HostIdentifier fails to canonicalize hostnames starting
with digits(Internet mail)
Hi Serguei,
Thanks for your review and help.
Please see comments inline.
From: serguei.spit...@oracle.com<mailto:serguei.spit...@oracle.com>
<mailto:serguei.spit...@oracle.com>
Sent: Wednesday, August 19, 2020 4:03 AM
To: jiefu(傅杰);
serviceability-dev@openjdk.java.net<mailto:serviceability-dev@openjdk.java.net>;
Claes Redestad
Subject: Re: 8251155: HostIdentifier fails to canonicalize hostnames starting
with digits(Internet mail)
83 *
84 * {@code } - transformed into "//localhost"
85 * localhost - transformed into "//localhost"
86 * hostname - transformed into "//hostname"
87 * hostname:port - transformed into "//hostname:port"
88 * proto:hostname - transformed into "proto://hostname"
89 * proto:hostname:port - transformed into
90 * "proto://hostname:port"
91 * proto://hostname:port
92 *
>> Is it worth to add an example to the list above?
Yes. It's really helpful for the review process. Thanks.
>> I wander if this fix needs a CSR.
I don't think so.
This is just a bug fix which doesn't add/remove/change any feature of the tools.
The original design has claimed to support hostname and hostname:port cases.
But it fails to do so when the hostname starts with digits.
It seems to be very common that the hostname will be started with digits in
dockers.
So I think it's worth to fix this bug.
>> How did you check this fix does not introduce any regressions?
In fact, Claes had helped me to answer this question here:
https://mail.openjdk.java.net/pipermail/serviceability-dev/2020-August/032691.html.
Also, I've tested this patch on Linux/x64 with tier1 ~ tier3 (no regression).
Thanks a lot.
Best regards,
Jie
Re: 8251155: HostIdentifier fails to canonicalize hostnames starting with digits(Internet mail)
Hi Serguei and Claes,
I forget to mention that you can also verify this fix using the following tests:
--
test/jdk/sun/tools/jstatd/TestJstatdExternalRegistry.java
test/jdk/sun/tools/jstatd/TestJstatdPort.java
test/jdk/sun/tools/jstatd/TestJstatdPortAndServer.java
test/jdk/sun/tools/jstatd/TestJstatdRmiPort.java
--
Without the patch, All of them will fail if the hostname starting from digits.
We've found that it seems very common that the hostname will start with digits
in dockers.
So it would be better to fix it.
What do you think?
Thanks.
Best regards,
Jie
From: "jiefu(傅杰)"
Date: Wednesday, August 19, 2020 at 4:05 PM
To: "serguei.spit...@oracle.com" ,
"serviceability-dev@openjdk.java.net" ,
Claes Redestad
Subject: Re: 8251155: HostIdentifier fails to canonicalize hostnames starting
with digits(Internet mail)
Hi Serguei,
Thanks for your review and help.
Please see comments inline.
From: serguei.spit...@oracle.com
Sent: Wednesday, August 19, 2020 4:03 AM
To: jiefu(傅杰); serviceability-dev@openjdk.java.net; Claes Redestad
Subject: Re: 8251155: HostIdentifier fails to canonicalize hostnames starting
with digits(Internet mail)
83 *
84 * {@code } - transformed into "//localhost"
85 * localhost - transformed into "//localhost"
86 * hostname - transformed into "//hostname"
87 * hostname:port - transformed into "//hostname:port"
88 * proto:hostname - transformed into "proto://hostname"
89 * proto:hostname:port - transformed into
90 * "proto://hostname:port"
91 * proto://hostname:port
92 *
>> Is it worth to add an example to the list above?
Yes. It's really helpful for the review process. Thanks.
>> I wander if this fix needs a CSR.
I don't think so.
This is just a bug fix which doesn't add/remove/change any feature of the tools.
The original design has claimed to support hostname and hostname:port cases.
But it fails to do so when the hostname starts with digits.
It seems to be very common that the hostname will be started with digits in
dockers.
So I think it's worth to fix this bug.
>> How did you check this fix does not introduce any regressions?
In fact, Claes had helped me to answer this question here:
https://mail.openjdk.java.net/pipermail/serviceability-dev/2020-August/032691.html.
Also, I've tested this patch on Linux/x64 with tier1 ~ tier3 (no regression).
Thanks a lot.
Best regards,
Jie
Re: 8251155: HostIdentifier fails to canonicalize hostnames starting with digits(Internet mail)
Hi Claes,
Thanks for your review and help.
Best regards,
Jie
From: Claes Redestad
Sent: Wednesday, August 19, 2020 12:51 PM
To: serguei.spit...@oracle.com; jiefu(傅杰); serviceability-dev@openjdk.java.net
Subject: Re: 8251155: HostIdentifier fails to canonicalize hostnames starting
with digits(Internet mail)
Hi,
not sure I do, but a quick read of the relevant RFC suggests that since
a URI scheme (protocol) must start with a letter[1] it seems safe to
assume the string must be of the form hostname or hostname:port if the
first character in the string is a digit.
/Claes
[1] https://tools.ietf.org/html/rfc3986#section-3.1
On 2020-08-18 22:03, serguei.spit...@oracle.com wrote:
> Hi Jie,
>
> I've added Claes to the list as he may have an expertise in this area.
>
>83 *
>84 * {@code } - transformed into "//localhost"
>85 * localhost - transformed into "//localhost"
>86 * hostname - transformed into "//hostname"
>87 * hostname:port - transformed into "//hostname:port"
>88 * proto:hostname - transformed into "proto://hostname"
>89 * proto:hostname:port - transformed into
>90 * "proto://hostname:port"
>91 * proto://hostname:port
>92 *
>
> Is it worth to add an example to the list above?
>
> I wander if this fix needs a CSR.
> How did you check this fix does not introduce any regressions?
>
> Thanks,
> Serguei
>
>
> On 8/17/20 08:13, jiefu(傅杰) wrote:
>>
>> Ping…
>>
>> Any comments?
>>
>> Thanks.
>>
>> Best regards,
>>
>> Jie
>>
>> *From: *serviceability-dev
>> on behalf of "jiefu(傅杰)"
>> *Date: *Friday, August 7, 2020 at 7:44 AM
>> *To: *"serviceability-dev@openjdk.java.net"
>>
>> *Subject: *Re: RFR: 8251155: HostIdentifier fails to canonicalize
>> hostnames starting with digits(Internet mail)
>>
>> FYI:
>>
>> This bug will lead to failures of the following tests on machines
>> with hostname starting from digits.
>>
>> - test/jdk/sun/tools/jstatd/TestJstatdExternalRegistry.java
>>
>> - test/jdk/sun/tools/jstatd/TestJstatdPort.java
>>
>> - test/jdk/sun/tools/jstatd/TestJstatdPortAndServer.java
>>
>> - test/jdk/sun/tools/jstatd/TestJstatdRmiPort.java
>>
>> So it's worth fixing it.
>>
>> Testing:
>>
>> - tier1-3 on Linux/x64
>>
>> Thanks.
>>
>> Best regards,
>>
>> Jie
>>
>> *From: *"jiefu(傅杰)"
>> *Date: *Wednesday, August 5, 2020 at 3:19 PM
>> *To: *"serviceability-dev@openjdk.java.net"
>>
>> *Subject: *RFR: 8251155: HostIdentifier fails to canonicalize
>> hostnames starting with digits
>>
>> Hi all,
>>
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8251155
>>
>> Webrev: http://cr.openjdk.java.net/~jiefu/8251155/webrev.00/
>>
>> HostIdentifier fails to canonicalize hostname:port if the hostname
>> starts with digits.
>>
>> The current implementation will get "scheme = hostname".
>>
>> But the scheme should not be started with digits, which leads to this bug.
>>
>> Thanks a lot.
>>
>> Best regards,
>>
>> Jie
>>
>
Re: 8251155: HostIdentifier fails to canonicalize hostnames starting with digits(Internet mail)
Hi Serguei,
Thanks for your review and help.
Please see comments inline.
From: serguei.spit...@oracle.com
Sent: Wednesday, August 19, 2020 4:03 AM
To: jiefu(傅杰); serviceability-dev@openjdk.java.net; Claes Redestad
Subject: Re: 8251155: HostIdentifier fails to canonicalize hostnames starting
with digits(Internet mail)
83 *
84 * {@code } - transformed into "//localhost"
85 * localhost - transformed into "//localhost"
86 * hostname - transformed into "//hostname"
87 * hostname:port - transformed into "//hostname:port"
88 * proto:hostname - transformed into "proto://hostname"
89 * proto:hostname:port - transformed into
90 * "proto://hostname:port"
91 * proto://hostname:port
92 *
>> Is it worth to add an example to the list above?
Yes. It's really helpful for the review process. Thanks.
>> I wander if this fix needs a CSR.
I don't think so.
This is just a bug fix which doesn't add/remove/change any feature of the tools.
The original design has claimed to support hostname and hostname:port cases.
But it fails to do so when the hostname starts with digits.
It seems to be very common that the hostname will be started with digits in
dockers.
So I think it's worth to fix this bug.
>> How did you check this fix does not introduce any regressions?
In fact, Claes had helped me to answer this question here:
https://mail.openjdk.java.net/pipermail/serviceability-dev/2020-August/032691.html.
Also, I've tested this patch on Linux/x64 with tier1 ~ tier3 (no regression).
Thanks a lot.
Best regards,
Jie
Re: 8251155: HostIdentifier fails to canonicalize hostnames starting with digits(Internet mail)
Ping… Any comments? Thanks. Best regards, Jie From: serviceability-dev on behalf of "jiefu(傅杰)" Date: Friday, August 7, 2020 at 7:44 AM To: "serviceability-dev@openjdk.java.net" Subject: Re: RFR: 8251155: HostIdentifier fails to canonicalize hostnames starting with digits(Internet mail) FYI: This bug will lead to failures of the following tests on machines with hostname starting from digits. - test/jdk/sun/tools/jstatd/TestJstatdExternalRegistry.java - test/jdk/sun/tools/jstatd/TestJstatdPort.java - test/jdk/sun/tools/jstatd/TestJstatdPortAndServer.java - test/jdk/sun/tools/jstatd/TestJstatdRmiPort.java So it's worth fixing it. Testing: - tier1-3 on Linux/x64 Thanks. Best regards, Jie From: "jiefu(傅杰)" Date: Wednesday, August 5, 2020 at 3:19 PM To: "serviceability-dev@openjdk.java.net" Subject: RFR: 8251155: HostIdentifier fails to canonicalize hostnames starting with digits Hi all, JBS:https://bugs.openjdk.java.net/browse/JDK-8251155 Webrev: http://cr.openjdk.java.net/~jiefu/8251155/webrev.00/ HostIdentifier fails to canonicalize hostname:port if the hostname starts with digits. The current implementation will get "scheme = hostname". But the scheme should not be started with digits, which leads to this bug. Thanks a lot. Best regards, Jie
Re: RFR: 8251155: HostIdentifier fails to canonicalize hostnames starting with digits
FYI: This bug will lead to failures of the following tests on machines with hostname starting from digits. - test/jdk/sun/tools/jstatd/TestJstatdExternalRegistry.java - test/jdk/sun/tools/jstatd/TestJstatdPort.java - test/jdk/sun/tools/jstatd/TestJstatdPortAndServer.java - test/jdk/sun/tools/jstatd/TestJstatdRmiPort.java So it's worth fixing it. Testing: - tier1-3 on Linux/x64 Thanks. Best regards, Jie From: "jiefu(傅杰)" Date: Wednesday, August 5, 2020 at 3:19 PM To: "serviceability-dev@openjdk.java.net" Subject: RFR: 8251155: HostIdentifier fails to canonicalize hostnames starting with digits Hi all, JBS:https://bugs.openjdk.java.net/browse/JDK-8251155 Webrev: http://cr.openjdk.java.net/~jiefu/8251155/webrev.00/ HostIdentifier fails to canonicalize hostname:port if the hostname starts with digits. The current implementation will get "scheme = hostname". But the scheme should not be started with digits, which leads to this bug. Thanks a lot. Best regards, Jie
RFR: 8251155: HostIdentifier fails to canonicalize hostnames starting with digits
Hi all, JBS:https://bugs.openjdk.java.net/browse/JDK-8251155 Webrev: http://cr.openjdk.java.net/~jiefu/8251155/webrev.00/ HostIdentifier fails to canonicalize hostname:port if the hostname starts with digits. The current implementation will get "scheme = hostname". But the scheme should not be started with digits, which leads to this bug. Thanks a lot. Best regards, Jie
Re: FW: 8251031: Some vmTestbase/nsk/monitoring/RuntimeMXBean tests fail with hostnames starting from digits(Internet mail)
Thanks Serguei for your review. I'll try to split it. Best regards, Jie From: serguei.spit...@oracle.com Sent: Wednesday, August 5, 2020 10:10 AM To: jiefu(傅杰); chris.plum...@oracle.com; David Holmes Cc: serviceability-dev@openjdk.java.net Subject: Re: FW: 8251031: Some vmTestbase/nsk/monitoring/RuntimeMXBean tests fail with hostnames starting from digits(Internet mail) Hi Jie, Could you, please, split the format string in two shorter lines? Otherwise, it looks okay. There is no need in another webrev. Thanks, Serguei On 8/4/20 18:38, jiefu(傅杰) wrote: Thanks Chris and David for your review. Will push it later. Best regards, Jie From: David Holmes <mailto:david.hol...@oracle.com> Sent: Wednesday, August 5, 2020 9:03 AM To: jiefu(傅杰); serviceability-dev@openjdk.java.net<mailto:serviceability-dev@openjdk.java.net> Subject: Re: FW: 8251031: Some vmTestbase/nsk/monitoring/RuntimeMXBean tests fail with hostnames starting from digits(Internet mail) My Review still stands. :) Thanks, David On 5/08/2020 1:10 am, jiefu(傅杰) wrote: > Forward it to serviceability-dev since this issue in the JBS has been > moved from hotspot/runtime to core-svc/java.lang.management. > > Please review it. > > Thanks. > > Best regards, > > Jie > > *From: *"jiefu(傅杰)" <mailto:ji...@tencent.com> > *Date: *Tuesday, August 4, 2020 at 5:10 PM > *To: > *"hotspot-runtime-...@openjdk.java.net"<mailto:hotspot-runtime-...@openjdk.java.net> > <mailto:hotspot-runtime-...@openjdk.java.net> > *Subject: *RFR: 8251031: Some vmTestbase/nsk/monitoring/RuntimeMXBean > tests fail with hostnames starting from digits > > Hi all, > > JBS:https://bugs.openjdk.java.net/browse/JDK-8251031 > > Webrev: http://cr.openjdk.java.net/~jiefu/8251031/webrev.00/ > > Some vmTestbase/nsk/monitoring/RuntimeMXBean tests failed in our test > infrastructure. > > The reason is that these tests reject hostnames starting with digits. > > However, hostnames starting from digits are actually valid according to > RFC1123 [1][2]. > > It would be better to fix it. > > Thanks a lot. > > Best regards, > > Jie > > [1] https://tools.ietf.org/html/rfc1123#page-13 > > [2] https://en.wikipedia.org/wiki/Hostname >
Re: FW: 8251031: Some vmTestbase/nsk/monitoring/RuntimeMXBean tests fail with hostnames starting from digits(Internet mail)
Thanks Chris and David for your review. Will push it later. Best regards, Jie From: David Holmes Sent: Wednesday, August 5, 2020 9:03 AM To: jiefu(傅杰); serviceability-dev@openjdk.java.net Subject: Re: FW: 8251031: Some vmTestbase/nsk/monitoring/RuntimeMXBean tests fail with hostnames starting from digits(Internet mail) My Review still stands. :) Thanks, David On 5/08/2020 1:10 am, jiefu(傅杰) wrote: > Forward it to serviceability-dev since this issue in the JBS has been > moved from hotspot/runtime to core-svc/java.lang.management. > > Please review it. > > Thanks. > > Best regards, > > Jie > > *From: *"jiefu(傅杰)" > *Date: *Tuesday, August 4, 2020 at 5:10 PM > *To: *"hotspot-runtime-...@openjdk.java.net" > > *Subject: *RFR: 8251031: Some vmTestbase/nsk/monitoring/RuntimeMXBean > tests fail with hostnames starting from digits > > Hi all, > > JBS:https://bugs.openjdk.java.net/browse/JDK-8251031 > > Webrev: http://cr.openjdk.java.net/~jiefu/8251031/webrev.00/ > > Some vmTestbase/nsk/monitoring/RuntimeMXBean tests failed in our test > infrastructure. > > The reason is that these tests reject hostnames starting with digits. > > However, hostnames starting from digits are actually valid according to > RFC1123 [1][2]. > > It would be better to fix it. > > Thanks a lot. > > Best regards, > > Jie > > [1] https://tools.ietf.org/html/rfc1123#page-13 > > [2] https://en.wikipedia.org/wiki/Hostname >
FW: 8251031: Some vmTestbase/nsk/monitoring/RuntimeMXBean tests fail with hostnames starting from digits
Forward it to serviceability-dev since this issue in the JBS has been moved from hotspot/runtime to core-svc/java.lang.management. Please review it. Thanks. Best regards, Jie From: "jiefu(傅杰)" Date: Tuesday, August 4, 2020 at 5:10 PM To: "hotspot-runtime-...@openjdk.java.net" Subject: RFR: 8251031: Some vmTestbase/nsk/monitoring/RuntimeMXBean tests fail with hostnames starting from digits Hi all, JBS:https://bugs.openjdk.java.net/browse/JDK-8251031 Webrev: http://cr.openjdk.java.net/~jiefu/8251031/webrev.00/ Some vmTestbase/nsk/monitoring/RuntimeMXBean tests failed in our test infrastructure. The reason is that these tests reject hostnames starting with digits. However, hostnames starting from digits are actually valid according to RFC1123 [1][2]. It would be better to fix it. Thanks a lot. Best regards, Jie [1] https://tools.ietf.org/html/rfc1123#page-13 [2] https://en.wikipedia.org/wiki/Hostname
Re: 8242480: Negative value may be returned by getFreeSwapSpaceSize() in the docker(Internet mail)
Thanks Severin and David for your review.
Will push it tomorrow.
Best regards,
Jie
On 2020/4/17, 8:56 PM, "David Holmes" wrote:
On 17/04/2020 5:00 pm, jiefu(傅杰) wrote:
> Hi David,
>
> Updated: http://cr.openjdk.java.net/~jiefu/8242480/webrev.02/
>
> The file header had been fixed. Please review it.
File header update looks good.
Thanks,
David
> Thanks a lot.
> Best regards,
> Jie
>
> On 2020/4/17, 11:59 AM, "David Holmes" wrote:
>
> Hi Jie,
>
> On 16/04/2020 11:23 pm, jiefu(傅杰) wrote:
> > Hi Severin,
> >
> > Thanks for your review and very nice suggestions.
> >
> > Updated: http://cr.openjdk.java.net/~jiefu/8242480/webrev.01/
> >
> > test/hotspot/jtreg/containers/docker/TestGetFreeSwapSpaceSize.java
is added to reproduce the bug.
>
>
> Can you please use the standard OpenJDK file header after your
Tencent
> copyright line:
>
>* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>*
>* This code is free software; you can redistribute it and/or
modify it
>* under the terms of the GNU General Public License version 2
only, as
>* published by the Free Software Foundation.
>*
>* This code is distributed in the hope that it will be useful, but
WITHOUT
>* ANY WARRANTY; without even the implied warranty of
MERCHANTABILITY or
>* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public
License
>* version 2 for more details (a copy is included in the LICENSE
file that
>* accompanied this code).
>*
>* You should have received a copy of the GNU General Public License
> version
>* 2 along with this work; if not, write to the Free Software
Foundation,
>* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
>*
>* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA
94065 USA
>* or visit www.oracle.com if you need additional information or
have any
>* questions.
>*/
>
> I don't think the "classpath exception" is relevant to tests -
certainly
> other tests I checked do not have it.
>
> Thanks,
> David
> -
>
> > Thanks a lot.
> > Best regards,
> > Jie
> >
> >
> > On 2020/4/16, 4:40 PM, "Severin Gehwolf"
wrote:
> >
> > Hi Jie,
> >
> > On Fri, 2020-04-10 at 01:49 +, jiefu(傅杰) wrote:
> > > Hi all,
> > >
> > > JBS:https://bugs.openjdk.java.net/browse/JDK-8242480
> > > Webrev: http://cr.openjdk.java.net/~jiefu/8242480/webrev.00/
> > >
> > > Negative values were returned by getFreeSwapSpaceSize() in
our docker testing.
> > > The reason is that current implementation doesn't consider
the situation when memory.limit_in_bytes == memory.memsw.limit_in_bytes, which
means do not use the swap space at all.
> > >
> > > In
src/jdk.management/unix/classes/com/sun/management/internal/OperatingSystemImpl.java,
let's see
> > >
> > > 71 public long getFreeSwapSpaceSize() {
> > > 72 if (containerMetrics != null) {
> > > 73 long memSwapLimit =
containerMetrics.getMemoryAndSwapLimit();
> > > 74 long memLimit =
containerMetrics.getMemoryLimit();
> > > 75 if (memSwapLimit >= 0 && memLimit >= 0) {
> > > 76 for (int attempt = 0; attempt <
MAX_ATTEMPTS_NUMBER; attempt++) {
> > > 77 long memSwapUsage =
containerMetrics.getMemoryAndSwapUsage();
> > > 78 long memUsage =
containerMetrics.getMemoryUsage();
> > > 79 if (memSwapUsage > 0 && memUsage >
0) {
> > > 80 // We read "memory usage" and
"memory and swap usa
Re: 8242480: Negative value may be returned by getFreeSwapSpaceSize() in the docker(Internet mail)
Hi David,
Updated: http://cr.openjdk.java.net/~jiefu/8242480/webrev.02/
The file header had been fixed. Please review it.
Thanks a lot.
Best regards,
Jie
On 2020/4/17, 11:59 AM, "David Holmes" wrote:
Hi Jie,
On 16/04/2020 11:23 pm, jiefu(傅杰) wrote:
> Hi Severin,
>
> Thanks for your review and very nice suggestions.
>
> Updated: http://cr.openjdk.java.net/~jiefu/8242480/webrev.01/
>
> test/hotspot/jtreg/containers/docker/TestGetFreeSwapSpaceSize.java is
added to reproduce the bug.
Can you please use the standard OpenJDK file header after your Tencent
copyright line:
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License
version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/
I don't think the "classpath exception" is relevant to tests - certainly
other tests I checked do not have it.
Thanks,
David
-
> Thanks a lot.
> Best regards,
> Jie
>
>
> On 2020/4/16, 4:40 PM, "Severin Gehwolf" wrote:
>
> Hi Jie,
>
> On Fri, 2020-04-10 at 01:49 +, jiefu(傅杰) wrote:
> > Hi all,
> >
> > JBS:https://bugs.openjdk.java.net/browse/JDK-8242480
> > Webrev: http://cr.openjdk.java.net/~jiefu/8242480/webrev.00/
> >
> > Negative values were returned by getFreeSwapSpaceSize() in our
docker testing.
> > The reason is that current implementation doesn't consider the
situation when memory.limit_in_bytes == memory.memsw.limit_in_bytes, which
means do not use the swap space at all.
> >
> > In
src/jdk.management/unix/classes/com/sun/management/internal/OperatingSystemImpl.java,
let's see
> >
> > 71 public long getFreeSwapSpaceSize() {
> > 72 if (containerMetrics != null) {
> > 73 long memSwapLimit =
containerMetrics.getMemoryAndSwapLimit();
> > 74 long memLimit = containerMetrics.getMemoryLimit();
> > 75 if (memSwapLimit >= 0 && memLimit >= 0) {
> > 76 for (int attempt = 0; attempt <
MAX_ATTEMPTS_NUMBER; attempt++) {
> > 77 long memSwapUsage =
containerMetrics.getMemoryAndSwapUsage();
> > 78 long memUsage =
containerMetrics.getMemoryUsage();
> > 79 if (memSwapUsage > 0 && memUsage > 0) {
> > 80 // We read "memory usage" and "memory
and swap usage" not atomically,
> > 81 // and it's possible to get the
negative value when subtracting these two.
> > 82 // If this happens just retry the loop
for a few iterations.
> > 83 if ((memSwapUsage - memUsage) >= 0) {
> > 84 return memSwapLimit - memLimit -
(memSwapUsage - memUsage);
> > 85 }
> > 86 }
> > 87 }
> > 88 }
> > 89 }
> > 90 return getFreeSwapSpaceSize0();
> > 91 }
> >
> > If memSwapLimit (@line 73) equals memLimit (@line 74), then
getFreeSwapSpaceSize() may return a negative value @line 84.
> >
> > It would be better to fix it.
> > Could you please review it and give me some advice?
>
> Would this be reproducible via a test? There is
> test/hotspot/jtreg/containers/docker/TestMemoryAwareness.java which
> contains testOperatingSystemMXBeanAwareness() tests.
>
> It would be good to capture this in a test somehow.
>
> Thanks,
> Severin
>
>
>
>
Re: 8242480: Negative value may be returned by getFreeSwapSpaceSize() in the docker(Internet mail)
Hi Severin, Updated: http://cr.openjdk.java.net/~jiefu/8242480/webrev.02/ Please review it. Thanks a lot. Best regards, Jie On 2020/4/16, 11:40 PM, "Severin Gehwolf" wrote: Since you've added a new test, please move them to the jdk docker tests in: test/jdk/jdk/internal/platform/docker/ Fixed. test/hotspot/jtreg/containers/docker/TestGetFreeSwapSpaceSize.java + * @build sun.hotspot.WhiteBox GetFreeSwapSpaceSize + * @run driver ClassFileInstaller -jar whitebox.jar sun.hotspot.WhiteBox sun.hotspot.WhiteBox$WhiteBoxPermission I don't see any reason why WhiteBox would be needed for this test. Is that an oversight or am I missing something? It's my oversight. Thanks for correcting me.
Re: 8242480: Negative value may be returned by getFreeSwapSpaceSize() in the docker(Internet mail)
Hi Severin,
Thanks for your review and very nice suggestions.
Updated: http://cr.openjdk.java.net/~jiefu/8242480/webrev.01/
test/hotspot/jtreg/containers/docker/TestGetFreeSwapSpaceSize.java is added to
reproduce the bug.
Thanks a lot.
Best regards,
Jie
On 2020/4/16, 4:40 PM, "Severin Gehwolf" wrote:
Hi Jie,
On Fri, 2020-04-10 at 01:49 +0000, jiefu(傅杰) wrote:
> Hi all,
>
> JBS:https://bugs.openjdk.java.net/browse/JDK-8242480
> Webrev: http://cr.openjdk.java.net/~jiefu/8242480/webrev.00/
>
> Negative values were returned by getFreeSwapSpaceSize() in our docker
testing.
> The reason is that current implementation doesn't consider the situation
when memory.limit_in_bytes == memory.memsw.limit_in_bytes, which means do not
use the swap space at all.
>
> In
src/jdk.management/unix/classes/com/sun/management/internal/OperatingSystemImpl.java,
let's see
>
> 71 public long getFreeSwapSpaceSize() {
> 72 if (containerMetrics != null) {
> 73 long memSwapLimit =
containerMetrics.getMemoryAndSwapLimit();
> 74 long memLimit = containerMetrics.getMemoryLimit();
> 75 if (memSwapLimit >= 0 && memLimit >= 0) {
> 76 for (int attempt = 0; attempt < MAX_ATTEMPTS_NUMBER;
attempt++) {
> 77 long memSwapUsage =
containerMetrics.getMemoryAndSwapUsage();
> 78 long memUsage = containerMetrics.getMemoryUsage();
> 79 if (memSwapUsage > 0 && memUsage > 0) {
> 80 // We read "memory usage" and "memory and
swap usage" not atomically,
> 81 // and it's possible to get the negative
value when subtracting these two.
> 82 // If this happens just retry the loop for a
few iterations.
> 83 if ((memSwapUsage - memUsage) >= 0) {
> 84 return memSwapLimit - memLimit -
(memSwapUsage - memUsage);
> 85 }
> 86 }
> 87 }
> 88 }
> 89 }
> 90 return getFreeSwapSpaceSize0();
> 91 }
>
> If memSwapLimit (@line 73) equals memLimit (@line 74), then
getFreeSwapSpaceSize() may return a negative value @line 84.
>
> It would be better to fix it.
> Could you please review it and give me some advice?
Would this be reproducible via a test? There is
test/hotspot/jtreg/containers/docker/TestMemoryAwareness.java which
contains testOperatingSystemMXBeanAwareness() tests.
It would be good to capture this in a test somehow.
Thanks,
Severin
RFR: 8242480: Negative value may be returned by getFreeSwapSpaceSize() in the docker
Hi all,
JBS:https://bugs.openjdk.java.net/browse/JDK-8242480
Webrev: http://cr.openjdk.java.net/~jiefu/8242480/webrev.00/
Negative values were returned by getFreeSwapSpaceSize() in our docker testing.
The reason is that current implementation doesn't consider the situation when
memory.limit_in_bytes == memory.memsw.limit_in_bytes, which means do not use
the swap space at all.
In
src/jdk.management/unix/classes/com/sun/management/internal/OperatingSystemImpl.java,
let's see
71 public long getFreeSwapSpaceSize() {
72 if (containerMetrics != null) {
73 long memSwapLimit = containerMetrics.getMemoryAndSwapLimit();
74 long memLimit = containerMetrics.getMemoryLimit();
75 if (memSwapLimit >= 0 && memLimit >= 0) {
76 for (int attempt = 0; attempt < MAX_ATTEMPTS_NUMBER;
attempt++) {
77 long memSwapUsage =
containerMetrics.getMemoryAndSwapUsage();
78 long memUsage = containerMetrics.getMemoryUsage();
79 if (memSwapUsage > 0 && memUsage > 0) {
80 // We read "memory usage" and "memory and swap
usage" not atomically,
81 // and it's possible to get the negative value when
subtracting these two.
82 // If this happens just retry the loop for a few
iterations.
83 if ((memSwapUsage - memUsage) >= 0) {
84 return memSwapLimit - memLimit - (memSwapUsage
- memUsage);
85 }
86 }
87 }
88 }
89 }
90 return getFreeSwapSpaceSize0();
91 }
If memSwapLimit (@line 73) equals memLimit (@line 74), then
getFreeSwapSpaceSize() may return a negative value @line 84.
It would be better to fix it.
Could you please review it and give me some advice?
Thanks a lot.
Best regards,
Jie
