RFR: 8343344: Windows attach logic failed to handle a failed open on a pipe

[Kevin Walls]

On Tue, 5 Nov 2024 00:42:23 GMT, Alex Menkov <amenkov at openjdk.org> wrote:

> The fix adds correct handling of "could not open pipe" errors in attach listener instead of assert which causes target VM crash.
> 
> testing: tier1,tier2,hs-tier5-svc

Hi,

List of tests doesn't mention tier4 where the failures were first noticed?


When PipeChannel::open logs error, and returns false, then Win32AttachOperation::open_pipe returns that value, then Win32AttachListener::dequeue() also logs that there was an error?
Is that message just a less detailed duplicate?
(it states whether it's v1 or v2, but maybe that's not that interesting?)

In the v2 case, Win32AttachListener::dequeue()  will log if open succeeded, if !op->read_request().
But AttachOperation::read_request mostly logs its own errors as well.
I think it's only this one:
639     if (buffer_size < 0) {
640       return false;
...where it does not log when returning false. Maybe it should log then, and Win32AttachListener::dequeue() does not need to log?



While are are here, is it possible to address these at the same time?
open/src/hotspot/share/services/attachListener.cpp

typo "ot" -> "or"
631   case ATTACH_API_V2: // <ver>0<size>0<cmd>0<arg>0<arg>0<arg>0
632     if (AttachListener::get_supported_version() < 2) {
633         log_error(attach)("Failed to read request: v2 is unsupported ot disabled"); 

exact -> "exactly"
649     // Must contain exact 'buffer_size' bytes.


Also, is the line 631 comment, with 3 args, misleading? (moving away from exactly 3 arguments...)

Thanks!

-------------

PR Review: https://git.openjdk.org/jdk/pull/21888#pullrequestreview-2415229062