Re: RFR: 7165755 OS Information much longer on linux than other platforms

[Nils Loodin]

Updated this with pulling out some shared code to os_posix.cpp.
Some methods were different enough that this wasn't possible though.

Do you like this better?

Webrev:
http://cr.openjdk.java.net/~nloodin/7165755/webrev.00/

On May 2, 2012, at 14:08 , Nils Loodin wrote:

> 
> On May 2, 2012, at 14:00 , David Holmes wrote:
> 
>> Hi Nils,
>> 
>> On 2/05/2012 9:56 PM, Nils Loodin wrote:
 Ignoring Windows (with prints very little) I'd say it is the printing of 
 /proc/meminfo that is the main difference. Not sure why printing that was 
 necessary ... but if we are going to remove it I think we need to know why 
 it was added.
>>> Yes, that's the reason.
>>> Note that nothing is removed. The method still prints exactly the same 
>>> info, but I introduced another method to print briefer info, to be kinder 
>>> to tool developers.
>> 
>> The current one prints /proc/meminfo. You turned that code into 
>> print_full_memory_info but in the main routine you call print_memory_info. 
>> Was that a mistake?
> 
> YES! Glad you caught that :) Guess (or hope) it would have been caught in 
> dump testing otherwise :)
> 
> Regards,
> Nils Loodin
> 
> 
>> 
>> David
>> 
>>> I really don't want to change the output for say, hs_err files, where I 
>>> believe this info is used.
>>> 
 
> This can make it hard for tool writers to get a summary that look good 
> and similar for multiple platforms (sizing of gui fields, having to parse 
> info in the tool code etc)
> Lookin at the code, it's in some serious need of refactoring. It would be 
> nice with a method to get a "brief" os info for these kinds of tools that 
> looks similar on all platforms.
> 
> This is my suggested change:
> http://cr.openjdk.java.net/~nloodin/7165755/webrev.00/
 
 Seems to me some of this could be factored into the top-level OS class if 
 we shoehorn Windows into the same shape as the other OSes ;-)
>>> This was my first attempt also, but then a lot of empty windows-methods 
>>> ensued, which was kind of ugly.
>>> 
 Or at least perhaps put some of the common stuff into os_posix.cpp ?
>>> There's a thought!
>>> I'll investigate that route, it could get things to look nicer.
>>> 
>>> 
 Cheers,
 David
>>> Regards,
>>> Nils Loodin
>>> 
> 

RFR - BUG7163117: SA Agent can't connect to process on Mac OSX

[Nils Loodin]

looking for cpu-arch "amd64" only, and not "x86_64" as mac osx is using 
nowadays.

http://cr.openjdk.java.net/~nloodin/7163117/webrev.00/



Need reviews and a  sponsor!
Regards,
Nils Loodin

Re: RFR: 7165755 OS Information much longer on linux than other platforms

[Dmitry Samersoff]

Nils,

os_bsd.cpp:

2359:   // Print warning if unsafe chroot environment detected
constant below is
unstable_chroot_error

2373:  Never heard about neither about /etc/XXX-release on BSD nor
   about mandrake or redhat bsd.
   Please fix it as well.

2409:  st->print(os::Bsd::glibc_version())
   glibc_version() looks a little bit weird as
   none of BSD has glibc.


-Dmitry



On 2012-05-04 12:26, Nils Loodin wrote:
> Updated this with pulling out some shared code to os_posix.cpp.
> Some methods were different enough that this wasn't possible though.
> 
> Do you like this better?
> 
> Webrev:
> http://cr.openjdk.java.net/~nloodin/7165755/webrev.00/
> 
> On May 2, 2012, at 14:08 , Nils Loodin wrote:
> 
>>
>> On May 2, 2012, at 14:00 , David Holmes wrote:
>>
>>> Hi Nils,
>>>
>>> On 2/05/2012 9:56 PM, Nils Loodin wrote:
> Ignoring Windows (with prints very little) I'd say it is the printing of 
> /proc/meminfo that is the main difference. Not sure why printing that was 
> necessary ... but if we are going to remove it I think we need to know 
> why it was added.
 Yes, that's the reason.
 Note that nothing is removed. The method still prints exactly the same 
 info, but I introduced another method to print briefer info, to be kinder 
 to tool developers.
>>>
>>> The current one prints /proc/meminfo. You turned that code into 
>>> print_full_memory_info but in the main routine you call print_memory_info. 
>>> Was that a mistake?
>>
>> YES! Glad you caught that :) Guess (or hope) it would have been caught in 
>> dump testing otherwise :)
>>
>> Regards,
>> Nils Loodin
>>
>>
>>>
>>> David
>>>
 I really don't want to change the output for say, hs_err files, where I 
 believe this info is used.

>
>> This can make it hard for tool writers to get a summary that look good 
>> and similar for multiple platforms (sizing of gui fields, having to 
>> parse info in the tool code etc)
>> Lookin at the code, it's in some serious need of refactoring. It would 
>> be nice with a method to get a "brief" os info for these kinds of tools 
>> that looks similar on all platforms.
>>
>> This is my suggested change:
>> http://cr.openjdk.java.net/~nloodin/7165755/webrev.00/
>
> Seems to me some of this could be factored into the top-level OS class if 
> we shoehorn Windows into the same shape as the other OSes ;-)
 This was my first attempt also, but then a lot of empty windows-methods 
 ensued, which was kind of ugly.

> Or at least perhaps put some of the common stuff into os_posix.cpp ?
 There's a thought!
 I'll investigate that route, it could get things to look nicer.


> Cheers,
> David
 Regards,
 Nils Loodin

>>
> 


-- 
Dmitry Samersoff
Java Hotspot development team, SPB04
* There will come soft rains ...

Re: RFR - BUG7163117: SA Agent can't connect to process on Mac OSX

[Staffan Larsen]

Looks good. I can sponsor the push.

What kind of testing have you done?

/Staffan

On 4 maj 2012, at 11:28, Nils Loodin wrote:

> looking for cpu-arch "amd64" only, and not "x86_64" as mac osx is using 
> nowadays.
> 
> http://cr.openjdk.java.net/~nloodin/7163117/webrev.00/
> 
> 
> 
> Need reviews and a  sponsor!
> Regards,
> Nils Loodin

Re: RFR: 7165755 OS Information much longer on linux than other platforms

[Staffan Larsen]

Note that there is _lots_ of old cruft in os_bsd.cpp that are leftovers from 
the bsd port and needs to be cleaned up. Basically the file is a copy of 
os_linux.cpp with things removed by #ifdefs. 

I don't think it is in the scope of Nils' current work to do this cleanup.

/Staffan

On 4 maj 2012, at 11:41, Dmitry Samersoff wrote:

> Nils,
> 
> os_bsd.cpp:
> 
> 2359:   // Print warning if unsafe chroot environment detected
> constant below is
>unstable_chroot_error
> 
> 2373:  Never heard about neither about /etc/XXX-release on BSD nor
>   about mandrake or redhat bsd.
>   Please fix it as well.
> 
> 2409:  st->print(os::Bsd::glibc_version())
>   glibc_version() looks a little bit weird as
>   none of BSD has glibc.
> 
> 
> -Dmitry
> 
> 
> 
> On 2012-05-04 12:26, Nils Loodin wrote:
>> Updated this with pulling out some shared code to os_posix.cpp.
>> Some methods were different enough that this wasn't possible though.
>> 
>> Do you like this better?
>> 
>> Webrev:
>> http://cr.openjdk.java.net/~nloodin/7165755/webrev.00/
>> 
>> On May 2, 2012, at 14:08 , Nils Loodin wrote:
>> 
>>> 
>>> On May 2, 2012, at 14:00 , David Holmes wrote:
>>> 
 Hi Nils,
 
 On 2/05/2012 9:56 PM, Nils Loodin wrote:
>> Ignoring Windows (with prints very little) I'd say it is the printing of 
>> /proc/meminfo that is the main difference. Not sure why printing that 
>> was necessary ... but if we are going to remove it I think we need to 
>> know why it was added.
> Yes, that's the reason.
> Note that nothing is removed. The method still prints exactly the same 
> info, but I introduced another method to print briefer info, to be kinder 
> to tool developers.
 
 The current one prints /proc/meminfo. You turned that code into 
 print_full_memory_info but in the main routine you call print_memory_info. 
 Was that a mistake?
>>> 
>>> YES! Glad you caught that :) Guess (or hope) it would have been caught in 
>>> dump testing otherwise :)
>>> 
>>> Regards,
>>> Nils Loodin
>>> 
>>> 
 
 David
 
> I really don't want to change the output for say, hs_err files, where I 
> believe this info is used.
> 
>> 
>>> This can make it hard for tool writers to get a summary that look good 
>>> and similar for multiple platforms (sizing of gui fields, having to 
>>> parse info in the tool code etc)
>>> Lookin at the code, it's in some serious need of refactoring. It would 
>>> be nice with a method to get a "brief" os info for these kinds of tools 
>>> that looks similar on all platforms.
>>> 
>>> This is my suggested change:
>>> http://cr.openjdk.java.net/~nloodin/7165755/webrev.00/
>> 
>> Seems to me some of this could be factored into the top-level OS class 
>> if we shoehorn Windows into the same shape as the other OSes ;-)
> This was my first attempt also, but then a lot of empty windows-methods 
> ensued, which was kind of ugly.
> 
>> Or at least perhaps put some of the common stuff into os_posix.cpp ?
> There's a thought!
> I'll investigate that route, it could get things to look nicer.
> 
> 
>> Cheers,
>> David
> Regards,
> Nils Loodin
> 
>>> 
>> 
> 
> 
> -- 
> Dmitry Samersoff
> Java Hotspot development team, SPB04
> * There will come soft rains ...

Re: RFR: 7165755 OS Information much longer on linux than other platforms

[Dmitry Samersoff]

Staffan,

OK.

-Dmitry


On 2012-05-04 14:11, Staffan Larsen wrote:
> Note that there is _lots_ of old cruft in os_bsd.cpp that are leftovers from 
> the bsd port and needs to be cleaned up. 
> Basically the file is a copy of os_linux.cpp with things removed by #ifdefs. 


> 
> I don't think it is in the scope of Nils' current work to do this cleanup.
> 
> /Staffan
> 
> On 4 maj 2012, at 11:41, Dmitry Samersoff wrote:
> 
>> Nils,
>>
>> os_bsd.cpp:
>>
>> 2359:   // Print warning if unsafe chroot environment detected
>> constant below is
>>unstable_chroot_error
>>
>> 2373:  Never heard about neither about /etc/XXX-release on BSD nor
>>   about mandrake or redhat bsd.
>>   Please fix it as well.
>>
>> 2409:  st->print(os::Bsd::glibc_version())
>>   glibc_version() looks a little bit weird as
>>   none of BSD has glibc.
>>
>>
>> -Dmitry
>>
>>
>>
>> On 2012-05-04 12:26, Nils Loodin wrote:
>>> Updated this with pulling out some shared code to os_posix.cpp.
>>> Some methods were different enough that this wasn't possible though.
>>>
>>> Do you like this better?
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~nloodin/7165755/webrev.00/
>>>
>>> On May 2, 2012, at 14:08 , Nils Loodin wrote:
>>>

 On May 2, 2012, at 14:00 , David Holmes wrote:

> Hi Nils,
>
> On 2/05/2012 9:56 PM, Nils Loodin wrote:
>>> Ignoring Windows (with prints very little) I'd say it is the printing 
>>> of /proc/meminfo that is the main difference. Not sure why printing 
>>> that was necessary ... but if we are going to remove it I think we need 
>>> to know why it was added.
>> Yes, that's the reason.
>> Note that nothing is removed. The method still prints exactly the same 
>> info, but I introduced another method to print briefer info, to be 
>> kinder to tool developers.
>
> The current one prints /proc/meminfo. You turned that code into 
> print_full_memory_info but in the main routine you call 
> print_memory_info. Was that a mistake?

 YES! Glad you caught that :) Guess (or hope) it would have been caught in 
 dump testing otherwise :)

 Regards,
 Nils Loodin


>
> David
>
>> I really don't want to change the output for say, hs_err files, where I 
>> believe this info is used.
>>
>>>
 This can make it hard for tool writers to get a summary that look good 
 and similar for multiple platforms (sizing of gui fields, having to 
 parse info in the tool code etc)
 Lookin at the code, it's in some serious need of refactoring. It would 
 be nice with a method to get a "brief" os info for these kinds of 
 tools that looks similar on all platforms.

 This is my suggested change:
 http://cr.openjdk.java.net/~nloodin/7165755/webrev.00/
>>>
>>> Seems to me some of this could be factored into the top-level OS class 
>>> if we shoehorn Windows into the same shape as the other OSes ;-)
>> This was my first attempt also, but then a lot of empty windows-methods 
>> ensued, which was kind of ugly.
>>
>>> Or at least perhaps put some of the common stuff into os_posix.cpp ?
>> There's a thought!
>> I'll investigate that route, it could get things to look nicer.
>>
>>
>>> Cheers,
>>> David
>> Regards,
>> Nils Loodin
>>

>>>
>>
>>
>> -- 
>> Dmitry Samersoff
>> Java Hotspot development team, SPB04
>> * There will come soft rains ...
> 


-- 
Dmitry Samersoff
Java Hotspot development team, SPB04
* There will come soft rains ...

Re: RFR - BUG7163117: SA Agent can't connect to process on Mac OSX

[Nils Loodin]

Built it and connected to a running java process and ran some commands.
I see other issues, but the connecting part actually works now.

This will probably open up more bugs.

/Nils Loodin

On May 4, 2012, at 12:05 , Staffan Larsen wrote:

> Looks good. I can sponsor the push.
> 
> What kind of testing have you done?
> 
> /Staffan
> 
> On 4 maj 2012, at 11:28, Nils Loodin wrote:
> 
>> looking for cpu-arch "amd64" only, and not "x86_64" as mac osx is using 
>> nowadays.
>> 
>> http://cr.openjdk.java.net/~nloodin/7163117/webrev.00/
>> 
>> 
>> 
>> Need reviews and a  sponsor!
>> Regards,
>> Nils Loodin
> 

RR(S) 7164191: properties.putAll API may fail with ConcurrentModifcationException on multi-thread scenario

[Dmitry Samersoff]

Hi Everybody,

Below is slightly modified version of the fix created by Deven You.

http://cr.openjdk.java.net/~dsamersoff/7164191/webrev.00/

-Dmitry


On 2012-04-26 05:21, Deven You wrote:
> Hi Dmitry,
> 
> Thanks for your help. I have created a CR with internal id 2236492 which
> hasn't be published yet. So please set this internal CR id as duplicate
> to 716419 as well.
> 
> This is the original mail for this problem:
> 
> 
> 
> ---
> 
> Hi core-libs-devs,
> 
> I am not sure if sun.management.Agent belongs to jmx-dev mailing list,
> if so please anyone tell me.
> 
> This issue is that the sun.management.Agent.loadManagementProperties()
> will invoke properties.putAll which will throw
> ConcurrentModifcationException if there are other threads which modify
> the properties concurrently.
> 
> I have made a patch[1] which synchronize the sysProps so that putAll can
> work on multi-thread scenario. The test case is also available in [1].
> 
> Thanks a lot!
> 
> [1] http://cr.openjdk.java.net/~littlee/OJDK-256/webrev.00
> 
> 
> --
> 
> 
> Thanks a lot!
> 
> 
> 
> On 04/25/2012 08:32 PM, Dmitry Samersoff wrote:
>> Deven,
>>
>> CR number is  7164191 .
>>
>> Could you re-send me your original e-mail with problem description and
>> webrev link.
>>
>>   I'll put it to CR comment field.
>>
>>
>> -Dmitry
>>
>>
>>
>> On 2012-04-24 16:15, Dmitry Samersoff wrote:
>>> Deven,
>>>
>>> After close look and off-line discussion with David Holmes,
>>> the changes looks good for me.
>>>
>>> I'll take care of the rest.
>>>
>>> We have one more place in Agent.java executing exactly the same code
>>> so I'll change both of them on your behalf.
>>>
>>> -Dmitry
>>>
>>>
>>> On 2012-04-23 11:43, Deven You wrote:
 Thanks David,

 So is it ok for you to contribute this patch?

 On 04/23/2012 02:36 PM, David Holmes wrote:
> Except of course that Properties is a Hashtable and synchronizes on
> 'this' for all public methods. So locking the properties object in the
> client code will guarantee exclusive access to it.
>
> Sorry about that.
>
> David
> -
>
> On 23/04/2012 4:30 PM, David Holmes wrote:
>> Deven,
>>
>> On 23/04/2012 3:54 PM, Deven You wrote:
>>> On 04/18/2012 02:20 PM, Deven You wrote:
 On 04/18/2012 01:34 PM, Mandy Chung wrote:
>
> I think this could still run into CME. System Properties is not a
> synchronized map and the setter methods (System.setProperty or
> Properties.put method) doesn't synchronize on the Properties
> object.
>
>
> The setter methods I'm referring to are System.setProperty and
> System.getProperties().put().
>
 I have gone through the Agent.java, I think other set/put methods
 related to properties are protected properly.

 public static void agentmain using parseString(args) which return a
 properties which is a local var and is not possible to cause
 concurrent problem when call config_props.putAll(arg_props).

 private static synchronized void startLocalManagementAgent() is
 synchronized already.

 private static synchronized void startRemoteManagementAgent(String
 args) is synchronized also.

 Could you point where the CME may ocurr?
>>> Is there any suggestion from the mailing list?
>> The problem is that System.getProperties() returns a globally
>> accessible
>> set of properties. So even if you prevent the Agent code from
>> modifying
>> those properties concurrently with other use in the Agent, you
>> have no
>> such guard for any other piece of code in the system which might also
>> modify the properties. So the race condition you were trying to fix
>> still exists. I don't see any way to fix this. No matter what you do
>> another thread can modify the system properties while you are
>> iterating
>> them. Instead you need to anticipate the CME and try to recover
>> from it
>> (also non-trivial).
>>
>> Cheers,
>> David Holmes

>>>
>>
> 
> 


-- 
Dmitry Samersoff
Java Hotspot development team, SPB04
* There will come soft rains ...

hg: jdk8/tl/langtools: 7166010: (javac) JavacMessager incorrectly restores log source file

[kumar . x . srinivasan]

Changeset: d10db3576c08
Author:ksrini
Date:  2012-05-04 07:55 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/langtools/rev/d10db3576c08

7166010: (javac) JavacMessager incorrectly restores log source file
Reviewed-by: jjg
Contributed-by: jan.lah...@oracle.com

! src/share/classes/com/sun/tools/javac/processing/JavacMessager.java
+ test/tools/javac/processing/messager/MessagerDiags.java

hg: jdk8/tl/jdk: 7166598: FilteredRowSetImpl can result in Invalid Cursor Position

[lance . andersen]

Changeset: 4580652d9828
Author:lancea
Date:  2012-05-04 16:00 -0400
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/4580652d9828

7166598: FilteredRowSetImpl can result in Invalid Cursor Position
Reviewed-by: lancea
Contributed-by: Knut Anders Hatlen 

! src/share/classes/com/sun/rowset/FilteredRowSetImpl.java

hg: jdk8/tl/jdk: 7153184: NullPointerException when calling SSLEngineImpl.getSupportedCipherSuites

[xuelei . fan]

Changeset: 41d3f7509e00
Author:xuelei
Date:  2012-05-04 17:28 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/41d3f7509e00

7153184: NullPointerException when calling 
SSLEngineImpl.getSupportedCipherSuites
Reviewed-by: weijun

! src/share/classes/sun/security/ssl/SSLContextImpl.java