RFR: JDK-8059586: hs_err report should treat redirected core pattern.
David Holmes
david.holmes at oracle.com
Mon Dec 15 01:22:56 UTC 2014
On 12/12/2014 10:41 PM, Staffan Larsen wrote:
> Looks good! Sorry for going away for so long - just too many things right now.
Thanks Staffan! I've now submitted this to JPRT. Thomas is also listed
as a reviewer as he does in fact have an openjdk username (sorry about
that misunderstanding).
David
> Thanks,
> /Staffan
>
>> On 11 dec 2014, at 23:50, David Holmes <david.holmes at oracle.com> wrote:
>>
>> Sorry, I've been advised I need a second reviewer with an openjdk user name and as Staffan has not come back to this thread, that means I need one more reviewer from the runtime team please. And please check the email trail on this as it is quite long and I really don't want to revisit covered ground. :) Thanks.
>>
>> David
>>
>> On 10/12/2014 4:37 PM, David Holmes wrote:
>>> Hi Yasumasa,
>>>
>>> On 9/12/2014 11:56 PM, Yasumasa Suenaga wrote:
>>>> David, Thomas,
>>>>
>>>> Thank you so much!
>>>> I wait 2nd reviewer.
>>>
>>> I'm a Reviewer and I think Thomas counts as a reviewer. Plus Staffan had
>>> a look too. So I think this is good to go - though I'll give it till my
>>> morning before finalizing it.
>>>
>>>> BTW, I'm not a committer.
>>>> So I'm also waiting a sponsor :-)
>>>
>>> I will sponsor if you can prepare the changeset.
>>>
>>> Thanks,
>>> David
>>>
>>>>> I'm okay with these changes. Just a minor style nit (no need for
>>>>> updated webrev) can you remove the blank lines in os_linux.cpp:
>>>>>
>>>>> 6011 }
>>>>> 6012
>>>>> 6013 }
>>>>> 6014
>>>>> 6015 }
>>>>>
>>>>> 6057 }
>>>>> 6058
>>>>> 6059 }
>>>>> 6060
>>>>> 6061 }
>>>>>
>>>>> If anyone has any objections please raise them asap.
>>>>
>>>> I will upload new webrev which is fix them after reviewing.
>>>>
>>>>
>>>> Thanks,
>>>>
>>>> Yasumasa
>>>>
>>>>
>>>> (2014/12/09 21:06), Thomas Stüfe wrote:
>>>>> Yes, Sure :-) @Yasumasa : thank you for this patch!
>>>>>
>>>>> Kind regards, Thomas
>>>>>
>>>>> On Dec 9, 2014 9:56 AM, "David Holmes" <david.holmes at oracle.com
>>>>> <mailto:david.holmes at oracle.com>> wrote:
>>>>>
>>>>> Hi Thomas,
>>>>>
>>>>> So can we take this as-is for now and file a RFE to address your
>>>>> concerns?
>>>>>
>>>>> Anybody else object to that?
>>>>>
>>>>> Thanks,
>>>>> David
>>>>>
>>>>> On 9/12/2014 6:09 PM, Thomas Stüfe wrote:
>>>>>
>>>>> Hi David,
>>>>>
>>>>> On Tue, Dec 9, 2014 at 6:39 AM, David Holmes
>>>>> <david.holmes at oracle.com <mailto:david.holmes at oracle.com>
>>>>> <mailto:david.holmes at oracle.__com
>>>>> <mailto:david.holmes at oracle.com>>> wrote:
>>>>>
>>>>> Hi Thomas,
>>>>>
>>>>> On 8/12/2014 8:27 PM, Thomas Stüfe wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> I do not really like the handling of the leading pipe
>>>>> symbol:
>>>>>
>>>>>
>>>>> To be fair to Yasumasa this aspect of the fix has been
>>>>> the same
>>>>> since Oct 15:
>>>>>
>>>>>
>>>>> http://cr.openjdk.java.net/~____ysuenaga/JDK-8059586/webrev.____02/
>>>>> <http://cr.openjdk.java.net/~__ysuenaga/JDK-8059586/webrev.__02/>
>>>>>
>>>>> <http://cr.openjdk.java.net/~__ysuenaga/JDK-8059586/webrev.__02/
>>>>> <http://cr.openjdk.java.net/~ysuenaga/JDK-8059586/webrev.02/>>
>>>>>
>>>>> and was not flagged.
>>>>>
>>>>>
>>>>> You are right, I did not read those mails close enough.
>>>>>
>>>>>
>>>>>
>>>>> So, we read the core_pattern, and if the pipe symbol
>>>>> is detected, we
>>>>> write the core pattern minus the pipe symbol but plus
>>>>> a leading
>>>>> quote to
>>>>> the output; the leading quote then serves as a info
>>>>> to the layer
>>>>> above
>>>>> in os_posix.cpp to treat this case specially. This
>>>>> means the logic
>>>>> spills out of the platform dependend os_linux.cpp to
>>>>> shared code and
>>>>> this is also difficult to read.
>>>>>
>>>>> This comes from the fact that "get_core_path()"
>>>>> assumes the core
>>>>> file is
>>>>> written to the file system. I think it just does not
>>>>> fit
>>>>> anymore, better
>>>>> would be to replace it with something like
>>>>> "os::print_core_file_location(____outputStream* os)",
>>>>> and the OS
>>>>> handles
>>>>> both core path retrieval and the printing. Because
>>>>> then the
>>>>> shared code
>>>>> does not need to know whether core file gets printed
>>>>> traditionally or
>>>>> piped to a executable or whatever.
>>>>>
>>>>>
>>>>> This sounds like a refactoring that I suggested would be
>>>>> too disruptive.
>>>>>
>>>>>
>>>>> http://mail.openjdk.java.net/____pipermail/hotspot-dev/2014-____October/015547.html
>>>>>
>>>>> <http://mail.openjdk.java.net/__pipermail/hotspot-dev/2014-__October/015547.html>
>>>>>
>>>>>
>>>>>
>>>>> <http://mail.openjdk.java.net/__pipermail/hotspot-dev/2014-__October/015547.html
>>>>>
>>>>> <http://mail.openjdk.java.net/pipermail/hotspot-dev/2014-October/015547.html>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> http://mail.openjdk.java.net/____pipermail/hotspot-dev/2014-____October/015557.html
>>>>>
>>>>> <http://mail.openjdk.java.net/__pipermail/hotspot-dev/2014-__October/015557.html>
>>>>>
>>>>>
>>>>>
>>>>> <http://mail.openjdk.java.net/__pipermail/hotspot-dev/2014-__October/015557.html
>>>>>
>>>>> <http://mail.openjdk.java.net/pipermail/hotspot-dev/2014-October/015557.html>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> http://mail.openjdk.java.net/____pipermail/hotspot-dev/2014-____October/015573.html
>>>>>
>>>>> <http://mail.openjdk.java.net/__pipermail/hotspot-dev/2014-__October/015573.html>
>>>>>
>>>>>
>>>>>
>>>>> <http://mail.openjdk.java.net/__pipermail/hotspot-dev/2014-__October/015573.html
>>>>>
>>>>> <http://mail.openjdk.java.net/pipermail/hotspot-dev/2014-October/015573.html>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> I do not think that this would be such a big a change, but it
>>>>> also could
>>>>> be done with another patch.
>>>>>
>>>>> Apart from my reservations I stated above the code looks fine
>>>>> and is
>>>>> definitly an improvement (just last week I was helplessly
>>>>> looking for a
>>>>> core on a machine where core_pattern turned out to be a
>>>>> redirection to
>>>>> another program).
>>>>>
>>>>> Kind Regards, Thomas
>>>>>
>>>>>
>>>>> David
>>>>>
>
More information about the hotspot-dev
mailing list