RFR: JDK-8059586: hs_err report should treat redirected core pattern.
Staffan Larsen
staffan.larsen at oracle.com
Fri Dec 12 12:41:21 UTC 2014
Looks good! Sorry for going away for so long - just too many things right now.
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-runtime-dev
mailing list