Request for review (S) 7009828: Fix for 6938627 breaks visualvm monitoring when -Djava.io.tmpdir is defined

[Daniel D. Daugherty]

Thumbs up on that version also!

Dan


On 1/12/2011 1:57 PM, Coleen Phillimore wrote:
> Thank you, Dan.  That was right, I'd added a '/' in places because 
> with java.io.tmpdir you could specify a path not ending in a 
> separator.  I had to change this webrev a little bit for hs_err_pid 
> file.  If on windows get_temp_directory() returns "" we don't try to 
> create a file as "\hs_err_pid%p.log".  See:
>
> open webrev at http://cr.openjdk.java.net/~coleenp/7009828_2/
>
> Thanks,
> Coleen
>
> On 1/12/2011 1:44 PM, Daniel D. Daugherty wrote:
>> Kelly,
>>
>> Coleen's fix is a partial anti-delta of the fix for 6938627:
>>
>> src/os/linux/vm/os_linux.cpp
>>   - previously returned "/tmp/"; now returns "/tmp", but the assumed
>>     path separator was obsoleted by other changes made in 6938627
>>     that have _not_ been anti-delta'ed
>>
>> src/os/solaris/vm/os_solaris.cpp
>>   - previously returned "/tmp/"; now returns "/tmp", but the assumed
>>     path separator was obsoleted by other changes made in 6938627
>>     that have _not_ been anti-delta'ed
>>
>> src/os/windows/vm/os_windows.cpp
>>   - removal of query of "java.io.tmpdir" property restores the
>>     behavior before 6938627
>>
>> I'm thumbs up on this partial anti-delta!
>>
>> Dan
>>
>>
>>
>> On 1/12/2011 11:29 AM, Kelly O'Hair wrote:
>>> I'm a little concerned about this change.
>>>
>>> On windows, this relies on GetTempPath, which seems to be influenced 
>>> by the environment
>>> variables TEMP or TMP. So first question is, what happens if TEMP or 
>>> TMP are both undefined
>>> or they refer to a directory that does not exist?
>>> I recently discovered that the VS2010 compiler just doesn't work if 
>>> TMP or TEMP are not defined properly,
>>> and with very little hints as to why.  There is no guarantee that it 
>>> doesn't just return 0 or a path that does
>>> not exist. It can fail and then you have to call GetLastError to 
>>> find out why, most people don't. :^(
>>>
>>> On Solaris/Linux, this just hardwires /tmp, which of course should 
>>> always exist, but it is not influenced
>>> by any environment variables like TMPDIR. I'm not sure what the 
>>> rules should be about obeying the value
>>> of TMPDIR. And as I recall, TMPDIR is supposed to be a system wide 
>>> temp area replacement for /tmp
>>> on Solaris at least. Do we care about TMPDIR? Do we want to allow 
>>> people to re-direct the temp directory
>>> location?
>>>
>>> Should the selection of the VM temp directory be influenced by 
>>> environment variables?
>>> Or is hardwiring to /tmp a good idea?
>>> Should the temp directory be verified as far as existence and 
>>> permissions?
>>> The java.io.tmpdir is /var/tmp on Linux and /tmp on Solaris as I 
>>> recall, does that matter?
>>>
>>> I know this seems like a simple change, but I'm not so sure it is 
>>> that simple.
>>>
>>> When I run tests I often re-define java.io.tmpdir so that I don't 
>>> collide with anyone else running tests
>>> on the same system. Now I can't. You can argue the testcase is bad 
>>> if it doesn't insure it creates unique
>>> filenames in /tmp, so I won't use that as an argument to not change 
>>> things. But I wonder how sloppy
>>> we are in creating files in /tmp, are all uses making sure that they 
>>> are unique and not used by anyone else?
>>>
>>> -kto
>>>
>>> On Jan 12, 2011, at 9:38 AM, Coleen Phillimore wrote:
>>>
>>>> Summary: Change get_temp_directory() back to /tmp and %TEMP% like 
>>>> it always was and where the tools expect it to be.
>>>>
>>>> There is more information in the bug, but essentially changing 
>>>> get_temp_directory() to use java.io.tmpdir breaks a lot of things.
>>>>
>>>> open webrev at http://cr.openjdk.java.net/~coleenp/7009828/
>>>> bug link at http://bugs.sun.com/view_bug.do?bug_id=7009828
>>>>
>>>> Thanks,
>>>> Coleen
>>>>
>>>
>