On Thu, 6 Mar 2025 17:27:56 GMT, Johan Sjölen <jsjo...@openjdk.org> wrote:
>> Alex Menkov has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> feedback
>
> src/hotspot/share/services/attachListener.cpp line 316:
>
>> 314:
>> 315: static jint data_dump(AttachOperation* op, attachStream* out) {
>> 316: out->set_result(JNI_OK); // allow streaming output
>
> I'm wondering, do you think it's useful to add a helper
> `start_streaming_if_available` (or whatever) that just calls
> `set_result(JNI_OK)`? That helper could have a small snippet of documentation
> to explain what's going on. That would probably be neater than a comment.
I don't think helper method is needed (and those comments too :)
I think the better approach would be "operation handler should call set_result
as soon as it knows the result code"
> src/hotspot/share/services/attachListener.cpp line 373:
>
>> 371: // The commands report error if the agent failed to load, so we need
>> to disable streaming output.
>> 372: const char jmx_prefix[] = "ManagementAgent.";
>> 373: if (strncmp(op->arg(0), jmx_prefix, strlen(jmx_prefix)) == 0) {
>
> `const char* jmx_prefix = "ManagementAgent.", not array.
Fixed.
> src/hotspot/share/services/attachListener.cpp line 396:
>
>> 394: if (HAS_PENDING_EXCEPTION) {
>> 395: // We can get an exception during command execution.
>> 396: // In the case _attach_stream->set_result() is already called and
>> operation result code is send
>
> "is senT to the client"
Fixed
> src/jdk.attach/share/classes/sun/tools/attach/HotSpotVirtualMachine.java line
> 379:
>
>> 377:
>> 378: // Reply is "<ver> option1,option2...".
>> 379: int delimPos = message.indexOf(' ');
>
> We can do this in an early-break sort of way. That'd reduce the indentation
> and let's us not to have to check `delimPos` again.
>
>
> int delimPos = message.indexOf(' ');
> int supportedVersion = Integer.parseUnsignedInt(delimPos < 0 ? message :
> message.substring(0, delimPos));
> if (delimPos < 0 || supportedVersion != VERSION_2) {
> return; // No options to parse, or doesn't support options anyway
> }
> /* Insert the parsing stuff here */
To me current implementation looks clearer. And I think it will be simpler to
extend the logic (if we ever need to extend v2 or introduce v3)
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23405#discussion_r1984330438
PR Review Comment: https://git.openjdk.org/jdk/pull/23405#discussion_r1984331040
PR Review Comment: https://git.openjdk.org/jdk/pull/23405#discussion_r1984332637
PR Review Comment: https://git.openjdk.org/jdk/pull/23405#discussion_r1984343797
