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-runtime-dev mailing list