RFR [XS] 8207832 - ClhsdbCDSCore.java failed with "Couldn't find core file location"

[David Holmes]

On 16/08/2018 11:50 AM, Ioi Lam wrote:
> 
> 
> On 8/15/18 6:14 PM, David Holmes wrote:
>> Hi Ioi,
>>
>> On 15/08/2018 9:50 AM, Ioi Lam wrote:
>>> Please review this simple fix.
>>>
>>>      https://bugs.openjdk.java.net/browse/JDK-8207832
>>> http://cr.openjdk.java.net/~iklam/jdk12/8207832_ClhsdbCDSCore_file_location.v01/ 
>>>
>>>
>>> On Linux, the core file location can be controlled via 
>>> /proc/sys/kernel/core_pattern.
>>> It could have various patterns such as
>>>
>>>     core
>>>     core.%p
>>>     core.%p.something
>>>     /tmp/core
>>>     /tmp/core.%p
>>>     /tmp/core.%p.something
>>
>> It could also have numerous other % variables that the test probably 
>> chokes on:
>>
>> https://www.kernel.org/doc/Documentation/sysctl/kernel.txt
>>
>> I'm not sure the right fix here is not to just make the test more 
>> resilient given the possible the pattern replacements.
>>
> Hi David,
> 
> If we want to handle all cases, we need to implement what kernel.txt 
> says (if only I knew what "%g gid (in initial user namespace)" means). 
> And it would be better to implement that in the VM, so it will benefit 
> all readers of hs_err files, instead of doing that in this test to 
> benefit this test only.

I don't think we intend for the hs_err file to specify exactly where the 
core file may be, only provide some guidance. So I don't think 
implementing a full core-pattern recognition scheme would serve any real 
purpose.

> 
> For now, I am content to just handle the %p case, as that's how our 
> testing hosts are configured. Even for just %p it's hard to do it in the 
> test, as you would need to find the child process's pid.

I would have argued that if the pattern contains a %p then the test can 
just skip that case and vacuously pass (if actually locating the core 
file is what it tries to do). Arguably our test machines should be more 
uniformly set up so that we don't have to deal with all these differences.

> If you don't have any objection, I'' push the patch as is. Can I count 
> you as a reviewer?

No objection. Reviewed.

Thanks,
David

> Thanks
> - Ioi
> 
>> But doing the %p expansion doesn't seem to hurt.
>>
>> Thanks,
>> David
>>
>>> I've tested the fix with all the above patterns, and the %p is correctly
>>> replaced by the process ID.
>>>
>>> Note that the pattern can also be more complex, starting with a 
>>> piping character ("|").
>>> I have not touched that part of the code, since it doesn't seem to 
>>> affect our internal
>>> test infrastructure.
>>>
>>> I also instrumented ClhsdbCDSCore.java so we can more easily diagnose 
>>> such problems
>>> in the future.
>>>
>>> For more info, see 
>>> https://www.kernel.org/doc/Documentation/sysctl/kernel.txt
>>>
>>> I'll validate with hs tiers 1/2/3/4.
>>>
>>> Thanks
>>> - Ioi
>>>
>>>
>