RFR: JDK-8059586: hs_err report should treat redirected core pattern.

[Yasumasa Suenaga]

Hi David,

> I will sponsor if you can prepare the changeset.

Thank you so much!
I've uploaded webrev which contains fixes for your comment.
http://cr.openjdk.java.net/~ysuenaga/JDK-8059586/webrev.05/

Could you push this patch?


Thanks,

Yasumasa


(2014/12/10 15:37), 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
>>>