Re: [TestBug] RFR : JDK-8192909 - Invalid username or password in HashedPasswordFileTest.java

2017-12-05 Thread Harsha Wardhana B 

Thanks Daniel and Christoph for review.

-Harsha

On Tuesday 05 December 2017 08:11 PM, Daniel Fuchs wrote:

+1

-- daniel

On 05/12/2017 12:04, Harsha Wardhana B wrote:

Hi Daniel,


On Tuesday 05 December 2017 03:42 PM, Daniel Fuchs wrote:

Hi Harsha,

Looks good.

Thanks for the review.


nit:

 366 if(random.nextBoolean()) {
 367 String[] tokens = line.split("\\s+");
 368 if ((tokens.length == 4 || tokens.length == 
3)) {


inverting the two if () (testing for the applicability of the line
first) would probably give a better chance that an existing
password is replaced, unless most lines are applicable.
All the lines will be applicable. The password file will be hashed 
before the above lines are executed. An Assert statement at line 353 
makes sure of that. Hence no point inverting the two if().


best regards,

-- daniel

Regards
Harsha


n 04/12/2017 18:27, Harsha Wardhana B wrote:

Hi All,

Please review and provide comments for fix for,

issue: https://bugs.openjdk.java.net/browse/JDK-8192909

having webrev at,

webrev : http://cr.openjdk.java.net/~hb/8192909/webrev.00/

Fix details: The test was failing intermittently because of 
duplicate entries for role in input password file. The duplicate 
entries get over-written by JMX agent, but the client was testing 
with stale entries for duplicated role. Also, the test now uses a 
single random number generator from test package 
(Utils.getRandomInstance) instead of two.


Regards

Harsha

Re: [TestBug] RFR : JDK-8192909 - Invalid username or password in HashedPasswordFileTest.java

2017-12-05 Thread Daniel Fuchs 

+1

-- daniel

On 05/12/2017 12:04, Harsha Wardhana B wrote:

Hi Daniel,


On Tuesday 05 December 2017 03:42 PM, Daniel Fuchs wrote:

Hi Harsha,

Looks good.

Thanks for the review.


nit:

 366 if(random.nextBoolean()) {
 367 String[] tokens = line.split("\\s+");
 368 if ((tokens.length == 4 || tokens.length == 
3)) {


inverting the two if () (testing for the applicability of the line
first) would probably give a better chance that an existing
password is replaced, unless most lines are applicable.
All the lines will be applicable. The password file will be hashed 
before the above lines are executed. An Assert statement at line 353 
makes sure of that. Hence no point inverting the two if().


best regards,

-- daniel

Regards
Harsha


n 04/12/2017 18:27, Harsha Wardhana B wrote:

Hi All,

Please review and provide comments for fix for,

issue: https://bugs.openjdk.java.net/browse/JDK-8192909

having webrev at,

webrev : http://cr.openjdk.java.net/~hb/8192909/webrev.00/

Fix details: The test was failing intermittently because of duplicate 
entries for role in input password file. The duplicate entries get 
over-written by JMX agent, but the client was testing with stale 
entries for duplicated role. Also, the test now uses a single random 
number generator from test package (Utils.getRandomInstance) instead 
of two.


Regards

Harsha

Re: [TestBug] RFR : JDK-8192909 - Invalid username or password in HashedPasswordFileTest.java

2017-12-05 Thread Harsha Wardhana B 

Hi Daniel,


On Tuesday 05 December 2017 03:42 PM, Daniel Fuchs wrote:

Hi Harsha,

Looks good.

Thanks for the review.


nit:

 366 if(random.nextBoolean()) {
 367 String[] tokens = line.split("\\s+");
 368 if ((tokens.length == 4 || tokens.length == 
3)) {


inverting the two if () (testing for the applicability of the line
first) would probably give a better chance that an existing
password is replaced, unless most lines are applicable.
All the lines will be applicable. The password file will be hashed 
before the above lines are executed. An Assert statement at line 353 
makes sure of that. Hence no point inverting the two if().


best regards,

-- daniel

Regards
Harsha


n 04/12/2017 18:27, Harsha Wardhana B wrote:

Hi All,

Please review and provide comments for fix for,

issue: https://bugs.openjdk.java.net/browse/JDK-8192909

having webrev at,

webrev : http://cr.openjdk.java.net/~hb/8192909/webrev.00/

Fix details: The test was failing intermittently because of duplicate 
entries for role in input password file. The duplicate entries get 
over-written by JMX agent, but the client was testing with stale 
entries for duplicated role. Also, the test now uses a single random 
number generator from test package (Utils.getRandomInstance) instead 
of two.


Regards

Harsha

Re: [TestBug] RFR : JDK-8192909 - Invalid username or password in HashedPasswordFileTest.java

2017-12-05 Thread Daniel Fuchs 

Hi Harsha,

Looks good.

nit:

 366 if(random.nextBoolean()) {
 367 String[] tokens = line.split("\\s+");
 368 if ((tokens.length == 4 || tokens.length == 3)) {

inverting the two if () (testing for the applicability of the line
first) would probably give a better chance that an existing
password is replaced, unless most lines are applicable.

best regards,

-- daniel

n 04/12/2017 18:27, Harsha Wardhana B wrote:

Hi All,

Please review and provide comments for fix for,

issue: https://bugs.openjdk.java.net/browse/JDK-8192909

having webrev at,

webrev : http://cr.openjdk.java.net/~hb/8192909/webrev.00/

Fix details: The test was failing intermittently because of duplicate 
entries for role in input password file. The duplicate entries get 
over-written by JMX agent, but the client was testing with stale entries 
for duplicated role. Also, the test now uses a single random number 
generator from test package (Utils.getRandomInstance) instead of two.


Regards

Harsha

Re: [TestBug] RFR : JDK-8192909 - Invalid username or password in HashedPasswordFileTest.java

2017-12-04 Thread Harsha Wardhana B 

Thanks Christoph for the review.

Can I get one more review please?

-Harsha

On Tuesday 05 December 2017 11:29 AM, Langer, Christoph wrote:

Hi Harsha,

this looks good to me.

Small finding: Line 366, there could be a space between if and (.

Best regards
Christoph


-Original Message-
From: serviceability-dev [mailto:serviceability-dev-
boun...@openjdk.java.net] On Behalf Of Harsha Wardhana B
Sent: Montag, 4. Dezember 2017 19:28
To: serviceability-dev@openjdk.java.net
Subject: [TestBug] RFR : JDK-8192909 - Invalid username or password in
HashedPasswordFileTest.java

Hi All,

Please review and provide comments for fix for,

issue: https://bugs.openjdk.java.net/browse/JDK-8192909

having webrev at,

webrev : http://cr.openjdk.java.net/~hb/8192909/webrev.00/

Fix details: The test was failing intermittently because of duplicate
entries for role in input password file. The duplicate entries get
over-written by JMX agent, but the client was testing with stale entries
for duplicated role. Also, the test now uses a single random number
generator from test package (Utils.getRandomInstance) instead of two.

Regards

Harsha

RE: [TestBug] RFR : JDK-8192909 - Invalid username or password in HashedPasswordFileTest.java

2017-12-04 Thread Langer, Christoph 
Hi Harsha,

this looks good to me.

Small finding: Line 366, there could be a space between if and (.

Best regards
Christoph

> -Original Message-
> From: serviceability-dev [mailto:serviceability-dev-
> boun...@openjdk.java.net] On Behalf Of Harsha Wardhana B
> Sent: Montag, 4. Dezember 2017 19:28
> To: serviceability-dev@openjdk.java.net
> Subject: [TestBug] RFR : JDK-8192909 - Invalid username or password in
> HashedPasswordFileTest.java
> 
> Hi All,
> 
> Please review and provide comments for fix for,
> 
> issue: https://bugs.openjdk.java.net/browse/JDK-8192909
> 
> having webrev at,
> 
> webrev : http://cr.openjdk.java.net/~hb/8192909/webrev.00/
> 
> Fix details: The test was failing intermittently because of duplicate
> entries for role in input password file. The duplicate entries get
> over-written by JMX agent, but the client was testing with stale entries
> for duplicated role. Also, the test now uses a single random number
> generator from test package (Utils.getRandomInstance) instead of two.
> 
> Regards
> 
> Harsha

[TestBug] RFR : JDK-8192909 - Invalid username or password in HashedPasswordFileTest.java

2017-12-04 Thread Harsha Wardhana B 

Hi All,

Please review and provide comments for fix for,

issue: https://bugs.openjdk.java.net/browse/JDK-8192909

having webrev at,

webrev : http://cr.openjdk.java.net/~hb/8192909/webrev.00/

Fix details: The test was failing intermittently because of duplicate 
entries for role in input password file. The duplicate entries get 
over-written by JMX agent, but the client was testing with stale entries 
for duplicated role. Also, the test now uses a single random number 
generator from test package (Utils.getRandomInstance) instead of two.


Regards

Harsha