RFR (S): 8012260: ciReplay: Include PID into the name of replay data file

[Christian Thalinger]

src/os/windows/vm/os_windows.cpp:

+   if (buflen > INT_MAX)  n = INT_MAX;

Why do we have this check on Windows?

src/share/vm/utilities/ostream.hpp:

!   fileStream(FILE* file, bool need_close=false) { _file = file; _need_close = need_close; }

Please put spaces before and after "=".

+        if (pos > 0 &&
+            Arguments::copy_expand_pid(default_pattern, strlen(default_pattern),
+                                       &buf[pos], buflen-pos)) {
+          fd = open(buf, O_RDWR | O_CREAT | O_TRUNC, 0666);
+        }

Could you factor this code into a method?  It's duplicated three times.

Otherwise this looks good.

-- Chris

On Apr 19, 2013, at 10:03 AM, Vladimir Ivanov <vladimir.x.ivanov at oracle.com> wrote:

> I need one more "official" review. Any volunteers? :-)
> 
> Thanks!
> 
> Best regards,
> Vladimir Ivanov
> 
> On 4/19/13 7:22 PM, Vladimir Kozlov wrote:
>> Looks good.
>> 
>> Thanks,
>> Vladimir
>> 
>> On 4/19/13 7:17 AM, Vladimir Ivanov wrote:
>>> Updated fix:
>>> http://cr.openjdk.java.net/~vlivanov/8012260/webrev.02/
>>> 
>>> Did some additional cleanup: changed get_current_directory to accept
>>> size_t instead of int as a buffer size.
>>> 
>>> Best regards,
>>> Vladimir Ivanov
>>> 
>>> On 4/17/13 8:30 PM, Vladimir Kozlov wrote:
>>>> On 4/17/13 9:03 AM, Vladimir Ivanov wrote:
>>>>> I considered it redundant since fdopen returns NULL when a call fails
>>>>> [1] and I already check replay_data_file. Was my reasoning wrong?
>>>> 
>>>> My understanding was that you can't pass fd == -1 to fdopen() and I
>>>> can't find any docs about such case so you may be right. And I am more
>>>> concern about Windows/Cygwin implementation of fdopen.
>>>> 
>>>> Vladimir
>>>> 
>>>>> 
>>>>> I checked on MacOS and when (fd == -1) it produces the following error
>>>>> message:
>>>>> # Can't open file to dump replay data. Error: Bad file descriptor
>>>>> 
>>>>> Best regards,
>>>>> Vladimir Ivanov
>>>>> 
>>>>> [1] Excerpt from fdopen man page:
>>>>> RETURN VALUES
>>>>>      Upon successful completion fopen(), fdopen(), and freopen()
>>>>> return
>>>>> a FILE pointer.  Otherwise, NULL is returned and the global variable
>>>>> errno is set to indicate the error.
>>>>> 
>>>>> On 4/17/13 7:54 PM, Vladimir Kozlov wrote:
>>>>>> Missing check (fd != -1) before fdopen:
>>>>>> 
>>>>>> 1036       FILE* replay_data_file = fdopen(fd, "w");
>>>>>> 
>>>>>> Vladimir
>>>>>> 
>>>>>> On 4/17/13 8:19 AM, Vladimir Ivanov wrote:
>>>>>>> Thanks for feedback, Vladimir!
>>>>>>> 
>>>>>>> Here's updated version:
>>>>>>> http://cr.openjdk.java.net/~vlivanov/8012260/webrev.01
>>>>>>> 
>>>>>>> Best regards,
>>>>>>> Vladimir Ivanov
>>>>>>> 
>>>>>>> On 4/17/13 9:22 AM, Vladimir Kozlov wrote:
>>>>>>>> Make it product flag since it is ON by default (and it is not
>>>>>>>> diagnostic
>>>>>>>> but error dump).
>>>>>>>> 
>>>>>>>> Also why not reuse code which opens ErrorFile (hs_err*)?
>>>>>>>> 
>>>>>>>> Compilation::compile_only_this_method() allocates fileStream on
>>>>>>>> stack:
>>>>>>>> 
>>>>>>>>   fileStream stream(fopen("c1_compile_only", "wt"));
>>>>>>>> 
>>>>>>>> thanks,
>>>>>>>> Vladimir
>>>>>>>> 
>>>>>>>> On 4/16/13 5:01 PM, Vladimir Ivanov wrote:
>>>>>>>>> http://cr.openjdk.java.net/~vlivanov/8012260/webrev.00
>>>>>>>>> 43 lines changed: 16 ins; 21 del; 6 mod;
>>>>>>>>> 
>>>>>>>>> Changed naming scheme for compilation replay data files from
>>>>>>>>> 'replay.txt' to 'replay_pid%p.log', where %p is substituted w/
>>>>>>>>> pid. It
>>>>>>>>> allows to easily correspond replay file with hs_err file and makes
>>>>>>>>> overwriting of existing files very unlikely.
>>>>>>>>> 
>>>>>>>>> Also, converted ReplayDataFile & DumpReplayDataOnError flags from
>>>>>>>>> develop to diagnostic, since they are useful in product binaries.
>>>>>>>>> Did
>>>>>>>>> some small cleanups.
>>>>>>>>> 
>>>>>>>>> One question still bothers me. There's a memory allocation in
>>>>>>>>> VMError::report_and_die() I don't know how to avoid:
>>>>>>>>> +         fileStream* replay_data_stream = new
>>>>>>>>> (ResourceObj::C_HEAP,
>>>>>>>>> mtCompiler) fileStream(replay_data_file);
>>>>>>>>> 
>>>>>>>>> It's not new, I just moved it up from auxiliary method. Any ideas
>>>>>>>>> how to
>>>>>>>>> make VMError::report_and_die() allocation-free again? :-)
>>>>>>>>> 
>>>>>>>>> Best regards,
>>>>>>>>> Vladimir Ivanov
>>>>>>>>>